-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-44438: Remove non-matching feature-gated CVO manifests from payload #5093
OCPBUGS-44438: Remove non-matching feature-gated CVO manifests from payload #5093
Conversation
/retest |
/test e2e-aks e2e-kubevirt-aws-ovn-reduced |
@petr-muller: This pull request references Jira Issue OCPBUGS-44438, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
/hold Holding until branch cut |
afe37bc
to
2c5ab36
Compare
/hold cancel |
2c5ab36
to
b04fab1
Compare
I have adopted Seth's approach from #5096. It is closer to desired behavior than my original proposal, I just tweaked it so it removes all non-matching manifests, instead of relying on a This means that CVO will need to always provide a manifest for a given featureset if a certain resource needs to be applied, it cannot rely on |
/jira refresh |
@petr-muller: This pull request references Jira Issue OCPBUGS-44438, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
please also add those changes here: hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go Line 124 in da24e17
|
f3a038d
to
895e5be
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest-required |
/retest-required |
CVO manifests contain some feature-gated ones: - since at least 4.16, there are feature-gated `ClusterVersion` CRDs - `UpdateStatus` feature is delivered through DevPreview (now) and TechPreview (later) feature set We observed HyperShift CI jobs to fail when adding DevPreview-gated deployment manifests, which was unexpected. Investigating further, we discovered that HyperShift applies these manifests: [cluster-version-operator-665c5789d5-8sr59-bootstrap.log](https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1091/pull-ci-openshift-cluster-version-operator-master-e2e-hypershift-conformance/1853764751801192448/artifacts/e2e-hypershift-conformance/dump/artifacts/namespaces/clusters-c01d0e18fc19f1e0757b/core/pods/logs/cluster-version-operator-665c5789d5-8sr59-bootstrap.log): ``` error: error parsing /var/payload/manifests/0000_00_update-status-controller_03_deployment-DevPreviewNoUpgrade.yaml: error converting YAML to JSON: yaml: invalid map key: map[interface {}]interface {}{".ReleaseImage":interface {}(nil)} ``` But even without these added manifests, this happens for existing `ClusterVersion` CRD manifests present in the payload: ```console ls -1 manifests/*clusterversions*crd.yaml manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yaml manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yaml manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yaml ``` In a passing HyperShift CI job, the [same log](https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1105/pull-ci-openshift-cluster-version-operator-master-e2e-hypershift-conformance/1854708481941049344/artifacts/e2e-hypershift-conformance/dump/artifacts/namespaces/clusters-a5d1b5c3fcb2445935f2/core/pods/logs/cluster-version-operator-96cdfbf7c-cxt9r-bootstrap.log) shows that all four manifests are applied instead of just one: ``` customresourcedefinition.apiextensions.k8s.io/clusterversions.config.openshift.io created customresourcedefinition.apiextensions.k8s.io/clusterversions.config.openshift.io configured customresourcedefinition.apiextensions.k8s.io/clusterversions.config.openshift.io configured customresourcedefinition.apiextensions.k8s.io/clusterversions.config.openshift.io configured ``` This likely means that HyperShift hosted clusters end up using TechPreviewNoUpgrade `ClusterVersion` CRD? Fix the problem by deleting all manifests whose filename indicates that it is a part of a featureset not matching the desired one. Co-authored-by: Seth Jennings <sjenning@redhat.com> Co-authored-by: Petr Muller <muller@redhat.com>
The fixtures are exact copies of the original, default featureset ones. This is to allow adding the new test separately so it can update the default fixtures and produce a clear difference from the defaults.
Fixtures updated with: ``` $ UPDATE=true go test ./control-plane-operator/controllers/hostedcontrolplane/... ./control-plane-operator/controllers/hostedcontrolplane/... ```
d075673
to
8374ac1
Compare
Had to rebase to pull fixture changes from https://github.com/openshift/hypershift/pull/5160/files#diff-33e09167c08ede07364a11e60abb341ce6e4141143fb1a7033c8af966f2c32aa to the TechPreview ones |
/retest-required |
@petr-muller: The
The following commands are available to trigger optional jobs:
Use
In 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 kubernetes-sigs/prow repository. |
/retest-required |
1 similar comment
/retest-required |
/test all |
/test e2e-aws-upgrade-hypershift-operator Failed just on destroy 🤞 |
/lgtm |
/retest |
LOL I was not expecting Konflux to piggyback on Prow commands |
/retest-required |
@petr-muller: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
c5e01a9
into
openshift:main
@petr-muller: Jira Issue OCPBUGS-44438: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-44438 has not been moved to the MODIFIED state. In 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. |
/jira refresh |
@petr-muller: Jira Issue OCPBUGS-44438: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-44438 has been moved to the MODIFIED state. In 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. |
[ART PR BUILD NOTIFIER] Distgit: hypershift |
What this PR does / why we need it:
CVO manifests contain some feature-gated ones:
ClusterVersion
CRDsUpdateStatus
feature is delivered through DevPreview (now) and TechPreview (later) feature setWe observed HyperShift CI jobs to fail when adding DevPreview-gated deployment manifests, which was unexpected. Investigating further, we discovered that HyperShift applies these manifests:
cluster-version-operator-665c5789d5-8sr59-bootstrap.log:
But even without these added manifests, this happens for existing
ClusterVersion
CRD manifests present in the payload:In a passing HyperShift CI job, the same log shows that all four manifests are applied instead of just one:
This likely means that HyperShift hosted clusters end up using TechPreviewNoUpgrade
ClusterVersion
CRD?The proper fix is probably to wire through the
FeatureGate
with desired featureset throughCVOParams
but we would need a bit of selection logic so for now we can just remove entropy by deleting all feature-gated manifests instead of stumbling at them.Which issue(s) this PR fixes:
OCPBUGS-44438
Checklist