read new fields for nodepool storage by deads2k · Pull Request #5207 · Azure/ARO-HCP

deads2k · 2026-05-11T14:13:38Z

part of #5206, must merge after #5206 makes it prod

deads2k · 2026-05-11T14:16:23Z

/hold for #5206 to get to prod

Copilot

Pull request overview

This PR is part of the multi-step migration (following #5206) to store NodePool resources in the generic document–compatible “inline” shape (mirroring GenericDocument[api.HCPOpenShiftClusterNodePool]), and to treat CosmosMetadata.ResourceID as the canonical Cosmos identifier for nodepool persistence and reads.

Changes:

Inline-serialize api.HCPOpenShiftClusterNodePool into the NodePool Cosmos document properties and update nodepool DB conversions to read/write using that inline shape.
Plumb CosmosMetadata.ResourceID through nodepool creation/conversion paths (OCM conversion, frontend decode, tests) and update deep-copy/read-only copy behavior accordingly.
Refresh integration/unit test fixtures and comparers to reflect the new stored document shape (adds id, name, type, systemData, etc. in expected JSON).

Reviewed changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test-integration/utils/databasemutationhelpers/per_resource_comparer.go	Ignore redundant `internalState.internalAPI.cosmosMetadata` during comparisons.
test-integration/frontend/informer_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/09-cosmosCompare-confirm-update/nodepool-basic-node-pool.json	Update expected Cosmos nodepool document shape with inline fields (`id`, `name`, `type`, etc.).
test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/09-cosmosCompare-confirm-update/nodepool-02.json	Update expected Cosmos nodepool document shape with inline fields (`id`, `name`, `type`, etc.).
test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/01-load-old-data/nodepool-node-pool-02.json	Update stored “old data” fixture to include inline nodepool fields.
test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/01-load-old-data/nodepool-basic-node-pool.json	Update stored “old data” fixture to include inline nodepool fields.
test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-node-pool-02.json	Update expected Cosmos nodepool document shape for current create flow.
test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-basic-node-pool.json	Update expected Cosmos nodepool document shape for current create flow.
test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/nodepool-node-pool-02.json	Update expected Cosmos shape for migration “read new data” verification.
test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/nodepool-basic-node-pool.json	Update expected Cosmos shape for migration “read new data” verification.
test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/nodepool-node-pool-02.json	Update migration fixture to include inline nodepool fields.
test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/nodepool-basic-node-pool.json	Update migration fixture to include inline nodepool fields.
test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/nodepool-node-pool-02.json	Update expected Cosmos shape for “confirm migration” step.
test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/nodepool-basic-node-pool.json	Update expected Cosmos shape for “confirm migration” step.
test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/nodepool-node-pool-02.json	Update “migrate old data” fixture to include inline fields.
test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/nodepool-basic-node-pool.json	Update “migrate old data” fixture to include inline fields.
test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-nodepool-operation/05-cosmosCompare-final-state/nodepool-test-nodepool.json	Update expected Cosmos nodepool doc shape during delete-with-pending-operation flow.
test-integration/backend/controllers/mismatches/artifacts/nodepool/remove_orphaned_nodepool_descendents/00-load-initial-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/nodepool/present_nodepool/99-cosmosCompare-end-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/nodepool/present_nodepool/00-load-initial-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/externalauth/remove_orphaned_externalauth_descendents/99-cosmosCompare-end-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/externalauth/remove_orphaned_externalauth_descendents/00-load-initial-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/99-cosmosCompare-end-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/00-load-initial-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
test-integration/backend/controllers/mismatches/artifacts/cluster/remove_orphaned_cluster_descendents/00-load-initial-state/nodepool-basic.json	Update backend mismatch fixtures to new inline nodepool storage shape.
internal/ocm/convert.go	Populate nodepool `CosmosMetadata.ResourceID` during CS→internal conversion.
internal/databasetesting/mock_dbclient_test.go	Update mock nodepool construction to include `CosmosMetadata.ResourceID`.
internal/database/types_nodepool.go	Inline `api.HCPOpenShiftClusterNodePool` into Cosmos `NodePoolProperties` for generic-doc compatibility.
internal/database/convert_nodepool.go	Use `CosmosMetadata.ResourceID` as canonical for Cosmos partitioning/indexing; read from inline shape.
internal/database/convert_nodepool_test.go	Adjust fuzzing/tests for new CosmosMetadata + ETag preservation path.
internal/database/convert_cluster_test.go	Update generic round-trip helper to clear nodepool `ExistingCosmosUID` from CosmosMetadata.
internal/conversion/readonly_nodepool.go	Preserve `CosmosMetadata` (resourceID/etag) across replace round-trips; remove old CosmosETag copying.
internal/api/zz_generated.deepcopy.go	Deep-copy `CosmosMetadata` for nodepools.
internal/api/v20251223preview/zero_value_roundtrip_test.go	Baseline nodepool now includes `CosmosMetadata.ResourceID`.
internal/api/v20251223preview/nodepools_methods.go	Convert-to-external uses `ResourceID` for `id`; ConvertToInternal also sets `ResourceID`.
internal/api/v20251223preview/nodepools_methods_test.go	Ensure nodepool tests include `CosmosMetadata.ResourceID`.
internal/api/v20251223preview/conversion_fuzz_test.go	Add fuzzer customization for `api.CosmosMetadata` and adjust ExistingCosmosUID handling.
internal/api/v20240610preview/nodepools_methods.go	Convert-to-external uses `ResourceID` for `id`; ConvertToInternal also sets `ResourceID`.
internal/api/v20240610preview/nodepools_methods_test.go	Ensure nodepool tests include `CosmosMetadata.ResourceID`.
internal/api/v20240610preview/conversion_fuzz_test.go	Add fuzzer customization for `api.CosmosMetadata` and adjust ExistingCosmosUID handling.
internal/api/types_nodepool.go	Embed `CosmosMetadata` into nodepool internal type; remove legacy CosmosETag shim.
internal/admission/admit_cluster_test.go	Ensure nodepool test objects include `CosmosMetadata.ResourceID`.
frontend/pkg/frontend/node_pool.go	Set `ResourceID` on create; validate stored nodepool has CosmosMetadata.ResourceID on read.
backend/pkg/listertesting/slice_listers_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/informers/informers_test.go	Ensure informer test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/read_and_persist_nodepool_scoped_maestro_readonly_bundles_content_controller_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/operationcontrollers/test_helpers_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/nodepoolpropertiescontroller/node_pool_properties_sync_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/metricscontrollers/resource_metrics_controller_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/datadumpcontrollers/cs_state_dump_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.
backend/pkg/controllers/create_nodepool_scoped_maestro_readonly_bundles_controller_test.go	Ensure test nodepools include `CosmosMetadata.ResourceID`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	if internalNodePool.ResourceID == nil {
+		return nil, fmt.Errorf("stored nodepool document is missing cosmosMetadata.resourceID")
+	}
+	if !strings.EqualFold(internalNodePool.ResourceID.String(), resourceID.String()) {
+		return nil, fmt.Errorf("unexpected resourceID: %s", internalNodePool.ResourceID.String())
 	}
 	internalNodePool.ID = resourceID



 	idString := ""
-	if from.ID != nil {
-		idString = from.ID.String()
+	if from.ResourceID != nil {
+		idString = from.ResourceID.String()
 	}


 	idString := ""
-	if from.ID != nil {
-		idString = from.ID.String()
+	if from.ResourceID != nil {
+		idString = from.ResourceID.String()
 	}


miguelsorianod · 2026-05-12T08:22:33Z

 		},
 	}

+	cosmosObj.InternalState.InternalAPI.CosmosMetadata = api.CosmosMetadata{}


Why are we now unsetting the cosmos obj CosmosMetadata top-level attribute as well as the one in InternalState.InternalAPI?

Is it because at some point we will stop having both the CosmosMetadata top-level attribute as well as InternalState.InternalAPI?

miguelsorianod · 2026-05-12T08:27:06Z

-		},
-		Location: cosmosObj.InternalState.InternalAPI.Location,
-		Tags:     resourceDoc.Tags,
+	internalObj := cosmosObj.DeepCopy()


Is it correct/desired that calling DeepCopy of cosmosObj returns the internal api type? I understand this occurs because we now inline the internal api type on the cosmos type and because of that the DeepCopy method is promoted to the cosmos type, but it surprised me as usually when using the DeepCopy method on a type you expect the type itself being deepcopied and not just an inlined attribute of it.

miguelsorianod · 2026-05-12T08:29:52Z

-		Location: cosmosObj.InternalState.InternalAPI.Location,
-		Tags:     resourceDoc.Tags,
+	internalObj := cosmosObj.DeepCopy()
+	internalObj.ExistingCosmosUID = cosmosObj.ID


Do we do this because the one the inlined object that we retrieve from cosmos doesn't have that information? the same question applies to ETag and ResourceID

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

deads2k · 2026-05-18T16:27:42Z

/hold cancel

deads2k · 2026-05-18T16:30:47Z

query confirms all data is ready for this

stevekuznetsov · 2026-05-18T16:47:00Z

/lgtm
/hold cancel

openshift-ci · 2026-05-18T16:47:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-bot · 2026-05-18T17:50:12Z

/retest-required

Remaining retests: 0 against base HEAD f64835f and 2 for PR HEAD f69ba4a in total

openshift-merge-bot · 2026-05-19T02:55:28Z

/retest-required

