CNTRLPLANE-2939: Coordinate CRD lifecycle with Cluster CAPI Operator#7996
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request references CNTRLPLANE-2939 which is a valid jira issue. 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. |
|
/test e2e-aws-minimal |
|
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:
📝 WalkthroughWalkthroughThe installer now performs server-side dry-run validation of CRDs before applying manifests. It uses discovery to detect whether the ClusterAPI (cluster.x-k8s.io) API is served on the management cluster. If ClusterAPI is present, the installer creates or updates the ClusterAPI configuration object to mark HyperShift CRDs as unmanaged. New unexported helpers implement discovery queries, CRD dry-run validation, and ensuring the singleton ClusterAPI config. The change also updates an OpenShift API dependency and removes Sequence DiagramsequenceDiagram
participant Installer
participant APIServer as Management Cluster API
participant Discovery
participant CAPIConfig as ClusterAPI Config
Installer->>APIServer: Dry-run validate CRDs (server-side patch)
APIServer-->>Installer: Validation results
Installer->>Discovery: Query API resources for cluster.x-k8s.io
Discovery->>APIServer: GET /apis/cluster.x-k8s.io
APIServer-->>Discovery: APIResourceList or NotFound
Discovery-->>Installer: ClusterAPI present? (yes/no)
alt ClusterAPI registered
Installer->>CAPIConfig: Get existing ClusterAPI config
CAPIConfig-->>Installer: Config (or not found)
Installer->>Installer: Merge HyperShift CRDs into unmanaged list
Installer->>CAPIConfig: Create/Update config with unmanaged CRDs
CAPIConfig-->>Installer: Config updated
end
Installer->>APIServer: Apply CRDs and manifests
APIServer-->>Installer: Apply result
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@muraee: This pull request references CNTRLPLANE-2939 which is a valid jira issue. 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. |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/install/install.go`:
- Around line 961-999: The Get/Create/Update sequence in ensureUnmanagedCRDs can
race with concurrent installers; wrap the whole read-modify-write block in a
retry.RetryOnConflict loop so it will re-fetch and retry on resource version
conflicts. Inside the loop, call client.Get to load ClusterAPI, on
apierrors.IsNotFound create the singleton and if client.Create returns
apierrors.IsAlreadyExists convert that to a conflict error (so RetryOnConflict
will re-get and continue); otherwise on success return. For the update path,
recompute merged UnmanagedCustomResourceDefinitions, set clusterAPI.Spec if nil,
and call client.Update; let RetryOnConflict retry on conflicts until the update
succeeds or a non-retryable error is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ec2b7873-5c3e-4bd5-b920-00ebf03314e0
⛔ Files ignored due to path filters (100)
api/go.sumis excluded by!**/*.sumapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**api/vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.goapi/vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**api/vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**api/vendor/modules.txtis excluded by!**/vendor/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlgo.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/api/apps/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.govendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types_console_sample.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.govendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.govendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.govendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated.swagger_doc_generated.govendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/go.modapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlcmd/install/install.gocmd/install/install_test.gogo.mod
💤 Files with no reviewable changes (1)
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yaml
|
/test verify |
muraee
force-pushed
the
capi-compatibility
branch
from
b0e0214 to
d3c739c
Compare
|
@muraee: This pull request references CNTRLPLANE-2939 which is a valid jira issue. 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. |
|
we'll want to test this via #8016 as well |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
I think eventually we will have upgrade checks in place so that the webhooks will warn about the future version being incompatible, but otherwise, PR description is accurate |
|
/lgtm
I think this has made us realise we need to adjust the way we are setting up CompatibilityRequirements, but that's fine, I'll make a note and we can make this concept work and be safe so that your initial dry runs are effective |
|
/retest-required |
|
@muraee can we rebase? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7996 +/- ##
==========================================
+ Coverage 37.22% 37.24% +0.01%
==========================================
Files 750 750
Lines 91789 91905 +116
==========================================
+ Hits 34168 34227 +59
- Misses 54981 55035 +54
- Partials 2640 2643 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I did a round of tests and attached the test report to the Jira comments. basically what I did were:
I did not test the hosted cluster creation yet after the hypershift operator installation succeeded (mgmt cluster expired ...). If the pr needs the verified tag today, I can tag :) , or I can do the hosted cluster creation next Tuesday. (PTO tomorrow and holidays) |
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
This is mostly there, thanks. Once you're also waiting for the config change to have been actioned it'll be there.
From our side, in addition to the missing status field I mentioned, note that we also haven't actually integrated CRD Compatibility into the installer yet. We're hopefully very close, but it won't current do anything. Until we do, even after you've marked the CRDs unmanaged we're still going to be trying to manage them. This is going to result in a controller fight.
muraee
force-pushed
the
capi-compatibility
branch
from
aee5318 to
cb85728
Compare
muraee
force-pushed
the
capi-compatibility
branch
2 times, most recently
from
45e4666 to
8e7f844
Compare
| capiCRDNames := set.New[string]() | ||
| for _, crd := range capiCRDs { | ||
| name := crd.GetName() | ||
| if strings.HasSuffix(name, ".cluster.x-k8s.io") { |
There was a problem hiding this comment.
I suspect this suffix selection will be insufficient. I'm thinking specifically of ASO on Azure (not currently deployed by CAPIO, but probably will be eventually) and ORC on OpenStack (currently deployed by CAPIO), but there could be others.
Something which can be addressed when it comes up, doesn't have to be a blocker today.
There was a problem hiding this comment.
Agreed, this will likely need expanding for ASO/ORC and others. I've left it as-is for now since those aren't deployed by CAPIO yet — we can extend the filter when the need arises.
| if err := hyperapi.YamlSerializer.Encode(clusterAPI, &buf); err != nil { | ||
| return fmt.Errorf("failed to encode ClusterAPI config: %w", err) | ||
| } | ||
| if err := client.Patch(ctx, clusterAPI, crclient.RawPatch(types.ApplyPatchType, buf.Bytes()), |
There was a problem hiding this comment.
FYI this is not safe in general due to the vagiaries of serialisation in Go. Suggest you use an ApplyConfiguration instead which is specifically designed for this. You'll find them in openshift/client-go.
There was a problem hiding this comment.
Good call. ApplyConfiguration types for operator/v1alpha1 don't exist in openshift/client-go yet, so I've switched to constructing the SSA patch payload as a map[string]any with encoding/json.Marshal. This ensures only the fields we intend to own are included in the patch, avoiding zero-value serialization issues.
| if clusterAPI.Status.ObservedRevisionGeneration < clusterAPI.Generation { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This is not correct, yet: you're asserting that the ClusterAPI object you've read here was up to date with itself, but you're not asserting that it's up to date with the one you just wrote. i.e. The first time this executes you could (and if you're using an informer almost definitely will, if you're not using an informer still might) read an old version of ClusterAPI before you updated it. This old ClusterAPI will almost certainly pass both of these checks, but it's still out of date.
There was a problem hiding this comment.
Fixed. ensureUnmanagedCRDs now returns the metadata.generation from the SSA patch response, and waitForCAPIOperatorSync compares status.observedRevisionGeneration against that known generation rather than against the polled object's own generation. Added a test case for the stale-read scenario.
| crclient.ForceOwnership, crclient.FieldOwner("hypershift"), | ||
| ); err != nil { | ||
| return fmt.Errorf("failed to apply ClusterAPI config: %w", err) | ||
| } |
There was a problem hiding this comment.
At this point your clusterAPI object has been updated to be current with whatever was written in the PATCH. The metadata.generation from this object is the one you need to compare against in waitForCAPIOperatorSync.
There was a problem hiding this comment.
Done — ensureUnmanagedCRDs now returns clusterAPI.Generation from the patch response object and the caller passes it through to waitForCAPIOperatorSync.
|
I have all the evidence needed. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
For each workflow file, if the file's content on Timeline:
PR #7996 only modifies Recommendations
Evidence
|
When the ClusterAPI CRD (clusterapis.operator.openshift.io) is registered on the management cluster, the install command now: 1. Detects CCAPIO presence by checking for the ClusterAPI CRD 2. Gets-or-creates the ClusterAPI config singleton and populates unmanagedCustomResourceDefinitions with HyperShift's CAPI CRD names (those with groups ending in .cluster.x-k8s.io) 3. Validates all CRDs via server-side dry-run before applying, regardless of CCAPIO presence, to catch webhook rejections and schema conflicts early This prevents conflicts between HyperShift and CCAPIO when both attempt to manage the same CAPI CRDs on the management cluster. Ref: CNTRLPLANE-2939 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use JSON map for SSA patch payload instead of YamlSerializer to avoid zero-value serialization issues (ApplyConfiguration types for operator/v1alpha1 are not yet available in openshift/client-go) - Return metadata.generation from ensureUnmanagedCRDs and pass it to waitForCAPIOperatorSync to prevent false positives from stale reads - Add test case for stale object scenario Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@muraee: 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. |
|
@muraee: 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. |
Fixes: https://redhat.atlassian.net/browse/OCPBUGS-86791 The HyperShift install code (openshift/hypershift#7996) added CAPI coordination logic that patches clusterapis.operator.openshift.io/cluster to register unmanaged CRDs with the Cluster CAPI Operator. The addon agent ClusterRole template was never updated to grant these permissions, causing the install job to fail with a forbidden error when the ClusterAPIInstall FeatureGate is enabled. Add get/list/watch/patch on operator.openshift.io/clusterapis to the addon agent ClusterRole template, and add a unit test to prevent regression. Signed-off-by: Chirag Deshpande <chdeshpa@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…#711) Fixes: https://redhat.atlassian.net/browse/OCPBUGS-86791 The HyperShift install code (openshift/hypershift#7996) added CAPI coordination logic that patches clusterapis.operator.openshift.io/cluster to register unmanaged CRDs with the Cluster CAPI Operator. The addon agent ClusterRole template was never updated to grant these permissions, causing the install job to fail with a forbidden error when the ClusterAPIInstall FeatureGate is enabled. Add get/list/watch/patch on operator.openshift.io/clusterapis to the addon agent ClusterRole template, and add a unit test to prevent regression. Signed-off-by: Chirag Deshpande <chdeshpa@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
8 participants
What this PR does / why we need it:
When a management cluster has the Cluster CAPI Operator (CCAPIO) installed, HyperShift's hypershift install command installs CAPI CRDs that may conflict with CRDs already managed by CCAPIO. This PR adds coordination logic to the install flow:
Dry-run validation: All CRDs are validated via server-side dry-run before applying, catching webhook rejections and schema conflicts early — regardless of CCAPIO presence
CCAPIO detection: Uses the discovery API to check if the ClusterAPI kind is served under operator.openshift.io/v1alpha1
CRD ownership signaling: Gets-or-creates the ClusterAPI config singleton and populates spec.unmanagedCustomResourceDefinitions with HyperShift's CAPI CRD names (groups ending in .cluster.x-k8s.io), telling CCAPIO to skip these CRDs
The coordination only mutates the ClusterAPI config after dry-run passes, so there are no side effects if CRDs cannot be applied.
Install flow (after this PR):
Vendor bump
Bumps github.com/openshift/api from v0.0.0-20260120150926-4c643a652d54 to v0.0.0-20260227165130-5a7add616a90 to vendor the ClusterAPI config type (operator.openshift.io/v1alpha1). The selected commit remains compatible with k8s.io v0.34.x.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-2939
Special notes for your reviewer:
The unmanagedCustomResourceDefinitions field is append-only (CEL enforced) — once a CRD name is added, it cannot be removed
In the pre-upgrade scenario (4.N-1), CCAPIO webhooks are not running so dry-run passes trivially — this is expected and correct
Helm chart rendering (hyperShiftOperatorManifests with nil client) is unaffected — CCAPIO coordination only runs in InstallHyperShiftOperator
Summary by CodeRabbit
New Features
Chores
Tests