OCPBUGS-54790: Move packageserver PDB from guest cluster to management cluster#8459
OCPBUGS-54790: Move packageserver PDB from guest cluster to management cluster#8459dhgautam99 wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-54790, 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. |
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughA PodDisruptionBudget manifest (packageserver-pdb, minAvailable: 1) was added. The packageserver component now adapts that PDB manifest via AdaptPodDisruptionBudget, and a unit test for NewComponent was added. The CVO deployment code imports policy/v1 and includes packageserver-pdb in resourcesToRemove for IBMCloudPlatform/PowerVSPlatform and the default cleanup list. Sequence Diagram(s)sequenceDiagram
participant Operator as HostedControlPlane Operator
participant Component as packageserver Component
participant CVO as CVO / deployment.go
participant Kube as Kubernetes API
Operator->>Component: NewComponent()
Component->>Component: WithManifestAdapter(pdb.yaml -> AdaptPodDisruptionBudget)
Operator->>CVO: prepare resourcesToRemove
CVO->>CVO: include packageserver-pdb in resourcesToRemove
CVO->>Kube: generate payload (may omit pdb in some payloads)
Operator->>Kube: apply manifests (packageserver-pdb present/ensured)
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-54790, 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8459 +/- ##
==========================================
+ Coverage 40.61% 40.70% +0.08%
==========================================
Files 755 755
Lines 93227 93374 +147
==========================================
+ Hits 37864 38004 +140
+ Misses 52640 52636 -4
- Partials 2723 2734 +11
... and 7 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
46bdd14 to
ffe96be
Compare
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aws |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aws |
| "0000_50_olm_07-collect-profiles.cronjob.yaml", | ||
| "0000_50_olm_08-catalog-operator.deployment.ibm-cloud-managed.yaml", | ||
| "0000_50_olm_08-catalog-operator.deployment.yaml", | ||
| "0000_50_olm_00-packageserver.pdb.yaml", |
There was a problem hiding this comment.
please put a PR against https://github.com/openshift/operator-framework-olm/blob/main/manifests/0000_50_olm_00-packageserver.pdb.yaml to remove this from the CVO payload instead
ffe96be to
86fec73
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component.go (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name should follow "When...it should..." pattern.
As per coding guidelines, unit tests should use the format "When ... it should ..." for test case descriptions.
📝 Suggested fix
-func TestNewComponent(t *testing.T) { +func TestNewComponent_WhenCreatingComponent_ItShouldReturnValidPackageserverComponent(t *testing.T) {As per coding guidelines: "Always use 'When ... it should ...' format for describing test cases when creating unit tests".
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component.go` at line 9, Test descriptions in the OLM package server tests must follow the "When ... it should ..." pattern; locate any test declarations (e.g., t.Run("..."), It("..."), or DescribeTable entries) related to the PackageServer component and rename their string descriptions to the form "When <condition> it should <expected outcome>" (for example change "validates X" to "When X it should validate Y"). Ensure all changed descriptions remain clear and update any related test helpers or snapshots that assert on the test name.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component_test.go (1)
9-9: ⚡ Quick winRename test to follow Gherkin "When... it should..." pattern.
The test name should follow the Gherkin syntax pattern as specified in the coding guidelines. Consider renaming to something like
TestNewComponent_WhenCalled_ItShouldReturnComponentNamedPackageserverto align with project standards.As per coding guidelines, "Always use 'When ... it should ...' format for describing test cases when creating unit tests."
♻️ Proposed fix for test naming
-func TestNewComponent(t *testing.T) { +func TestNewComponent_WhenCalled_ItShouldReturnComponentNamedPackageserver(t *testing.T) { t.Parallel() g := NewWithT(t)🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component_test.go` at line 9, Rename the unit test function TestNewComponent to follow the Gherkin "When... it should..." pattern; update the function name to something like TestNewComponent_WhenCalled_ItShouldReturnComponentNamedPackageserver and adjust any references or test runners accordingly so the test still compiles and executes (look for the TestNewComponent function in component_test.go and rename it consistently).
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go`:
- Line 270: Add the packageserver PodDisruptionBudget manifest to the
manifestsToOmit list so it is never deployed (rather than created then removed);
specifically append the PodDisruptionBudget entry for Name="packageserver-pdb",
Namespace="openshift-operator-lifecycle-manager" (the same object literal used
in the diff: &policyv1.PodDisruptionBudget{ObjectMeta: metav1.ObjectMeta{Name:
"packageserver-pdb", Namespace: "openshift-operator-lifecycle-manager"}}) into
the manifestsToOmit array where other omitted manifests are listed (look for the
manifestsToOmit slice/variable in this file) so the CVO will skip deploying that
PDB on new clusters.
---
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component.go`:
- Line 9: Test descriptions in the OLM package server tests must follow the
"When ... it should ..." pattern; locate any test declarations (e.g.,
t.Run("..."), It("..."), or DescribeTable entries) related to the PackageServer
component and rename their string descriptions to the form "When <condition> it
should <expected outcome>" (for example change "validates X" to "When X it
should validate Y"). Ensure all changed descriptions remain clear and update any
related test helpers or snapshots that assert on the test name.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component_test.go`:
- Line 9: Rename the unit test function TestNewComponent to follow the Gherkin
"When... it should..." pattern; update the function name to something like
TestNewComponent_WhenCalled_ItShouldReturnComponentNamedPackageserver and adjust
any references or test runners accordingly so the test still compiles and
executes (look for the TestNewComponent function in component_test.go and rename
it consistently).
🪄 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: 3c0b9051-33db-4f0b-b4b2-c45a059fff33
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/AROSwift/zz_fixture_TestControlPlaneComponents_packageserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/AROSwift/zz_fixture_TestControlPlaneComponents_packageserver_pdb_poddisruptionbudget.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/GCP/zz_fixture_TestControlPlaneComponents_packageserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/GCP/zz_fixture_TestControlPlaneComponents_packageserver_pdb_poddisruptionbudget.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/IBMCloud/zz_fixture_TestControlPlaneComponents_packageserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/IBMCloud/zz_fixture_TestControlPlaneComponents_packageserver_pdb_poddisruptionbudget.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_packageserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_packageserver_pdb_poddisruptionbudget.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/zz_fixture_TestControlPlaneComponents_packageserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/zz_fixture_TestControlPlaneComponents_packageserver_pdb_poddisruptionbudget.yamlis excluded by!**/testdata/**
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/packageserver/pdb.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/packageserver/pdb.yaml
| switch platformType { | ||
| case hyperv1.IBMCloudPlatform, hyperv1.PowerVSPlatform: | ||
| return []client.Object{ | ||
| &policyv1.PodDisruptionBudget{ObjectMeta: metav1.ObjectMeta{Name: "packageserver-pdb", Namespace: "openshift-operator-lifecycle-manager"}}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for packageserver PDB manifest in the codebase and verify manifestsToOmit contains it.
# Search for packageserver PDB manifest file
echo "=== Searching for packageserver PDB manifest ==="
fd -e yaml -e yml . | rg -i 'packageserver.*pdb|pdb.*packageserver'
# Check if it's referenced in manifestsToOmit
echo -e "\n=== Checking manifestsToOmit array ==="
rg -A 60 'manifestsToOmit.*=.*\[\]string' control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go | rg -i 'packageserver.*pdb'
# Search for the actual manifest name in CVO payload or OLM manifests
echo -e "\n=== Searching for OLM packageserver manifests ==="
rg -l '0000_50_olm.*packageserver' --type yaml
echo -e "\n=== Checking if PDB manifest name follows OLM pattern ==="
rg 'kind:\s*PodDisruptionBudget' --type yaml -A 5 -B 5 | rg -i packageserverRepository: openshift/hypershift
Length of output: 17195
🏁 Script executed:
cat -n control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go | sed -n '140,200p'Repository: openshift/hypershift
Length of output: 3942
🏁 Script executed:
cat -n control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go | sed -n '265,290p'Repository: openshift/hypershift
Length of output: 2747
🏁 Script executed:
# Search for packageserver PDB manifest and check if it exists with a standard OLM naming pattern
cd control-plane-operator/controllers/hostedcontrolplane/v2/assets/packageserver
ls -la
cat pdb.yamlRepository: openshift/hypershift
Length of output: 464
🏁 Script executed:
# Check how packageserver PDB is loaded and deployed in deployment.go
# Look for references to pdb.yaml or how assets are loaded
rg -A 10 -B 5 'packageserver.*pdb|pdb.*packageserver' control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.goRepository: openshift/hypershift
Length of output: 2425
🏁 Script executed:
# Search for how packageserver component is structured and deployed
# Check the packageserver component code to understand asset handling
rg -l 'packageserver' control-plane-operator/controllers/hostedcontrolplane/v2/ --type go | head -5Repository: openshift/hypershift
Length of output: 502
🏁 Script executed:
# Check the packageserver component to see how it deploys assets
cat -n control-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component.go | head -50Repository: openshift/hypershift
Length of output: 1782
🏁 Script executed:
# The component.go shows pdb.yaml is deployed by hypershift packageserver component.
# Now check if there's a packageserver PDB from CVO payload that needs to be omitted.
# Search for OLM manifests with packageserver and pdb pattern
# First, let's check what the CVO payload looks like in test fixtures
rg 'kind: PodDisruptionBudget' control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/ -A 5 -B 5 | grep -A 5 -B 5 packageserverRepository: openshift/hypershift
Length of output: 11022
🏁 Script executed:
# Check the CVO deployment script to see what manifest filename the packageserver PDB has
# Extract more context from one of the CVO deployment test files
grep -n "0000_01_cleanup.yaml" control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yaml | head -1Repository: openshift/hypershift
Length of output: 142
🏁 Script executed:
# Search for the actual packageserver PDB manifest file in the CVO payload
# Check what manifests are actually in the payload vs what's in cleanup
rg 'packageserver.*pdb|0000_.*packageserver.*pdb' control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yaml | head -20Repository: openshift/hypershift
Length of output: 100
🏁 Script executed:
# Look at the preparePayloadScript function to understand how manifests are processed
# Search for the actual manifest list operations
rg -A 200 'func preparePayloadScript' control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go | grep -i 'packageserver\|manifest' | head -20Repository: openshift/hypershift
Length of output: 1654
🏁 Script executed:
# Check what the actual cleanup script contains in the test fixtures
sed -n '190,250p' control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-version-operator/zz_fixture_TestControlPlaneComponents_cluster_version_operator_deployment.yamlRepository: openshift/hypershift
Length of output: 2468
🏁 Script executed:
# Check if there's an actual packageserver PDB manifest in the CVO release that should be omitted
# Look at the preparePayloadScript more carefully to see where release-manifests come from
sed -n '198,280p' control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.goRepository: openshift/hypershift
Length of output: 4453
🏁 Script executed:
# Look for any comments or references to packageserver PDB manifest naming
# and check if there's a specific manifest file for it
rg '0000_.*packageserver' --type yaml control-plane-operator/controllers/hostedcontrolplane/v2/Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
# Check if there's a packageserver PDB manifest in the CVO payload by searching git history or release content
# First, let's check what files are being copied into the payload
rg 'release-manifests' control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go -B 5 -A 5 | head -30Repository: openshift/hypershift
Length of output: 1720
🏁 Script executed:
# Check if there's any packageserver PDB manifest from OpenShift release in the test data
# Look for what manifests are in the release-manifests directory
find control-plane-operator/controllers/hostedcontrolplane/testdata -name "*.yaml" | xargs grep -l 'kind: PodDisruptionBudget' | xargs grep -l 'packageserver'Repository: openshift/hypershift
Length of output: 2598
🏁 Script executed:
# Final verification: Check if there are any git changes or recent commits related to manifestsToOmit
git log --oneline -20 -- control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go 2>/dev/null || echo "Git log not available"Repository: openshift/hypershift
Length of output: 136
Add packageserver PDB manifest to manifestsToOmit array.
The packageserver PDB exists in the CVO release payload (confirmed by resourcesToRemove cleanup), but is missing from the manifestsToOmit array. While resourcesToRemove will clean it up on upgrades, manifestsToOmit is needed to prevent the manifest from being deployed on new clusters in the first place. Without this entry, the CVO's packageserver PDB is unnecessarily created before being removed by the cleanup manifest, rather than being omitted from deployment initially.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go`
at line 270, Add the packageserver PodDisruptionBudget manifest to the
manifestsToOmit list so it is never deployed (rather than created then removed);
specifically append the PodDisruptionBudget entry for Name="packageserver-pdb",
Namespace="openshift-operator-lifecycle-manager" (the same object literal used
in the diff: &policyv1.PodDisruptionBudget{ObjectMeta: metav1.ObjectMeta{Name:
"packageserver-pdb", Namespace: "openshift-operator-lifecycle-manager"}}) into
the manifestsToOmit array where other omitted manifests are listed (look for the
manifestsToOmit slice/variable in this file) so the CVO will skip deploying that
PDB on new clusters.
|
/retest |
86fec73 to
938c84c
Compare
…ster The packageserver PDB was being created in the guest cluster's openshift-operator-lifecycle-manager namespace by CVO, but packageserver pods run on the management cluster in the clusters-<hosted-cluster> namespace. This moves the PDB to the correct location. - Add packageserver PDB manifest to manifestsToOmit to prevent CVO from creating it in guest clusters - Add packageserver-pdb to resourcesToRemove for all platforms to clean up the orphaned PDB on existing clusters during upgrade - Register PDB manifest adapter in packageserver component to create the PDB in the management cluster namespace
|
/retest ci/prow/security |
|
/test security |
938c84c to
8bfaa36
Compare
|
Actionable comments posted: 0 |
c2beda8 to
08fa09c
Compare
|
/unassign |
Regenerate CVO deployment and packageserver component test fixtures to reflect the packageserver PDB being omitted from the guest cluster CVO payload and added to the management cluster namespace.
08fa09c to
1185ea9
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dhgautam99 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 |
|
@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. |
|
I now have all the evidence needed to produce the report. The root cause is clear: this is a Konflux infrastructure outage, not a code issue with PR #8459. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll four Konflux pipeline failures are caused by a transient Konflux infrastructure outage on 2026-06-01 between ~08:11 and ~10:12 UTC — not by any code change in PR #8459. Every pipeline failed at the very first substantive step ( Root CauseTransient Konflux CI infrastructure outage (network/registry connectivity failure) The Corroborating evidence that this is infrastructure, not code:
Recommendations
Evidence
|
|
/retest |
What this PR does / why we need it:
The packageserver PodDisruptionBudget was being created in the guest cluster's
openshift-operator-lifecycle-managernamespace by CVO. However, packageserverpods run on the management cluster in the
clusters-<hosted-cluster>namespace,making the guest cluster PDB ineffective.
This PR:
manifestsToOmit)resourcesToRemove)Which issue(s) this PR fixes:
Fixes OCPBUGS-54790
Special notes for your reviewer:
The PDB cleanup applies to all platforms (both IBM/PowerVS and default) since
packageserver runs on the management cluster regardless of platform.
Checklist:
Summary by CodeRabbit
Improvements
Tests