Remaining retests: 0 against base HEAD eb44277 and 1 for PR HEAD f69ba4a in total

Copilot AI review requested due to automatic review settings May 11, 2026 14:13

openshift-ci Bot requested review from janboll and mbarnes May 11, 2026 14:13

openshift-ci Bot added the approved label May 11, 2026

Copilot started reviewing on behalf of deads2k May 11, 2026 14:14 View session

This was referenced May 11, 2026

stop serializing old nodepool fields #5208

Closed

switch to using generic storage for nodepools #5209

Closed

add genericdocument compatible serialization of nodepools #5206

Merged

openshift-ci Bot added the do-not-merge/hold label May 11, 2026

Copilot AI reviewed May 11, 2026

View reviewed changes

miguelsorianod reviewed May 12, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase label May 12, 2026

deads2k changed the title ~~switch to using generic storage for nodepools~~ read new fields for nodepool storage May 18, 2026

read new nodepool content

f69ba4a

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

openshift-ci Bot removed the do-not-merge/hold label May 18, 2026

deads2k force-pushed the cs-173-read-new branch from f6d4580 to f69ba4a Compare May 18, 2026 16:28

openshift-ci Bot removed the needs-rebase label May 18, 2026

openshift-ci Bot assigned stevekuznetsov May 18, 2026

openshift-ci Bot added the lgtm label May 18, 2026

openshift-merge-bot Bot merged commit d73e23c into main May 19, 2026
15 checks passed

openshift-merge-bot Bot deleted the cs-173-read-new branch May 19, 2026 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read new fields for nodepool storage#5207

read new fields for nodepool storage#5207
openshift-merge-bot[bot] merged 1 commit into
mainfrom
cs-173-read-new

deads2k commented May 11, 2026

Uh oh!

deads2k commented May 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

miguelsorianod May 12, 2026

Uh oh!

miguelsorianod May 12, 2026

Uh oh!

miguelsorianod May 12, 2026

Uh oh!

miguelsorianod May 12, 2026

Uh oh!

deads2k commented May 18, 2026

Uh oh!

deads2k commented May 18, 2026

Uh oh!

stevekuznetsov commented May 18, 2026

Uh oh!

openshift-ci Bot commented May 18, 2026

Uh oh!

openshift-merge-bot Bot commented May 18, 2026

Uh oh!

openshift-merge-bot Bot commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

deads2k commented May 11, 2026

Uh oh!

deads2k commented May 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

miguelsorianod May 12, 2026

Choose a reason for hiding this comment

Uh oh!

miguelsorianod May 12, 2026

Choose a reason for hiding this comment

Uh oh!

miguelsorianod May 12, 2026

Choose a reason for hiding this comment

Uh oh!

miguelsorianod May 12, 2026

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 18, 2026

Uh oh!

deads2k commented May 18, 2026

Uh oh!

stevekuznetsov commented May 18, 2026

Uh oh!

openshift-ci Bot commented May 18, 2026

Uh oh!

openshift-merge-bot Bot commented May 18, 2026

Uh oh!

openshift-merge-bot Bot commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants