OCPBUGS-65617: fix(conformance): set priorityClass on global-pull-secret-syncer DaemonSet - #7236
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-65617, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request. 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. |
WalkthroughAdded an exported constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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 |
openshift-ci
Bot
added
the
approved
…onSet Set openshift-user-critical priorityClass to fix conformance test failures. Resolves empty priority class validation errors in kube-system pods. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
599fff1 to
d05cb18
Compare
|
/lgtm |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go
🔇 Additional comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go (1)
27-32: LGTM!The constant is properly defined and aligned with existing constants. The formatting is consistent.
| AutomountServiceAccountToken: ptr.To(false), | ||
| SecurityContext: &corev1.PodSecurityContext{}, | ||
| DNSPolicy: corev1.DNSDefault, | ||
| PriorityClassName: openshiftUserCriticalPriorityClass, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the priority class choice is appropriate for this system-level DaemonSet.
The assignment correctly sets a priority class, which addresses the conformance test failure. However, since this DaemonSet performs privileged system operations (writes to kubelet config, restarts kubelet service), consider verifying that openshift-user-critical is the intended priority class rather than system-node-critical, which is typically used for node-level critical infrastructure.
🌐 Web query:
What is the difference between openshift-user-critical and system-node-critical priority classes in OpenShift, and which should be used for DaemonSets that manage kubelet configuration?
💡 Result:
Short answer: use system-node-critical for DaemonSets that manage kubelet configuration.
Why:
- system-node-critical is reserved for pods that should never be evicted and protect node-level services. OpenShift lists node-essential components under system-node-critical. [1][2]
- openshift-user-critical is intended for important user/monitoring workloads (Prometheus, user-workload monitoring) that intentionally can drop priority when they over-consume resources so the scheduler has flexibility. It is not for node-level control-plane/agent DaemonSets. [2]
- Kubernetes/OpenShift priority semantics: higher priority prevents preemption/eviction for critical node services; node-management/ kubelet-affecting DaemonSets belong to the node-critical class. [3]
Citations:
- OpenShift docs — priority classes (system-node-critical, system-cluster-critical, openshift-user-critical). [1][2]
- Kubernetes Pod Priority & Preemption overview. [3]
Use system-node-critical priority class for this DaemonSet.
The system-node-critical priority class is reserved for pods that should never be evicted and protect node-level services, while openshift-user-critical is intended for important user/monitoring workloads that can intentionally drop priority when over-consuming resources. Since this DaemonSet manages kubelet configuration (a node-critical operation), it should use system-node-critical rather than openshift-user-critical.
Change openshiftUserCriticalPriorityClass to systemNodeCriticalPriorityClass at line 290.
🤖 Prompt for AI Agents
In
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go
around line 290, the DaemonSet currently sets PriorityClassName to
openshiftUserCriticalPriorityClass but must use systemNodeCriticalPriorityClass
because this manages kubelet/node-critical functionality; update the
PriorityClassName value from openshiftUserCriticalPriorityClass to
systemNodeCriticalPriorityClass and ensure the systemNodeCriticalPriorityClass
constant (or equivalent string "system-node-critical") is defined/available in
scope and used consistently.
|
/test e2e-aks-4-20 |
|
/test e2e-aws-upgrade-hypershift-operator |
|
/test e2e-aws-conformance |
|
@sjenning: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
/test e2e-conformance |
|
/retest-required |
|
/retest |
|
/test e2e-conformance |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-conformance |
|
/retest-required |
|
/test e2e-conformance |
1 similar comment
|
/test e2e-conformance |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-conformance |
|
/verified later @jparrill |
openshift-ci-robot
added
verified-later
verified
|
@csrwng: This PR has been marked to be verified later 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. |
|
@jparrill: The following test failed, say
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. |
|
/test e2e-aws |
e83d907
into
openshift:main
|
@jparrill: Jira Issue OCPBUGS-65617: All pull requests linked via external trackers have merged: This pull request has the 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. |
5 participants
What this PR does / why we need it:
Configure openshift-user-critical priorityClass on the
global-pull-secret-syncerDaemonSet to ensure conformance tests pass. This resolves the priority class validation failure where pods were found with empty priority class values.Which issue(s) this PR fixes: