OCPBUGS-101783: reconcile canary certificate on dependency creation - #1538
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@not-stbenjam: This pull request explicitly references no jira issue. 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-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe canary certificate controller now watches the default Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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
the
needs-ok-to-test
|
Hi @not-stbenjam. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
pkg/operator/controller/canary-certificate/controller.go (2)
87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd resource identity to the lookup error log.
Line 88 logs the failure without the IngressController namespace or name. Include
defaultICName.NamespaceanddefaultICName.Nameas structured fields.Proposed change
-log.Error(err, "Failed to get default IngressController") +log.Error(err, "Failed to get default IngressController", + "namespace", defaultICName.Namespace, + "name", defaultICName.Name, +)As per coding guidelines, “Use structured logging via go-logr/logr with relevant context (namespace, name, resource type).”
🤖 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/controller/canary-certificate/controller.go` around lines 87 - 89, Update the error log in the default IngressController lookup to include the resource identity as structured fields, using defaultICName.Namespace and defaultICName.Name alongside the existing error and message. Keep the existing not-found filtering and failure handling unchanged.Source: Coding guidelines
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap each Watch setup error with operation context.
Lines 101, 112, and 120 return raw errors. Identify the failed resource watch and preserve the error chain with
%w.Proposed change
- return nil, err + return nil, fmt.Errorf("failed to watch canary certificate Secrets: %w", err)Apply equivalent context for the
IngressControllerandDaemonSetwatches.As per coding guidelines, “Return errors with context using fmt.Errorf with %w format specifier for error wrapping to allow errors.Is/errors.As unwrapping.”
Also applies to: 111-113, 119-120
🤖 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/controller/canary-certificate/controller.go` around lines 100 - 102, Wrap the errors returned by each Watch setup in the controller’s watch-registration flow with fmt.Errorf using %w, adding resource-specific context for the Secret, IngressController, and DaemonSet watches while preserving the original error chain.Source: Coding guidelines
pkg/operator/controller/canary-certificate/controller_test.go (3)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required specific-test naming convention.
Rename these tests to end with
Functionality.Proposed change
-func TestCanaryCertificateDependencyEvents(t *testing.T) { +func TestCanaryCertificateDependencyEventsFunctionality(t *testing.T) { -func TestHasNamespacedNameRejectsUnrelatedDependencies(t *testing.T) { +func TestHasNamespacedNameRejectsUnrelatedDependenciesFunctionality(t *testing.T) {As per coding guidelines, “Follow test naming conventions: Test_foo for general tests, TestFooFunctionality for specific functionality tests.”
Also applies to: 84-84
🤖 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/controller/canary-certificate/controller_test.go` at line 20, Rename the specific functionality tests, including TestCanaryCertificateDependencyEvents and the additionally referenced test, so each name ends with Functionality while preserving their existing descriptive prefixes and test behavior.Source: Coding guidelines
67-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
testify/assertfor test assertions.Replace the direct
t.Fatalfassertion blocks withassert.True,assert.False,assert.Len, andassert.Equal. Keep the current failure context in assertion messages.As per coding guidelines, “Use testify/assert for assertions in tests.”
Also applies to: 105-109
🤖 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/controller/canary-certificate/controller_test.go` around lines 67 - 80, Update the test assertions in the t.Run cases around hasNamespacedName, tc.matches, and requests to use testify/assert instead of direct t.Fatalf calls. Use assert.True, assert.False, assert.Len, and assert.Equal as appropriate, preserving the existing failure context in assertion messages and applying the same change to the additional assertion block.Source: Coding guidelines
105-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun each negative dependency case as a subtest.
This table has three distinct cases. Add a static
descriptionfield and wrap each iteration int.Run(tc.description, ...). This isolates failures and identifies the rejected resource in test output.As per coding guidelines, “Use t.Run() with descriptive names for nested subtests in test files.” Based on learnings, use
t.Run()when a table has two or more distinct cases.🤖 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/controller/canary-certificate/controller_test.go` around lines 105 - 109, Update the negative dependency test loop to include a static description field in each test case and execute each case with t.Run(tc.description, ...), keeping the existing tc.matches assertion inside the subtest so failures identify the rejected resource.Sources: Coding guidelines, Learnings
🤖 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 `@pkg/operator/controller/canary-certificate/controller_test.go`:
- Line 20: Rename the specific functionality tests, including
TestCanaryCertificateDependencyEvents and the additionally referenced test, so
each name ends with Functionality while preserving their existing descriptive
prefixes and test behavior.
- Around line 67-80: Update the test assertions in the t.Run cases around
hasNamespacedName, tc.matches, and requests to use testify/assert instead of
direct t.Fatalf calls. Use assert.True, assert.False, assert.Len, and
assert.Equal as appropriate, preserving the existing failure context in
assertion messages and applying the same change to the additional assertion
block.
- Around line 105-109: Update the negative dependency test loop to include a
static description field in each test case and execute each case with
t.Run(tc.description, ...), keeping the existing tc.matches assertion inside the
subtest so failures identify the rejected resource.
In `@pkg/operator/controller/canary-certificate/controller.go`:
- Around line 87-89: Update the error log in the default IngressController
lookup to include the resource identity as structured fields, using
defaultICName.Namespace and defaultICName.Name alongside the existing error and
message. Keep the existing not-found filtering and failure handling unchanged.
- Around line 100-102: Wrap the errors returned by each Watch setup in the
controller’s watch-registration flow with fmt.Errorf using %w, adding
resource-specific context for the Secret, IngressController, and DaemonSet
watches while preserving the original error chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8632d40c-63f9-489a-9a26-c3a1a1c872f7
📒 Files selected for processing (2)
pkg/operator/controller/canary-certificate/controller.gopkg/operator/controller/canary-certificate/controller_test.go
stbenjam
changed the title
|
@not-stbenjam: This pull request references Jira Issue OCPBUGS-101783, 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
Review: Canary certificate dependency watch fixI was asked to review this PR by the team in #forum-ocp-network-edge. Here's my analysis — I'm an AI assistant (Chai Bot), so this is supplemental to human reviewer judgment. Root cause analysis ✅The race condition is accurately identified and well-documented. The existing c.Watch(source.Kind[client.Object](operatorCache, &corev1.Secret{}, ...))The This is a classic controller-runtime dependency-ordering race. Fix assessment ✅The approach is correct and minimal:
Observations
VerdictThe fix looks correct and well-scoped to me. It addresses a real payload-killing race condition with the standard controller-runtime pattern of watching all dependencies. I'd recommend an Ingress domain expert (cc @jcmoraisjr per OWNERS) give this a look and approve if they agree with the approach. Review requested from Slack by the release team. This is an AI-assisted review — final approval should come from a domain expert. AI-generated. Review for accuracy. |
|
/lgtm |
|
Scheduling tests matching the |
|
@coderabbitai are the failing checks due to code or infra? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openshift/cluster-ingress-operator"
pr_number="1538"
echo "== Pull request checks =="
gh pr checks "$pr_number" --repo "$repo" --json name,state,workflow,link,startedAt,completedAt
echo
echo "== Failed check runs =="
gh api "repos/$repo/commits/$(gh pr view "$pr_number" --repo "$repo" --json headRefOid --jq .headRefOid)/check-runs?status=completed&per_page=100" \
--jq '.check_runs[] | select(.conclusion == "failure" or .conclusion == "timed_out" or .conclusion == "cancelled" or .conclusion == "action_required") | {name, conclusion, details_url, output: {title: .output.title, summary: .output.summary, text: .output.text}}'Length of output: 7338 🏁 Script executed: #!/bin/bash
set -euo pipefail
urls=(
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-operator/2084341714843078656"
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-ovn-hypershift-conformance/2084341714922770432"
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-azure-operator/2084341715069571072"
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-gcp-operator/2084341715090542592"
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-vsphere-static-metallb-operator-gwapi/2084341715166040064"
)
for url in "${urls[@]}"; do
echo "===== $url ====="
page="$(curl -fsSL "$url")"
printf '%s' "$page" |
rg -io 'https?[^"[:space:]<>]+(build-log\.txt|finished\.json|junit[^"[:space:]<>]*\.xml|artifacts[^"[:space:]<>]*)' |
sed 's/&/\&/g' |
sort -u |
head -30
doneLength of output: 1912 🏁 Script executed: #!/bin/bash
set -u
prefixes=(
"pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-operator/2084341714843078656"
"pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-ovn-hypershift-conformance/2084341714922770432"
"pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-azure-operator/2084341715069571072"
"pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-gcp-operator/2084341715090542592"
"pr-logs/pull/openshift_cluster-ingress-operator/1538/pull-ci-openshift-cluster-ingress-operator-master-e2e-vsphere-static-metallb-operator-gwapi/2084341715166040064"
)
for prefix in "${prefixes[@]}"; do
echo "===== $prefix ====="
echo "-- finished.json --"
curl -fsSL "https://storage.googleapis.com/test-platform-results/$prefix/finished.json" 2>&1 || true
echo
echo "-- terminal build-log lines --"
curl -fsSL "https://storage.googleapis.com/test-platform-results/$prefix/build-log.txt" 2>&1 |
tail -250 |
rg -in -C 3 'fail|error|timeout|panic|degrad|install|test' || true
echo
doneLength of output: 118174
The evidence does not show one common CI infrastructure outage. The failures are unrelated platform-suite failures:
The modified canary paths passed in the Azure and GCP jobs. This includes I would classify these as unrelated e2e or payload/platform instability, not as failures introduced by this PR. You are interacting with an AI system. |
|
/retest |
1 similar comment
|
/retest |
|
Scheduling tests matching the |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@stbenjam: 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. |
| // permanently discard that Secret event. Generation filtering avoids | ||
| // reconciling on status-only updates. | ||
| isDefaultIngressController := predicate.NewPredicateFuncs(func(o client.Object) bool { | ||
| return isDefaultIngressControllerDependency(o, config.OperatorNamespace) | ||
| }) | ||
| if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorv1.IngressController{}, enqueueRequestForCanaryCertificate, predicate.And(isDefaultIngressController, predicate.GenerationChangedPredicate{}))); err != nil { |
There was a problem hiding this comment.
I thought we only cared about an out-of-order create event. Why do we care about update events?
There was a problem hiding this comment.
The create event is what fixes this bug, but update events matter too: the effective default-certificate Secret name is derived from the IngressController's spec.defaultCertificate. If that field is updated to point at a Secret that already exists, no Secret event fires (the isDefaultIngressCert predicate computes the effective name at event time, so it would only match a future event on the new Secret), and the canary certificate would keep mirroring the old certificate until something else nudged the controller. Watching spec updates closes that gap, and GenerationChangedPredicate keeps status-only updates from triggering reconciles.
|
You need a rebase to eliminate the merge commit in https://github.com/openshift/cluster-ingress-operator/pull/1538/commits. Less importantly, it would be nice to put the refactoring (introducing the |
6364c97 to
4a89a91
Compare
openshift-ci-robot
removed
the
verified
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@Miciah Rebased onto master to drop the merge commit and restructured the branch into three commits, with the fix first so it can be cherry-picked to release branches on its own (verified it applies cleanly to release-4.20 and passes unit tests there):
While restructuring, I also addressed the inline comments: reverted the |
The canary-certificate controller watches only Secrets. Its default-certificate predicate rejects the Secret event when the default IngressController does not exist yet, and if the IngressController is created afterward, no watched event retries the controller, so canary-serving-cert is never created and ingress degrades. Watch the default IngressController and the canary DaemonSet, the two non-Secret reconciliation dependencies, and map their events to the controller's single reconcile target so that dependency creation order cannot permanently discard the Secret event. Filter status-only updates with the generation predicate. Add predicate and event-mapping unit tests, including unrelated-resource rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the inline canary-certificate predicate closure with a named isCanaryCertificate function built on the hasNamespacedName helper, and extend the dependency-event unit tests to cover it. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The default-certificate predicate logs an error whenever the default IngressController lookup fails. A NotFound result is expected while the IngressController has not been created yet, so stop logging it as an error, and include the namespace and name in the log fields for the remaining unexpected failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4a89a91 to
fe1b58a
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
|
@stbenjam: 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. |
openshift-ci-robot
added
the
verified
|
@not-stbenjam: 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. |
|
@not-stbenjam: Jira Issue Verification Checks: Jira Issue OCPBUGS-101783 Jira Issue OCPBUGS-101783 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. |
11 participants
The canary-certificate controller can permanently miss reconciliation when its dependencies are created in an unlucky order. It watches only Secrets, and its default-certificate predicate rejects the Secret event when the default IngressController does not exist yet. If the IngressController appears afterward, no watched event retries the controller.
A failed 5.0 HyperShift AKS job captured the exact race: the default certificate existed, its initial event was rejected at 02:25:32.434Z, and the default IngressController appeared about two seconds later.
canary-serving-certwas never created; every canary pod remained Pending on the missing Secret, the Service had no endpoints, the admitted route returned EOF, and ingress degraded. A same-payload control with safe object ordering created the certificate successfully. This was the job's sole terminal cause, for a conservative historical impact of one green Prow job.This change:
Validation:
GOTOOLCHAIN=auto GOMAXPROCS=2 go test -mod=vendor -p=2 ./pkg/operator/controller/canary-certificategit diff --checkRepresentative job: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aks/2082648859074367488