GCP-441: correct CCM crash toleration key to match pod label - #7926
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
📝 WalkthroughWalkthroughAdded a TODO comment in the test utility ( 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
1 similar comment
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/util/util.go`:
- Around line 137-139: EnsureNoCrashingPods wrongly collapses provider-specific
CCM tolerations because getComponentName() prefers pod.Labels["app"], so the
"kubevirt-cloud-controller-manager": 2 entry can never match the new
"cloud-controller-manager" app label; fix by making getComponentName() prefer
provider-aware labels (check in order e.g. pod.Labels["app.kubernetes.io/name"],
pod.Labels["kubevirt.io/component"] or any provider-specific label) before
falling back to pod.Labels["app"], and/or update the podCrashTolerations map to
include both the generic "cloud-controller-manager" and the provider-specific
key (kubevirt-cloud-controller-manager) so the stricter 2-restart allowance for
KubeVirt is honored by EnsureNoCrashingPods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a1a53851-9fd9-4104-a526-55cd7a78c499
📒 Files selected for processing (1)
test/e2e/util/util.go
The podCrashTolerations map in e2e tests used "gcp-cloud-controller-manager" as the key, but getComponentName() returns the pod's "app" label which was "cloud-controller-manager" for all CCM providers. This caused the toleration added in GCP-441 to never match. Fix by changing the GCP CCM deployment's app label to "gcp-cloud-controller-manager" so getComponentName() returns the correct value. This is safe since GCP HCP is pre-GA with no existing clusters that would have an immutable selector mismatch. Ref: https://issues.redhat.com/browse/GCP-441 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a581e40 to
810b227
Compare
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
|
@cristianoveiga: This pull request references GCP-441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @cristianoveiga |
|
@cristianoveiga: 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. |
Test Resultse2e-aws
Failed TestsTotal failed tests: 24
... and 19 more failed tests e2e-aks
|
|
/retest |
1 similar comment
|
/retest |
|
/approve |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristianoveiga, sjenning 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 |
|
@cristianoveiga: 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. |
Revert the app label change from PR openshift#7926 that renamed the GCP CCM label from "cloud-controller-manager" to "gcp-cloud-controller-manager". The label was changed to support a separate crash toleration entry for the token-minter race condition (GCP-441), which is now fixed by injecting the token-minter as a native sidecar init container. Reverting the label fixes the EnsurePodsWithEmptyDirPVsHaveSafeToEvictAnnotations e2e test, which expects GCP CCM to match the existing "cloud-controller-manager" entry in the audited app list. Ref: https://issues.redhat.com/browse/GCP-447 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Revert the app label change from PR openshift#7926 that renamed the GCP CCM label from "cloud-controller-manager" to "gcp-cloud-controller-manager". The label was changed to support a separate crash toleration entry for the token-minter race condition (GCP-441), which is now fixed by injecting the token-minter as a native sidecar init container. Reverting the label fixes the EnsurePodsWithEmptyDirPVsHaveSafeToEvictAnnotations e2e test, which expects GCP CCM to match the existing "cloud-controller-manager" entry in the audited app list. Ref: https://issues.redhat.com/browse/GCP-447 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Revert the app label change from PR openshift#7926 that renamed the GCP CCM label from "cloud-controller-manager" to "gcp-cloud-controller-manager". The label was changed to support a separate crash toleration entry for the token-minter race condition (GCP-441), which is now fixed by injecting the token-minter as a native sidecar init container. Reverting the label fixes the EnsurePodsWithEmptyDirPVsHaveSafeToEvictAnnotations e2e test, which expects GCP CCM to match the existing "cloud-controller-manager" entry in the audited app list. Ref: https://issues.redhat.com/browse/GCP-447 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Revert the app label change from PR openshift#7926 that renamed the GCP CCM label from "cloud-controller-manager" to "gcp-cloud-controller-manager". The label was changed to support a separate crash toleration entry for the token-minter race condition (GCP-441), which is now fixed by injecting the token-minter as a native sidecar init container. Reverting the label fixes the EnsurePodsWithEmptyDirPVsHaveSafeToEvictAnnotations e2e test, which expects GCP CCM to match the existing "cloud-controller-manager" entry in the audited app list. Ref: https://issues.redhat.com/browse/GCP-447 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
What this PR does / why we need it:
Fixes the CCM crash toleration key in the e2e test suite. The podCrashTolerations map used "gcp-cloud-controller-manager" as the key, but getComponentName() returns the pod's app label which is "cloud-controller-manager" for all CCM providers (AWS, Azure, GCP, KubeVirt). This caused the toleration added in GCP-441 to never match, leaving the GCP CCM token-minter race condition unmitigated in CI.
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/GCP-441
Special notes for your reviewer:
This is a temporary workaround until https://issues.redhat.com/browse/GCP-447 is implemented, which will inject the token-minter as a native sidecar init container with a StartupProbe, eliminating the race condition entirely.
A TODO(GCP-447) comment has been added to track the removal of this toleration.
Note that the fix applies the toleration to all CCM providers (AWS, Azure, GCP, KubeVirt) since they all share the same app: cloud-controller-manager label. This is acceptable because the token-minter race condition affects all providers — it has only been observed in GCP CI due to the GCP CCM's fail-fast initialization.
Checklist:
Summary by CodeRabbit