OCPBUGS-85680: Re-fetch infraConfig on every periodic loop iteration#1458
OCPBUGS-85680: Re-fetch infraConfig on every periodic loop iteration#1458Thealisyed wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@Thealisyed: This pull request references Jira Issue OCPBUGS-85680, 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. |
📝 WalkthroughWalkthroughThis pull request adds test coverage for replica determination logic and refactors the 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Previously, Operator.Start() fetched the Infrastructure config once at startup and reused the stale object in the periodic loop that ensures the default IngressController exists. If topology changed at runtime, the operator would never react. We are moving the Infrastructure config fetch inside the loop to match the pattern already used for the Ingress config. The ExternalTopologyMode check is also evaluated on each tick, allowing the operator to respond to topology transitions without requiring a restart. Additionally we also add unit tests for DetermineReplicas() which had no test coverage. Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
Thealisyed
force-pushed
the
OCPBUGS-85680/refetch-infraconfig
branch
from
4916fca to
02161d5
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Thealisyed: This pull request references Jira Issue OCPBUGS-85680, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
do a review on my PR please @coderabbitai :) |
|
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
pkg/operator/operator.go (1)
389-392: 💤 Low valueConsider using
log.Infoor providing error context for cache sync failure.
WaitForCacheSyncreturns a boolean, not an error. Passingniltolog.Erroris semantically unclear. Consider either usinglog.Infowith an appropriate message, or providing context about why the cache sync failed.Suggested alternative
if !o.manager.GetCache().WaitForCacheSync(ctx) { - log.Error(nil, "failed to sync cache before ensuring default ingresscontroller") + log.Info("cache sync incomplete before ensuring default ingresscontroller, will retry") return }🤖 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 `@pkg/operator/operator.go` around lines 389 - 392, The current cache sync failure uses log.Error(nil, ...) which passes nil instead of an error; update the block where o.manager.GetCache().WaitForCacheSync(ctx) is checked (the WaitForCacheSync call) to either log an informational message with log.Info describing the sync failure (e.g., "cache not synced before ensuring default ingresscontroller") or construct a contextual error and pass it to log.Error so the call is not given nil; ensure you update the message to reference the operation (ensuring default ingresscontroller) for clarity and keep the return behavior the same.
🤖 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 `@pkg/operator/operator.go`:
- Around line 394-407: The client.Get calls inside Start() are using
context.TODO() which prevents cancellation propagation; update those calls to
use the ctx parameter passed into Start() so the fetches respect shutdown
signals—specifically replace context.TODO() in the o.client.Get calls that fetch
operatorcontroller.InfrastructureClusterConfigName() into infraConfig and
operatorcontroller.IngressClusterConfigName() into ingressConfig with the
Start() ctx variable.
---
Nitpick comments:
In `@pkg/operator/operator.go`:
- Around line 389-392: The current cache sync failure uses log.Error(nil, ...)
which passes nil instead of an error; update the block where
o.manager.GetCache().WaitForCacheSync(ctx) is checked (the WaitForCacheSync
call) to either log an informational message with log.Info describing the sync
failure (e.g., "cache not synced before ensuring default ingresscontroller") or
construct a contextual error and pass it to log.Error so the call is not given
nil; ensure you update the message to reference the operation (ensuring default
ingresscontroller) for clarity and keep the return behavior the same.
🪄 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: e467154e-ec52-4b2d-8ae7-09944914a2a6
📒 Files selected for processing (2)
pkg/operator/controller/ingress/replicas_test.gopkg/operator/operator.go
|
/assign @grzpiotrowski |
|
@Thealisyed: This pull request references Jira Issue OCPBUGS-85680, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
Infra failures unrelated to our changes /retest |
|
@Thealisyed: 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. |
|
/assign @davidesalerno |
| // Periodically ensure the default controller exists. | ||
| go wait.Until(func() { | ||
| if !o.manager.GetCache().WaitForCacheSync(ctx) { | ||
| log.Error(nil, "failed to sync cache before ensuring default ingresscontroller") |
There was a problem hiding this comment.
I think that since there is no actual error, log.Info(...) is cleaner than log.Error(nil, ...).
|
The main fix looks good to me — moving infra fetch into the periodic loop correctly addresses the stale topology problem. Only one minor nit related to logging. |
|
marking as verified since it ingress unit test is passing |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: 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. |
7 participants
Problem
The
Infrastructureconfig (which includesControlPlaneTopology) is fetched once duringOperator.Start()and reused on every 1 minute reconciliation tick. If the topology changes at runtime, the operator never detects it and continues using stale configuration until it is restarted.This was identified as problematic
We follow the pattern of the
Ingressconfig where it is already re-fetched inside the periodic loop.Solution
Infrastructureconfig fetch inside the periodicwait.Untilloop, matching the pattern already used for theIngressconfigExternalTopologyModecheck is now evaluated each tick, allowing the operator to respond to topology transitions without a restartDetermineReplicas()which had no prior test coverage