CNTRLPLANE-3604: Capi v1beta2 storage migration#8938
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@clebs: This pull request references CNTRLPLANE-3604 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clebs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/cli
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds CAPI CRD storage-version migration support. It introduces storage-version-specific CRD override generation, installer flags and migration guards, operator wiring for Sequence Diagram(s)sequenceDiagram
participant Installer
participant HyperShiftOperator
participant CRDMigrator
participant KubernetesAPI
Installer->>KubernetesAPI: Apply CAPI CRDs with v1beta2 storage
Installer->>HyperShiftOperator: Configure CAPI_STORAGE_VERSION
HyperShiftOperator->>CRDMigrator: Start migration controller
CRDMigrator->>KubernetesAPI: Migrate resources and update CRD status
CRDMigrator->>KubernetesAPI: Persist migration status
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8938 +/- ##
==========================================
+ Coverage 44.11% 44.19% +0.07%
==========================================
Files 772 774 +2
Lines 96226 96396 +170
==========================================
+ Hits 42448 42599 +151
- Misses 50832 50845 +13
- Partials 2946 2952 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/install/assets/crds/assets.go (1)
169-173: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject unknown
--capi-storage-versionvalues before applying overrides. Incmd/install/assets/crds/assets.go:169-173, an unexpected version string makes every entry incrd.Spec.Versionsnon-storage and produces an invalid CRD. Validate the input up front so the install fails fast with a clear error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/install/assets/crds/assets.go` around lines 169 - 173, Validate the `--capi-storage-version` override before `loadCRDs` applies it in `assets.go` so unknown values are rejected early instead of marking every entry in `crd.Spec.Versions` as non-storage. Update the override handling around `overrides[crd.Name]` and `crd.Spec.Versions` to check that `override.StorageVersion` matches one of the version names for that CRD, and return a clear error from the install flow if it does not.
🧹 Nitpick comments (1)
cmd/install/assets/crds/assets_test.go (1)
45-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting exactly one storage version per CRD.
The current switch only checks the two known version names, so it would not detect the failure mode where the selected version name matches none of a CRD's versions and every version ends up
Storage=false. Counting storage versions and asserting== 1per CAPI CRD would harden this test against the risk flagged inassets.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/install/assets/crds/assets_test.go` around lines 45 - 99, The storage-version checks in TestCustomResourceDefinitionsStorageVersion only verify the known version names, so they can miss cases where no version is marked as storage. Update both subtests to count the number of true Storage entries for each CAPI CRD produced by CustomResourceDefinitions and assert that exactly one version is storage, while still checking that the selected version name (v1beta1 or v1beta2) is the one marked true and the other known version is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/install/assets/crds/assets_test.go`:
- Around line 12-98: Update the new subtest labels in assets_test.go to match
the repository’s test naming convention used by TestCapiResources: rename each
t.Run description in CAPICRDOverridesWithStorageVersion and
CustomResourceDefinitionsStorageVersion to the “When … it should …” format. Keep
the test logic the same, but make the names describe the input version and
expected storage behavior in that phrasing.
In `@cmd/install/install_test.go`:
- Around line 2442-2535: Update the table-driven subtest names in the install
tests to match the required “When … it should …” convention used by this suite.
Reword each case in the test block around the existing storage-version scenarios
(e.g. the fresh install, upgrade, re-install, and downgrade cases in the test
using makeCAPICRD and Options) so every name starts with “When” and describes
the condition plus expected outcome with “it should …”.
In `@cmd/install/install.go`:
- Around line 443-445: Validate the `--capi-storage-version` value before CRD
rendering so only `v1beta1` or `v1beta2` are accepted. Add an allow-list check
in `Options.Validate` or at the start of `setupCRDs`, using the existing
`opts.CAPIStorageVersion` path, and reject any other value with a clear error
before continuing into CRD setup.
In `@support/capi-crdmigrator/crd_migrator.go`:
- Around line 497-529: The ttlCache implementation only evicts expired keys
during has(), so stale CR/generation entries can accumulate in memory. Update
ttlCache.add to sweep and remove expired entries before inserting the new key,
using the existing ttlCache fields and mutex so the map stays bounded even when
old keys are never read again.
In `@test/e2e/capi_storage_migration_test.go`:
- Line 51: The test setup has leftover unused scaffolding: hcpNamespace is
computed in the CAPI storage migration test but only discarded later, and the
redundant _ = g no-op is unnecessary. Remove the unused hcpNamespace assignment
and the _ = g discard, or fully wire hcpNamespace into the intended CAPI
resource checks in the test body if that was the original goal; reference the
existing test function and its g.Expect(t.Run(...)) usage to update the right
spots consistently.
- Around line 140-174: The log stream in checkHOLogsForMigratorErrors is
deferred inside the pod loop, so each PodLogs stream stays open until the
function exits instead of being closed after each pod is processed. Update the
loop to close each stream immediately after scanning that pod’s logs, and keep
the existing stream creation and log-scanning logic in
checkHOLogsForMigratorErrors unchanged otherwise.
---
Outside diff comments:
In `@cmd/install/assets/crds/assets.go`:
- Around line 169-173: Validate the `--capi-storage-version` override before
`loadCRDs` applies it in `assets.go` so unknown values are rejected early
instead of marking every entry in `crd.Spec.Versions` as non-storage. Update the
override handling around `overrides[crd.Name]` and `crd.Spec.Versions` to check
that `override.StorageVersion` matches one of the version names for that CRD,
and return a clear error from the install flow if it does not.
---
Nitpick comments:
In `@cmd/install/assets/crds/assets_test.go`:
- Around line 45-99: The storage-version checks in
TestCustomResourceDefinitionsStorageVersion only verify the known version names,
so they can miss cases where no version is marked as storage. Update both
subtests to count the number of true Storage entries for each CAPI CRD produced
by CustomResourceDefinitions and assert that exactly one version is storage,
while still checking that the selected version name (v1beta1 or v1beta2) is the
one marked true and the other known version is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8bd87e84-55f5-4d56-aa66-25868c88e041
⛔ Files ignored due to path filters (1)
hypershift-operator/controllers/hostedcluster/testdata/cluster-api/zz_fixture_TestReconcileComponents.yamlis excluded by!**/testdata/**
📒 Files selected for processing (17)
cmd/install/assets/crds/assets.gocmd/install/assets/crds/assets_test.gocmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment_test.godocs/content/how-to/capi-storage-migration.mddocs/mkdocs.ymlhypershift-operator/main.gosupport/capi-crdmigrator/crd_migrator.gosupport/capi-crdmigrator/crd_migrator_test.gotest/e2e/capi_storage_migration_test.gotest/e2e/e2e_test.gotest/e2e/util/install.gotest/e2e/util/options.go
4b6cf33 to
19841ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/how-to/capi-storage-migration.md`:
- Around line 69-73: Add a language tag to the fenced Markdown snippets in the
migration doc so markdownlint treats them as text blocks. Update the plain
fenced blocks in the affected sections of the document to use the same syntax as
the other output/error snippets, keeping the content unchanged and applying the
fix in both locations referenced by the comment.
- Line 83: Fix the typo in the documentation sentence so the CLI flag name and
the following word are separated correctly; update the text in the migration
guide sentence that mentions --capi-storage-version and
--acknowledge-capi-migration so it reads with a space between the flag value and
the word flag, preserving the rest of the wording unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 296b59f6-396c-483e-9bdc-fae567985130
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (2)
docs/content/how-to/capi-storage-migration.mddocs/mkdocs.yml
✅ Files skipped from review due to trivial changes (1)
- docs/mkdocs.yml
a6838eb to
d72bb04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/install/install_test.go (1)
2476-2577: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for mixed
storedVersions.The table only covers singleton
storedVersions. During CRD storage migration, Kubernetes can report bothv1beta1andv1beta2; add a case for that intermediate state with the intended guard behavior. As per coding guidelines,**/*_test.go: Unit test any code changes and additions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/install/install_test.go` around lines 2476 - 2577, Add a test case in the install test table to cover a CRD whose storedVersions contains both v1beta1 and v1beta2, since the current scenarios only use a single version. Update the existing table-driven test around the install/migration checks to build the mixed-state object with makeCAPICRD and assert the intended guard behavior for the relevant path in the install flow, using the same Options fields such as CAPIStorageVersion, AcknowledgeCAPIMigration, and ForceCAPIStorageDowngrade as needed.Source: Coding guidelines
cmd/install/assets/crds/assets.go (1)
51-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReturn a defensive copy from
CAPICRDNames.
CAPICRDNamesexposes the backing array forcapiCRDNames; any caller that sorts, appends, or edits the returned slice can change global CRD override behavior for the process.Proposed fix
func CAPICRDNames() []string { - return capiCRDNames + names := make([]string, len(capiCRDNames)) + copy(names, capiCRDNames) + return names }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/install/assets/crds/assets.go` around lines 51 - 54, CAPICRDNames is returning the shared capiCRDNames slice directly, which lets callers mutate global state. Update CAPICRDNames to return a defensive copy of capiCRDNames instead of the backing slice, so any sorting, appending, or editing by callers cannot affect process-wide CRD behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/install/assets/crds/assets.go`:
- Around line 62-67: CAPICRDOverridesWithStorageVersion currently allows any
non-empty storageVersion to flow through, which can later mark every CRD version
as non-storage and produce an invalid CRD; make the handling explicit by
validating the requested version against the available CRD versions before
building overrides and fail fast when no match is found. Update the CRD
construction path around CAPICRDOverridesWithStorageVersion and the
storage-version selection logic that sets the version Storage field so it tracks
whether a match was found and returns an error instead of implicitly falling
back to all false, keeping the exported API behavior explicit.
---
Nitpick comments:
In `@cmd/install/assets/crds/assets.go`:
- Around line 51-54: CAPICRDNames is returning the shared capiCRDNames slice
directly, which lets callers mutate global state. Update CAPICRDNames to return
a defensive copy of capiCRDNames instead of the backing slice, so any sorting,
appending, or editing by callers cannot affect process-wide CRD behavior.
In `@cmd/install/install_test.go`:
- Around line 2476-2577: Add a test case in the install test table to cover a
CRD whose storedVersions contains both v1beta1 and v1beta2, since the current
scenarios only use a single version. Update the existing table-driven test
around the install/migration checks to build the mixed-state object with
makeCAPICRD and assert the intended guard behavior for the relevant path in the
install flow, using the same Options fields such as CAPIStorageVersion,
AcknowledgeCAPIMigration, and ForceCAPIStorageDowngrade as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d05b1d1b-4f73-4acd-adf3-1b233a6bdf33
⛔ Files ignored due to path filters (2)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mdhypershift-operator/controllers/hostedcluster/testdata/cluster-api/zz_fixture_TestReconcileComponents.yamlis excluded by!**/testdata/**
📒 Files selected for processing (17)
cmd/install/assets/crds/assets.gocmd/install/assets/crds/assets_test.gocmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment_test.godocs/content/how-to/capi-storage-migration.mddocs/mkdocs.ymlhypershift-operator/main.gosupport/capi-crdmigrator/crd_migrator.gosupport/capi-crdmigrator/crd_migrator_test.gotest/e2e/capi_storage_migration_test.gotest/e2e/e2e_test.gotest/e2e/util/install.gotest/e2e/util/options.go
✅ Files skipped from review due to trivial changes (4)
- test/e2e/util/install.go
- test/e2e/util/options.go
- docs/content/how-to/capi-storage-migration.md
- docs/mkdocs.yml
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/install/assets/hypershift_operator_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment_test.go
- cmd/install/assets/hypershift_operator.go
- hypershift-operator/main.go
- cmd/install/assets/crds/assets_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/capi_manager/deployment.go
- cmd/install/install.go
- test/e2e/e2e_test.go
- test/e2e/capi_storage_migration_test.go
- support/capi-crdmigrator/crd_migrator_test.go
- support/capi-crdmigrator/crd_migrator.go
|
/test e2e-aws |
Test Resultse2e-aws
|
openshift-ci
Bot
added
the
needs-rebase
125c9bf to
edcd70d
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test e2e-aws |
2507829 to
9c59bda
Compare
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
Now I have a clear picture. Let me summarize the 4 distinct root causes:
All of these failures are infrastructure/environment issues (API server 503s, AWS keypair quota exhaustion, DaemonSet scheduling issues) — none are related to the CAPI v1beta2 storage migration code changes. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 17 test failures (4 distinct root failures with cascading parent failures) are caused by CI infrastructure/environment issues unrelated to the PR's code changes. The management cluster's API server returned HTTP 503 ("the server is currently unable to handle the request") 16 times throughout the run, affecting the EnsureAllRoutesUseHCPRouter check for TestAutoscaling and TestKarpenterUpgradeControlPlane, cluster dump operations during teardown, and namespace cleanup. Separately, the EnsureGlobalPullSecret test hit a DaemonSet scheduling issue (kubelet-config-verifier stuck at 2/3 pods ready for >20 minutes until deadline), and the AWS account hit the 5000 keypair quota limit. The actual test for the PR's feature ( Root CauseThe failures stem from three independent CI infrastructure/environment issues: 1. Management cluster API server instability (HTTP 503): The shared management cluster API server returned "the server is currently unable to handle the request" errors at least 16 times during the test run. This directly caused:
2. DaemonSet scheduling issue (EnsureGlobalPullSecret): The 3. AWS EC2 keypair quota exhaustion: The AWS account hit the 5000 keypair limit: Not related to PR changes: The PR's feature test Recommendations
Evidence
|
9c59bda to
e4918e5
Compare
|
/test e2e-aws |
Add storage version migration to the hypershift install command. The migration runs by default and can be disabled with the --disable-capi-migration flag. The migration is performed by CAPI's CRD migrator controller, setup and executed centrally by the hypershift operator. Additionally migration progress is persisted in a ConfigMap by the migrator controller and metrics for migrated CRDs and total CRDs to migrate are exposed. Signed-off-by: Borja Clemente <bclement@redhat.com> new opt-out approach
Add unit tests for the CAPI v1beta2 storage migration and for all new or changed flags in hypershift install. Add new tests for the adjusted CRD migrator copied from upstream CAPI. Signed-off-by: Borja Clemente <bclement@redhat.com>
Add an e2e test that verifies the CAPI storage version migration from v1beta1 to v1beta2. The test creates a hosted cluster with v1beta1 as storage versiona and then runs `hypershift install` again with the `--capi-storage-migration=v1beta2` flag. Then it verifies that the migrator runs, all CRDs have v1beta2 as storage version and the cluster is still running. This e2e test is gated behind a flag to avoid interferring with other suites and be tested in isolation. Signed-off-by: Borja Clemente <bclement@redhat.com>
Add a new how-to guide for the storage version migration explaining the process, all the new flags and all scenarios this feature interacts with and what the result for each is. Signed-off-by: Borja Clemente <bclement@redhat.com>
Add crd_migrator.go to the codecov ignore list. This is acceptable because it has been adopted from upstream CAPI and is tested via envtest as well. Signed-off-by: Borja Clemente <bclement@redhat.com>
e4918e5 to
613eea0
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@clebs: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
openshift-ci-robot
added
the
verified
|
@clebs: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@clebs: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified remove The previous tag was intended for a different PR. |
openshift-ci-robot
removed
the
verified
|
@clebs: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
3 participants
What this PR does / why we need it:
This PR enables CAPI CRD storage version migration from v1beta1 to v1beta2. The migration is necessary because upstream Cluster API is deprecating v1beta1, and all existing CAPI resources stored in etcd must be re-stored using the v1beta2 schema before v1beta1 can be removed.
Additionally, keeping the old storage version also has shown to block the ability to migrate clients to the new API because of various conversion issues.
Upstream, the supported path is migrating the storage version first while clients stay on the old API and afterwards migrating the clients. Therefore, we will follow the same procedure.
Safety guards require explicit acknowledgement before migration and prevent accidental downgrades after migration completes.
When a user runs
hypershift install, the CLI applies all CAPI CRDs with v1beta2 as the storage version, and the hypershift operator starts a CRD migrator controller that performs no-op server-side applies on every existing CAPI resource to force re storage. CAPI's built-in migrator is disabled on the CAPI manager deployment to avoid conflicts and managing the process centrally on the hypershift operator, gated by the flags and guardrails described.The migration can be disabled by passing the
--disable-capi-migrationflag.Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3604
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
--disable-capi-migrationinstallation flag and optionalCAPI_STORAGE_VERSIONwiring for the migrator.