NO-JIRA: machine-api-termination-handler: add scc annotation, terminationMessage: FallbackToLogsOnError#1494
Conversation
The termination-handler container was missing terminationMessagePolicy=FallbackToLogsOnError
|
@damdo: 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughnewTerminationPodTemplateSpec now clones the shared pod annotations and adds the OpenShift SCC annotation for the termination-handler Pod template. The termination-handler container spec now sets TerminationMessagePolicy to TerminationMessageFallbackToLogsOnError. ChangesTermination Handler Pod Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
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 `@pkg/operator/sync.go`:
- Around line 1006-1011: The pod-template annotation map is being shared and
later mutated by ensureDependecyAnnotations, leaking deployment-specific
dependency-hash annotations into other pods; instead of relying on
maps.Clone(commonPodTemplateAnnotations) in only some places, ensure every use
creates an independent copy: replace any direct aliasing of
commonPodTemplateAnnotations (including where deployment pod templates are
created) with a fresh clone (e.g., call maps.Clone(commonPodTemplateAnnotations)
at each assignment site) or modify ensureDependecyAnnotations to accept and
mutate a copy rather than the original; update usages around
maps.Clone(commonPodTemplateAnnotations), commonPodTemplateAnnotations, and
ensureDependecyAnnotations so no shared map is passed into multiple pod
templates.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 53476141-736e-49dd-b290-250edf49b3c7
📒 Files selected for processing (1)
pkg/operator/sync.go
|
/retest |
pin the termination-handler DaemonSet pods to the dedicated machine-api-termination-handler SCC using the openshift.io/required-scc annotation. This makes the SCC binding explicit and prevents silent fallback to a different SCC if admission ordering changes
e527fdd to
d787f6b
Compare
|
@damdo: 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. |
2 similar comments
|
@damdo: 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. |
|
@damdo: 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. |
|
/lgtm |
|
@nrb: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb 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 |
set terminationMessagePolicy on termination-handler container
The termination-handler container was missing terminationMessagePolicy=FallbackToLogsOnError
require machine-api-termination-handler SCC via annotation
pin the termination-handler DaemonSet pods to the dedicated
machine-api-termination-handler SCC using the openshift.io/required-scc annotation.
This makes the SCC binding explicit and prevents silent fallback to a different
SCC if admission ordering changes
Summary by CodeRabbit