OCPBUGS-88757: Align HCCO CatalogSource registryPoll interval with OCP defaults#8745
OCPBUGS-88757: Align HCCO CatalogSource registryPoll interval with OCP defaults#8745shijadha wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@shijadha: This pull request references Jira Issue OCPBUGS-88757, which is invalid:
Comment 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
do-not-merge/work-in-progress
|
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 OLM catalog source reconciliation logic now sets the registry polling interval to 240 minutes instead of 10 minutes. The test file adds coverage for catalog source reconciliation helpers, including field defaults, placement-specific address/image handling, the updated polling interval, and annotation preservation. 🚥 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
|
Hi @shijadha. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/hostedclusterconfigoperator/controllers/resources/olm/catalogs.go`:
- Around line 44-45: Add a unit test to verify the 240-minute polling interval
configuration change for the CatalogSource. Create a test function that
instantiates a CatalogSource, calls the function that modifies it (likely
ReconcileRedHatOperatorsCatalogSource based on the context), and then asserts
that both the RawInterval field equals "240m" and the Interval.Duration field
equals 240*time.Minute as shown in the provided example test structure. This
test should be placed in the appropriate test file for the catalogs.go module.
- Around line 44-45: The polling interval changes for the four default catalog
sources (certified, community, marketplace, red-hat-operators) in the
catalogs.go file lack accompanying unit tests. Add unit tests that verify the
RawInterval and Interval fields are correctly configured for each of the four
default catalog sources with the 240-minute polling interval. The tests should
validate that the interval values are properly set and match across both the
string representation and the metav1.Duration object for each catalog source.
🪄 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: 7d0e7fc3-d98e-44a5-ab91-4db377db2305
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/olm/catalogs.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@oarribas: This pull request references Jira Issue OCPBUGS-88757, which is valid. The bug has been moved to the POST state. 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. |
|
/jira refresh |
|
@celebdor: This pull request references Jira Issue OCPBUGS-88757, 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/approve |
openshift-ci
Bot
added
the
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8745 +/- ##
==========================================
+ Coverage 43.53% 43.57% +0.03%
==========================================
Files 771 771
Lines 95798 95798
==========================================
+ Hits 41707 41741 +34
+ Misses 51192 51158 -34
Partials 2899 2899
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Per OpenShift policies, coderabbit comments should be addressed before pinging human reviews. Can you please address/respond to the coderabbit comments? |
shijadha
force-pushed
the
fix-catalogsource-poll-interval
branch
from
d83ffee to
d043889
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hostedclusterconfigoperator/controllers/resources/olm/catalogs_test.go`:
- Around line 52-56: The OLM catalog tests are dereferencing
RegistryPoll.Interval directly, which can panic if the field is unset; update
the assertions in the catalog test cases around the RegistryPoll checks to first
verify cs.Spec.UpdateStrategy.RegistryPoll.Interval is not nil before asserting
on .Duration. Keep the existing checks in the relevant test blocks in
catalogs_test.go, but add the nil guard wherever RegistryPoll.Interval is
accessed so failures are reported as assertions instead of panics.
🪄 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: b63c1d13-0fb1-4daa-8173-5f3cd9fe07ab
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/olm/catalogs.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/olm/catalogs_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/olm/catalogs.go
shijadha
force-pushed
the
fix-catalogsource-poll-interval
branch
from
d043889 to
28076e4
Compare
@bryan-cox , if I'm not wrong, latest coderabbit comments are now addressed. Is it possible to review this PR for merging? |
|
Looks like the |
|
/ok-to-test |
shijadha
force-pushed
the
fix-catalogsource-poll-interval
branch
from
28076e4 to
016db67
Compare
|
/ok-to-test |
shijadha
force-pushed
the
fix-catalogsource-poll-interval
branch
2 times, most recently
from
322fea9 to
53086ff
Compare
|
/ok-to-test |
There was a problem hiding this comment.
This doesn't seem related to the PR
…terval with OCP defaults Changes the OLM CatalogSource registry poll interval from 10m to 240m to match OpenShift Container Platform default behavior. Adds unit tests to verify the RegistryPoll configuration is correctly set and to prevent regressions on the interval value. Signed-off-by: Shital Jadhav <shijadha@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
shijadha
force-pushed
the
fix-catalogsource-poll-interval
branch
from
53086ff to
d151050
Compare
|
@bryan-cox The .gitignore change has been removed. The PR now only contains the catalog poll interval changes (10m → 240m) and the associated unit tests. Thanks for catching that! |
Looks like the |
@oarribas no it was removed because new code was pushed since lgtm was applied. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, muraee, shijadha The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I now have all the evidence needed. The failure is a CI infrastructure issue on the Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis failure is a CI infrastructure scheduling issue on the Root CauseThe pod
The net result: zero nodes were ever schedulable for this pod during the entire 1-hour window. This is a CI cluster capacity/resource contention issue — the Recommendations
Evidence
|
|
/retest |
|
/ok-to-test |
|
@shijadha: 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. |
6 participants
Summary
Updates the HCCO-managed CatalogSource
registryPoll.intervalfrom10mto240mto align with standard OCP 4.18+ defaults established by operator-marketplace.Context
In standard OCP clusters (4.18+), the operator-marketplace component sets the default
registryPollinterval to 240 minutes (4 hours) to address performance issues including unbounded etcd growth and high I/O on control plane nodes (see operator-marketplace PR #695 and OCPBUGS-69441).In HyperShift/HCP hosted clusters, the Hosted Cluster Config Operator (HCCO) manages CatalogSources independently with its own reconciliation logic in
catalogs.go, which still hardcodes the interval to 10 minutes. This creates inconsistency between standard OCP and HCP behavior.Changes
RawIntervalfrom"10m"to"240m"incatalogs.goIntervalduration from10 * time.Minuteto240 * time.MinuteBenefits
Trade-offs
spec.imagefieldRelated Issues
Testing
Summary by CodeRabbit
CatalogSourceregistry polling interval from 10 minutes to 240 minutes (4 hours) to reduce how frequently catalogs are checked..mcp.jsonfiles.CatalogSourcereconciliation tests validating default spec settings (including the 240-minute polling interval), management vs. guest placement behavior, and preservation/merging of existing annotations.