switch to using generic storage for nodepools#5209
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold for #5207 to get to prod |
There was a problem hiding this comment.
Pull request overview
This PR is part of the staged migration to store nodepools in Cosmos using the shared GenericDocument format instead of the bespoke database.NodePool document shape, and updates conversions/tests/artifacts accordingly.
Changes:
- Switch nodepool CRUD/listers/transactions and mock DB client paths to use
database.GenericDocument[api.HCPOpenShiftClusterNodePool]+CosmosGenericToInternal. - Inline/persist
cosmosMetadataonapi.HCPOpenShiftClusterNodePool, update read-only copy behavior, and adjust API-version conversions to source ARMidfrom the case-preserving stored ResourceID. - Update integration artifacts/tests to match the new persisted JSON shape for nodepools.
Reviewed changes
Copilot reviewed 70 out of 70 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test-integration/utils/databasemutationhelpers/per_resource_comparer.go | Ignore redundant inline internalState.internalAPI.cosmosMetadata in integration comparisons. |
| test-integration/frontend/informer_test.go | Add CosmosMetadata.ResourceID to nodepool test objects. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/09-cosmosCompare-confirm-update/nodepool-basic-node-pool.json | Update expected nodepool document JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/09-cosmosCompare-confirm-update/nodepool-02.json | Update expected nodepool document JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/01-load-old-data/nodepool-node-pool-02.json | Update seed “old data” nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/01-load-old-data/nodepool-basic-node-pool.json | Update seed “old data” nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-node-pool-02.json | Update expected created nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-basic-node-pool.json | Update expected created nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/nodepool-node-pool-02.json | Update expected migrated/read nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/nodepool-basic-node-pool.json | Update expected migrated/read nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/nodepool-node-pool-02.json | Update seed JSON for migration scenarios to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/nodepool-basic-node-pool.json | Update seed JSON for migration scenarios to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/nodepool-node-pool-02.json | Update expected post-migration nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/nodepool-basic-node-pool.json | Update expected post-migration nodepool JSON to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/nodepool-node-pool-02.json | Update seed JSON for migrate-old-data flows to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/nodepool-basic-node-pool.json | Update seed JSON for migrate-old-data flows to generic/inline shape. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-nodepool-operation/05-cosmosCompare-final-state/nodepool-test-nodepool.json | Update expected nodepool JSON in cluster-delete scenario to generic/inline shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/09-untypedList-immutability-cluster/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/08-untypedListRecursive-immutability-cluster/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/07-untypedListRecursive-resourcegroup-via-child/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/05-untypedListRecursive-subscription/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/04-untypedListRecursive-resourcegroup/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/02-untypedListRecursive-immutability/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/01-load-initial/immutability-np.json | Remove old nested internalState payload from expected untyped outputs. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/remove_orphaned_nodepool_descendents/00-load-initial-state/nodepool-basic.json | Update nodepool mismatch-test artifact to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/present_nodepool/99-cosmosCompare-end-state/nodepool-basic.json | Update nodepool mismatch-test artifact to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/present_nodepool/00-load-initial-state/nodepool-basic.json | Update nodepool mismatch-test artifact to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/externalauth/remove_orphaned_externalauth_descendents/99-cosmosCompare-end-state/nodepool-basic.json | Update nodepool descendant artifact to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/externalauth/remove_orphaned_externalauth_descendents/00-load-initial-state/nodepool-basic.json | Update nodepool descendant artifact to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/99-cosmosCompare-end-state/nodepool-basic.json | Update orphan-check artifacts to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/00-load-initial-state/nodepool-basic.json | Update orphan-check artifacts to generic/inline shape. |
| test-integration/backend/controllers/mismatches/artifacts/cluster/remove_orphaned_cluster_descendents/00-load-initial-state/nodepool-basic.json | Update cluster-descendant artifact nodepool to generic/inline shape. |
| internal/ocm/convert.go | Populate nodepool CosmosMetadata.ResourceID during CS->internal conversion. |
| internal/databasetesting/mock_resources_global_lister.go | Switch mock nodepool global lister to generic document type. |
| internal/databasetesting/mock_resources_db_client.go | Switch mock transaction GetItem for nodepools to generic document conversion. |
| internal/databasetesting/mock_resources_crud.go | Switch mock nodepool CRUD to generic document type parameters. |
| internal/databasetesting/mock_dbclient_test.go | Add CosmosMetadata.ResourceID to nodepool used in mock DB tests. |
| internal/database/types_nodepool.go | Remove legacy database.NodePool Cosmos type. |
| internal/database/transaction.go | Switch nodepool transaction casting to GenericDocument[HCPOpenShiftClusterNodePool]. |
| internal/database/global_lister.go | Switch nodepool global lister to GenericDocument[HCPOpenShiftClusterNodePool]. |
| internal/database/crud_nested_resource.go | Update nodepool CRUD interface assertion to generic document type. |
| internal/database/crud_hcpcluster.go | Switch nodepool nested CRUD to generic document type. |
| internal/database/convert_nodepool.go | Remove legacy nodepool-specific conversion functions. |
| internal/database/convert_nodepool_test.go | Remove legacy nodepool cosmos conversion tests; keep EnsureDefaults unit test. |
| internal/database/convert_generic.go | Ensure EnsureDefaults() is applied during generic Cosmos->internal conversion. |
| internal/database/convert_defaults_consistency_test.go | Update nodepool “pre-existing data” defaulting test to use generic documents. |
| internal/database/convert_cluster_test.go | Update generic round-trip test helper to clear ExistingCosmosUID via embedded cosmosmetadata on nodepools. |
| internal/database/convert_any.go | Remove nodepool custom conversion branch so nodepools use generic conversions. |
| internal/conversion/readonly_nodepool.go | Copy embedded CosmosMetadata as read-only state across replace round-trips. |
| internal/api/zz_generated.deepcopy.go | Deep-copy nodepool CosmosMetadata in generated deep-copy implementation. |
| internal/api/v20251223preview/zero_value_roundtrip_test.go | Provide baseline nodepool with cosmosmetadata populated for round-trip tests. |
| internal/api/v20251223preview/nodepools_methods.go | Use stored ResourceID for external id and set ResourceID on ConvertToInternal. |
| internal/api/v20251223preview/nodepools_methods_test.go | Populate cosmosmetadata for nodepool round-trip tests. |
| internal/api/v20251223preview/conversion_fuzz_test.go | Add fuzzer customization for api.CosmosMetadata and adjust fields reset. |
| internal/api/v20240610preview/nodepools_methods.go | Use stored ResourceID for external id and set ResourceID on ConvertToInternal. |
| internal/api/v20240610preview/nodepools_methods_test.go | Populate cosmosmetadata for nodepool round-trip tests. |
| internal/api/v20240610preview/conversion_fuzz_test.go | Add fuzzer customization for api.CosmosMetadata and adjust fields reset. |
| internal/api/types_nodepool.go | Inline CosmosMetadata onto nodepool internal type; remove old etag/existingCosmosUID plumbing. |
| internal/admission/admit_cluster_test.go | Populate nodepool cosmosmetadata in admission tests. |
| frontend/pkg/frontend/node_pool.go | Ensure write path sets nodepool ResourceID; validate stored ResourceID on read. |
| backend/pkg/listertesting/slice_listers_test.go | Populate nodepool cosmosmetadata in lister tests. |
| backend/pkg/informers/informers_test.go | Populate nodepool cosmosmetadata in informer tests. |
| backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go | Populate nodepool cosmosmetadata in upgrade controller tests. |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go | Populate nodepool cosmosmetadata in control plane desired version tests. |
| backend/pkg/controllers/read_and_persist_nodepool_scoped_maestro_readonly_bundles_content_controller_test.go | Populate nodepool cosmosmetadata in maestro bundle persistence tests. |
| backend/pkg/controllers/operationcontrollers/test_helpers_test.go | Populate nodepool cosmosmetadata in operation controller fixtures. |
| backend/pkg/controllers/nodepoolpropertiescontroller/node_pool_properties_sync_test.go | Populate nodepool cosmosmetadata in properties sync tests. |
| backend/pkg/controllers/metricscontrollers/resource_metrics_controller_test.go | Populate nodepool cosmosmetadata in metrics tests. |
| backend/pkg/controllers/datadumpcontrollers/cs_state_dump_test.go | Populate nodepool cosmosmetadata in state dump tests. |
| backend/pkg/controllers/create_nodepool_scoped_maestro_readonly_bundles_controller_test.go | Populate nodepool cosmosmetadata across multiple readonly bundle controller tests. |
Comments suppressed due to low confidence (1)
internal/database/convert_nodepool_test.go:27
- This file no longer has a round-trip test covering NodePool Cosmos <-> internal conversion now that nodepools moved to GenericDocument/CosmosGenericToInternal. Consider adding a focused test that does InternalToCosmosGeneric + CosmosGenericToInternal for HCPOpenShiftClusterNodePool (including verifying CosmosETag/ExistingCosmosUID handling and defaulting via EnsureDefaults) so this migration path is guarded.
func TestNodePoolEnsureDefaults(t *testing.T) {
tests := []struct {
name string
diskStorageAccountType api.DiskStorageAccountType
wantDiskStorageAccountType api.DiskStorageAccountType
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // HCPOpenShiftClusterNodePool. | ||
| CosmosETag azcore.ETag `json:"-"` | ||
| } | ||
|
|
| idString := "" | ||
| if from.ID != nil { | ||
| idString = from.ID.String() | ||
| if from.ResourceID != nil { | ||
| idString = from.ResourceID.String() | ||
| } | ||
|
|
| if from.ID != nil { | ||
| idString = from.ID.String() | ||
| if from.ResourceID != nil { | ||
| idString = from.ResourceID.String() |
| cosmosData := ret.(arm.CosmosPersistable).GetCosmosData() | ||
| cosmosData.ExistingCosmosUID = cosmosObj.ID | ||
| ret.SetEtag(cosmosObj.CosmosETag) | ||
|
|
||
| if defaulter, ok := ret.(Defaulter); ok { |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
replaced by #5395 |
part of #5206, must merge after #5207 makes it to prod