use generic storage for clusters#5180
Conversation
|
[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 |
There was a problem hiding this comment.
Pull request overview
This PR completes the migration step of switching cluster persistence from the bespoke HCPCluster Cosmos document wrapper to the generic GenericDocument[api.HCPOpenShiftCluster] storage model, and updates tests/artifacts accordingly.
Changes:
- Switched cluster CRUD/list/transaction plumbing (and DB test mocks) to use
GenericDocument[api.HCPOpenShiftCluster]instead of the legacyHCPClusterdocument type. - Inlined
CosmosMetadataintoapi.HCPOpenShiftClusterand updated conversions/deepcopy/read-only-copy logic so ResourceID/etag/UID metadata round-trips correctly. - Updated integration test fixtures and expected Cosmos JSON artifacts to the new “generic document + inlined cluster content” shape.
Reviewed changes
Copilot reviewed 99 out of 99 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test-integration/utils/databasemutationhelpers/per_resource_comparer.go | Ignores additional volatile/UID-like fields introduced/exposed by the new cluster serialization shape. |
| test-integration/frontend/informer_test.go | Adds CosmosMetadata.ResourceID when constructing clusters for informer integration tests. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/09-cosmosCompare-confirm-update/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for NodePool CRUD old-data scenario. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/read-old-data/01-load-old-data/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for NodePool CRUD old-data load. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for NodePool CRUD current create. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json | Updates expected Cosmos cluster document shape for cluster deleting state. |
| test-integration/frontend/artifacts/FrontendCRUD/NodePool/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json | Updates expected Cosmos cluster document shape for cluster creating state. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/18-cosmosCompare-confirm/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for migration read-new-data confirmation. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/read-new-data/01-load-old-data/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for migration read-new-data old-data load. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/99-cosmosCompare-confirm-migration/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for migration final confirmation. |
| test-integration/frontend/artifacts/FrontendCRUD/Migration/migrate-old-data/01-load-old-data/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for migration old-data load. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/read-old-data/09-cosmosCompare-confirm-update/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for ExternalAuth CRUD old-data scenario. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/read-old-data/01-load-old-data/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for ExternalAuth CRUD old-data load. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/create-current/06-cosmosCompare-ending-content/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for ExternalAuth CRUD current create. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/cluster-deleting/02-loadCosmos-cluster/cosmos-01-cluster.json | Updates expected Cosmos cluster document shape for ExternalAuth cluster deleting state. |
| test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/cluster-creating/02-loadCosmos-cluster/cosmos-01-cluster.json | Updates expected Cosmos cluster document shape for ExternalAuth cluster creating state. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/read-old-data/09-cosmosCompare-confirm-update/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for Cluster CRUD old-data scenario. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/read-old-data/01-load-old-data/create-with-tags.json | Updates expected Cosmos cluster document shape for Cluster CRUD old-data load. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-nodepool-operation/05-cosmosCompare-final-state/cluster-test-cluster.json | Updates expected Cosmos cluster document shape for delete w/ pending nodepool op. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-cluster-operation/03-cosmosCompare-final-state/cluster-test-cluster.json | Updates expected Cosmos cluster document shape for delete w/ pending cluster op. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/create-current/09-cosmosCompare-confirm-update/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for Cluster CRUD current update. |
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/create-current/02-cosmosCompare-confirm-content/cluster-create-with-tags.json | Updates expected Cosmos cluster document shape for Cluster CRUD current create. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/revoke/01-load-initial-cosmos-state/01-cluster.json | Updates admin-credential test Cosmos seed data to new cluster document shape. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/revoke-provisioning-conflict/01-load-initial-cosmos-state/01-cluster.json | Updates admin-credential conflict seed data to new shape. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/revoke-forbidden-afec-pending/01-load-initial-cosmos-state/01-cluster.json | Updates AFEC-pending seed data to new shape. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/revoke-forbidden-afec-missing/01-load-initial-cosmos-state/01-cluster.json | Updates AFEC-missing seed data to new shape. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/request/01-load-initial-cosmos-state/01-cluster.json | Updates admin-credential request seed data to new shape. |
| test-integration/frontend/artifacts/FrontendCRUD/AdminCredentials/request-provisioning-conflict/01-load-initial-cosmos-state/01-cluster.json | Updates admin-credential request conflict seed data to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/12-untypedList-resourcegroup/create-with-tags-cluster.json | Updates untyped CRUD expected Cosmos cluster document to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/10-untypedList-resourcegroup/create-with-tags-cluster.json | Updates untyped CRUD expected Cosmos cluster document to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/07-untypedListRecursive-resourcegroup-via-child/create-with-tags-cluster.json | Updates untyped recursive list expected doc to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/05-untypedListRecursive-subscription/create-with-tags-cluster.json | Updates untyped recursive list expected doc to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/04-untypedListRecursive-resourcegroup/create-with-tags-cluster.json | Updates untyped recursive list expected doc to new shape. |
| test-integration/frontend/artifacts/DatabaseCRUD/UntypedCRUD/basic/01-load-initial/create-with-tags-cluster.json | Updates untyped CRUD initial seed doc to new shape. |
| test-integration/backend/launch/metrics_test.go | Sets CosmosMetadata.ResourceID in metrics integration test cluster. |
| test-integration/backend/controllers/mismatches/artifacts/nodepool/present_nodepool/00-load-initial-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/controller_under_missing_nodepool_deleted/99-cosmosCompare-end-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/controller_under_missing_nodepool_deleted/00-load-initial-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/99-cosmosCompare-end-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/delete_orphaned_cosmos/all_parents_exist/00-load-initial-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/cluster/present_cluster/99-cosmosCompare-end-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/mismatches/artifacts/cluster/present_cluster/00-load-initial-state/cluster.json | Updates backend mismatch artifact cluster doc to new shape. |
| test-integration/backend/controllers/do_nothing/artifacts/sync_cluster/00-load-initial-state/cluster.json | Updates backend “do nothing” artifact cluster doc to new shape. |
| test-integration/admin/artifacts/AdminCRUD/HCP/lowercase-middleware/00-load-initial-cosmos-state/01-cluster.json | Updates admin CRUD artifact cluster doc to new shape. |
| test-integration/admin/artifacts/AdminCRUD/HCP/hello-world/00-load-initial-cosmos-state/01-cluster.json | Updates admin CRUD artifact cluster doc to new shape. |
| test-integration/admin/artifacts/AdminCRUD/HCP/cosmosdump/00-load-initial-cosmos-state/01-cluster.json | Updates admin CRUD artifact cluster doc to new shape. |
| test-integration/admin/artifacts/AdminCRUD/HCP/breakglass/00-load-initial-cosmos-state/01-cluster.json | Updates admin CRUD artifact cluster doc to new shape. |
| internal/serverutils/dump_billing_test.go | Sets CosmosMetadata.ResourceID in billing dump tests. |
| internal/databasetesting/mock_global_lister.go | Switches mock global cluster lister to GenericDocument[HCPOpenShiftCluster]. |
| internal/databasetesting/mock_dbclient.go | Uses GenericDocument[HCPOpenShiftCluster] and CosmosGenericToInternal for cluster reads. |
| internal/databasetesting/mock_dbclient_test.go | Updates cluster test objects to include CosmosMetadata.ResourceID. |
| internal/databasetesting/mock_crud.go | Switches mock HCP cluster CRUD to GenericDocument[HCPOpenShiftCluster]. |
| internal/database/types_hcpcluster.go | Removes legacy HCPCluster Cosmos wrapper type. |
| internal/database/transaction.go | Switches transaction GetItem casting for clusters to GenericDocument[HCPOpenShiftCluster]. |
| internal/database/global_lister.go | Switches global lister for clusters to GenericDocument[HCPOpenShiftCluster]. |
| internal/database/crud_hcpcluster.go | Switches HCPCluster CRUD implementation to GenericDocument[HCPOpenShiftCluster]. |
| internal/database/convert_generic.go | Ensures defaults are applied during generic Cosmos→internal conversion (via Defaulter). |
| internal/database/convert_defaults_consistency_test.go | Updates cluster defaulting tests to use GenericDocument + CosmosGenericToInternal. |
| internal/database/convert_cluster.go | Removes cluster-specific Cosmos conversion functions, leaving shared helpers. |
| internal/database/convert_cluster_test.go | Removes old cluster round-trip conversion tests; keeps EnsureDefaults tests and helper. |
| internal/database/convert_any.go | Removes HCPCluster conversion cases and relies on generic conversion for clusters. |
| internal/conversion/readonly_cluster.go | Copies CosmosMetadata during update flows to preserve ResourceID/etag across replace round-trips. |
| internal/api/zz_generated.deepcopy.go | Ensures HCPOpenShiftCluster.DeepCopy copies inlined CosmosMetadata. |
| internal/api/v20251223preview/zero_value_roundtrip_test.go | Initializes clusters with CosmosMetadata.ResourceID for roundtrip tests. |
| internal/api/v20251223preview/hcpopenshiftclusters_methods.go | Uses the case-preserving ResourceID when producing external id; sets internal ResourceID on conversion. |
| internal/api/v20251223preview/conversion_fuzz_test.go | Adds fuzz customizations for CosmosMetadata; removes old cluster CosmosETag clearing. |
| internal/api/v20240610preview/hcpopenshiftclusters_methods.go | Same as v20251223preview: external id from ResourceID, and sets internal ResourceID. |
| internal/api/v20240610preview/conversion_fuzz_test.go | Adds fuzz customizations for CosmosMetadata; removes old cluster CosmosETag clearing. |
| internal/api/types_cluster.go | Inlines CosmosMetadata into HCPOpenShiftCluster; removes legacy CosmosETag/ExistingCosmosUID plumbing. |
| frontend/pkg/frontend/helpers_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| frontend/pkg/frontend/frontend_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| frontend/pkg/frontend/cluster.go | Sets ResourceID via SetResourceID during create; validates against internalCluster.ResourceID on reads. |
| backend/pkg/listertesting/slice_listers_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/informers/informers_test.go | Updates informer test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go | Updates test cluster to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/upgradecontrollers/control_plane_active_version_controller_test.go | Updates test cluster to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/read_and_persist_cluster_scoped_maestro_readonly_bundles_content_controller_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/operationcontrollers/test_helpers_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/nodepoolpropertiescontroller/node_pool_properties_sync_test.go | Updates test cluster to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/metricscontrollers/resource_metrics_controller_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/maestro_readonly_bundle_helpers_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/datadumpcontrollers/cs_state_dump_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/create_cluster_scoped_maestro_readonly_bundles_controller_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/clusterpropertiescontroller/cluster_properties_sync_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/billingcontrollers/orphaned_billing_cleanup_test.go | Updates test cluster(s) to include CosmosMetadata.ResourceID. |
| backend/pkg/controllers/billingcontrollers/create_billing_doc_test.go | Updates test cluster to include CosmosMetadata.ResourceID. |
| admin/server/handlers/hcp/serialconsole_test.go | Updates test clusters to include CosmosMetadata.ResourceID. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
362d72c to
0979ae4
Compare
0979ae4 to
169b40c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 100 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/database/convert_cluster_test.go:61
- There is no longer a round-trip test that exercises cluster Cosmos conversion (internal -> Cosmos -> internal), even though this PR changes cluster storage to
GenericDocument[api.HCPOpenShiftCluster]. NodePool/ExternalAuth still have round-trip conversion tests, so cluster conversion seems untested at the unit level now. Adding a cluster round-trip test here would help catch regressions in serialization/defaulting behavior during the ongoing storage migration.
169b40c to
39bd36b
Compare
| type Defaulter interface { | ||
| EnsureDefaults() | ||
| } |
There was a problem hiding this comment.
I like this, but I think I'd prefer it in the api package and add static checks to the resource types, like:
var _ Defaulter = &HCPOpenShiftCluster{}
There was a problem hiding this comment.
I like this, but I think I'd prefer it in the
apipackage and add static checks to the resource types, like:
Currently some do and some don't. As a future improvement, ok.
|
/hold until #5171 makes it to prod |
|
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 4 of #5095
Can merge at the same time as #5174, separated for ease of review.