OCPBUGS-96908: remove redhat-marketplace catalog from HyperShift - #8958
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-96908, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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-robot
added
the
jira/valid-bug
|
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 removes Red Hat Marketplace catalog references from the hosted cluster API, OLM catalog wiring, deployment 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-96908, which is valid. 3 validation(s) were run on this bug
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. |
57473c1 to
54cfdc5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8958 +/- ##
=======================================
Coverage 45.03% 45.04%
=======================================
Files 780 780
Lines 97623 97627 +4
=======================================
+ Hits 43969 43974 +5
+ Misses 50626 50622 -4
- Partials 3028 3031 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
7e32689 to
ecd4c4f
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
area/documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/content/how-to/disconnected/idms-icsp-for-management-clusters.md (1)
141-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer “will not be created” here.
is not going to be createdis wordier than needed in this doc sentence.Suggested wording
- In this case the imageStream is not going to be created, and it will be up to the guest cluster owner updating the value of the annotations when the internal mirror will get refreshed to pull in operator updates. + In this case the imageStream will not be created, and it will be up to the guest cluster owner to update the annotation values when the internal mirror is refreshed to pull in operator updates.🤖 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 `@docs/content/how-to/disconnected/idms-icsp-for-management-clusters.md` around lines 141 - 143, The HostedCluster documentation sentence uses overly wordy phrasing for the imagestream behavior. In the paragraph describing the OLM catalog override annotations, update the wording around the imageStream creation in the HostedCluster CR explanation to use “will not be created” instead of “is not going to be created,” while keeping the meaning unchanged and preserving the references to the three hypershift.openshift.io/*-operators-catalog-image annotations.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/content/how-to/disconnected/idms-icsp-for-management-clusters.md`:
- Around line 141-143: The HostedCluster documentation sentence uses overly
wordy phrasing for the imagestream behavior. In the paragraph describing the OLM
catalog override annotations, update the wording around the imageStream creation
in the HostedCluster CR explanation to use “will not be created” instead of “is
not going to be created,” while keeping the meaning unchanged and preserving the
references to the three hypershift.openshift.io/*-operators-catalog-image
annotations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4736149a-4f3c-41b8-9508-96f055ea354d
⛔ 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 (4)
docs/content/how-to/agent/create-agent-cluster.mddocs/content/how-to/disconnected/idms-icsp-for-management-clusters.mddocs/content/how-to/none/create-none-cluster.mddocs/content/how-to/restart-control-plane-components.md
💤 Files with no reviewable changes (3)
- docs/content/how-to/restart-control-plane-components.md
- docs/content/how-to/agent/create-agent-cluster.md
- docs/content/how-to/none/create-none-cluster.md
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Review of the redhat-marketplace catalog removal. The CPO-side changes look correct since the CPO is version-specific, but there are concerns around the HO (multi-version), upgrade cleanup, and E2E test compatibility.
| // If used, all of them should be set at the same time referring images only by digest (`...@sha256:<id>`). | ||
| // This will disable the imagestream used to keep the catalog images up to date. | ||
| CertifiedOperatorsCatalogImageAnnotation = "hypershift.openshift.io/certified-operators-catalog-image" | ||
| CommunityOperatorsCatalogImageAnnotation = "hypershift.openshift.io/community-operators-catalog-image" |
There was a problem hiding this comment.
We should not remove the annotation because the HyperShift operator can work with hosted clusters of older OCP versions that do still have this catalog (catalog was removed in 4.22). Also the validation that requires all override annotations should be relaxed, since for newer OCP (4.22+), only the remaining 3 catalogs need to be overridden.
| {manifest: manifests.CertifiedOperatorsCatalogSource, reconcile: olm.ReconcileCertifiedOperatorsCatalogSource}, | ||
| {manifest: manifests.CommunityOperatorsCatalogSource, reconcile: olm.ReconcileCommunityOperatorsCatalogSource}, | ||
| {manifest: manifests.RedHatMarketplaceCatalogSource, reconcile: olm.ReconcileRedHatMarketplaceCatalogSource}, | ||
| {manifest: manifests.RedHatOperatorsCatalogSource, reconcile: olm.ReconcileRedHatOperatorsCatalogSource}, |
There was a problem hiding this comment.
The old redhat-marketplace CatalogSource in openshift-marketplace on the guest cluster needs explicit deletion during upgrade. The reconcile loop only manages catalogs it knows about — removing the entry here leaves the old CatalogSource orphaned.
| hyperv1.TopologyAnnotation, | ||
| hyperv1.DisableMachineManagement, | ||
| hyperv1.CertifiedOperatorsCatalogImageAnnotation, | ||
| hyperv1.CommunityOperatorsCatalogImageAnnotation, |
There was a problem hiding this comment.
The management-plane Deployment, Service, and ControlPlaneComponent CR for redhat-marketplace-catalog need explicit cleanup on upgrade, similar to cleanupOldKonnectivityServerDeployment and friends.
| "olm.catalogSource": "redhat-marketplace", | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
The E2E test infrastructure runs against all supported OCP versions, not just 4.22+. Unconditionally removing redhat-marketplace-catalog from the workload registry means tests against older OCP versions (where the marketplace catalog still exists) will either fail or silently skip checking that workload. This removal needs to be conditioned on the OCP version.
7a3bbc9 to
7c6b543
Compare
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe PR removes the Root CauseThe test
The PR adds this cleanup code to // Cleanup: delete the deprecated redhat-marketplace CatalogSource
deprecatedMarketplaceCatalog := &operatorsv1alpha1.CatalogSource{
ObjectMeta: metav1.ObjectMeta{
Name: "redhat-marketplace",
Namespace: "openshift-marketplace",
},
}
if _, err := k8sutil.DeleteIfNeeded(ctx, r.client, deprecatedMarketplaceCatalog); err != nil {
errs = append(errs, ...)
}
This breaks the invariant: RecommendationsAdd the deprecated
Add to // Exempt the deprecated redhat-marketplace CatalogSource from random Get errors.
// DeleteIfNeeded does a Get without a corresponding Create.
&operatorsv1alpha1.CatalogSource{
ObjectMeta: metav1.ObjectMeta{
Name: "redhat-marketplace",
Namespace: "openshift-marketplace",
},
},This ensures the cleanup Get is never randomly errored, preserving the test's 1:1 Get-to-Create invariant. No other test changes are needed. Evidence
|
openshift-ci
Bot
added
needs-rebase
53971e8 to
7478c29
Compare
openshift-ci
Bot
removed
the
needs-rebase
…HyperShift The redhat-marketplace catalog is no longer needed. This commit removes all references to the redhat-marketplace OLM catalog across the codebase: - Remove RedHatMarketplaceCatalogImageAnnotation API constant - Delete v2 component assets (deployment.yaml, service.yaml) - Remove from CPOv2 catalogs component registration - Remove from HCCO guest-side CatalogSource reconciliation - Remove from NO_PROXY lists in catalog-operator, olm-operator, packageserver - Update catalog image override validation (3 default catalogs instead of 4) - Remove from E2E test workload registry and crash tolerations - Delete test fixtures and regenerate imagestream golden files - Update vendor copy
…ation Remove redhat-marketplace-catalog from example pod listings, component lists, and annotation references across HyperShift documentation.
7478c29 to
1fa8063
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e-aks |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/verified by @vismishr Verified with CPO image quay.io/dgautam/remove-redhat-marketplace-catalog:v1. |
openshift-ci-robot
added
the
verified
|
@vismishr: 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. |
|
/test security |
|
@dhgautam99: all tests passed! 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. |
|
@dhgautam99: Jira Issue Verification Checks: Jira Issue OCPBUGS-96908 Jira Issue OCPBUGS-96908 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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: #8958 failed to apply on top of branch "release-4.22": 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 kubernetes-sigs/prow repository. |
|
/jira backport release-4.22 |
|
@dhgautam99: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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-robot: #8958 failed to apply on top of branch "release-4.22": 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 kubernetes-sigs/prow repository. |
10 participants
What this PR does / why we need it:
Permanently removes the
redhat-marketplaceOLM catalog from the HyperShift codebase. This includes:RedHatMarketplaceCatalogImageAnnotationAPI constant and its propagationWhich issue(s) this PR fixes:
Fixes OCPBUGS-96908
Special notes for your reviewer:
Documentation updates (docs/) are not included in this PR and will be handled separately.
Checklist:
Summary by CodeRabbit
NO_PROXYhandling to exempt only the remaining supported operator catalogs.