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 |
openshift-ci-robot
added
jira/valid-reference
|
@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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughA PodDisruptionBudget manifest for the packageserver workload was added (packageserver-pdb with minAvailable: 1). The packageserver component now adapts this PDB manifest and a unit test for NewComponent was added. The CVO deployment code imports policy/v1 and updates its resourcesToRemove logic to include the packageserver-pdb in the cleanup list for the modified platform branches. Sequence Diagram(s)sequenceDiagram
participant Operator as HostedControlPlane Operator
participant Component as packageserver Component
participant CVO as CVO / payload generator
participant Kube as Kubernetes API
Operator->>Component: NewComponent()
Component->>Component: WithManifestAdapter(pdb.yaml -> AdaptPodDisruptionBudget)
Operator->>CVO: preparePayload / resourcesToRemove
CVO->>CVO: include packageserver-pdb in resourcesToRemove
CVO->>Kube: omit pdb from generated payload (when applicable)
Operator->>Kube: apply manifests (ensure packageserver-pdb exists for cleanup)
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-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 37.50% 40.36% +2.85%
==========================================
Files 751 755 +4
Lines 91992 93173 +1181
==========================================
+ Hits 34505 37606 +3101
+ Misses 54844 52864 -1980
- Partials 2643 2703 +60
... and 70 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:
|
dhgautam99
force-pushed
the
remove-packageserver-pdb-from-guest
branch
from
46bdd14 to
ffe96be
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/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
…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
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.
dhgautam99
force-pushed
the
remove-packageserver-pdb-from-guest
branch
from
ffe96be to
86fec73
Compare
|
New changes are detected. LGTM label has been removed. |
|
[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 |
![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
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.
|
@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. |
|
/retest |
|
I now have a comprehensive understanding of the situation. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThese two Konflux Enterprise Contract (EC) failures are not caused by PR #8459. They are a pre-existing, intermittent infrastructure issue specific to the Root CauseThe 2 EC policy failures are a pre-existing, intermittent infrastructure issue in the Konflux Enterprise Contract verification pipeline — not caused by this PR's code changes. Key evidence proving this is not PR-related:
Recommendations
Evidence
|
6 participants
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