Feat: RunOnInfra to use openshift.io/node-selector annotation on namespace#1194
Open
anandrkskd wants to merge 7 commits into
Open
Feat: RunOnInfra to use openshift.io/node-selector annotation on namespace#1194anandrkskd wants to merge 7 commits into
anandrkskd wants to merge 7 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR moves ChangesInfra Node Selector via Namespace Annotation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go (1)
137-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the fallback nodeSelector explicitly after cleanup.
ShouldNot(HaveTemplateSpecNodeSelector({"kubernetes.io/os":"linux","key1":"value1"}))only proveskey1is gone. It also passes if the controller drops the defaultkubernetes.io/os=linuxselector entirely. Please assert the final selector equals the default map instead of only asserting inequality.🤖 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 `@test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go` around lines 137 - 153, The cleanup assertions in the node selector test are too weak because ShouldNot(HaveTemplateSpecNodeSelector(...)) still passes if the default selector is removed entirely. Update the assertions in the loop over deploymentNameList and the appControllerSS check to verify the final template spec node selector matches the expected fallback/default selector exactly, using the existing HaveTemplateSpecNodeSelector helper on the Deployment and StatefulSet fixtures.Source: Path instructions
🧹 Nitpick comments (1)
controllers/gitopsservice_controller_test.go (1)
643-650: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a preservation case for custom
spec.nodeSelector.The PR contract says
RunOnInfrashould stop injecting the infra selector without dropping user-provided node selectors, but this test only covers the empty-selector path. A regression that clearsGitopsServiceSpec.NodeSelectorwould still pass here. Please add a case withSpec.NodeSelectorpopulated and assert that the backendDeploymentandArgoCD.Spec.NodePlacement.NodeSelectorkeep that map while the namespace annotation is still set.🤖 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 `@controllers/gitopsservice_controller_test.go` around lines 643 - 650, Add a preservation test for custom GitopsServiceSpec.NodeSelector in the RunOnInfra path, since the current gitopsservice_controller_test only verifies the empty-selector case. Update the relevant test setup around the GitopsService reconciliation and the ArgoCD assertions so a populated NodeSelector is provided, then verify the backend Deployment and ArgoCD.Spec.NodePlacement.NodeSelector both retain that map while the namespace annotation still reflects infra placement. Use the existing GitopsService, ArgoCD, and deployment checks to ensure user-provided selectors are not cleared.
🤖 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 `@controllers/gitopsservice_controller.go`:
- Around line 100-106: The GitOpsService bootstrap in the controller’s setup
path only logs failures from r.Client.Create and then continues, which lets
SetupWithManager succeed even when creation actually failed. Update the
bootstrap flow around the Create call in the GitopsService controller to return
the non-AlreadyExists error to the caller, while still treating
errors.IsAlreadyExists as a non-fatal case. Make sure the setup method that
invokes this creation propagates the error so operator startup fails fast on
RBAC/API issues instead of leaving the singleton uncreated.
- Around line 1029-1045: The ensureInfraNodeSelectorAnnotation helper is
overwriting or deleting the entire openshift.io/node-selector annotation instead
of preserving any existing selector terms. Update this function so it only adds
the infra selector when runOnInfra is true and removes just that infra clause
when false, leaving any other node-selector constraints intact; use
ensureInfraNodeSelectorAnnotation and
common.InfraNodeSelectorAnnotation/common.InfraNodeSelectorAnnotationValue to
locate the logic.
In `@test/openshift/e2e/ginkgo/sequential/1-028-validate_run_on_infra_test.go`:
- Around line 144-153: The disable-path assertion in the OpenShift GitOps e2e
test is too narrow because it only checks the namespace annotation and the
cluster deployment. Update the verification in the same test block that uses
openshiftGitopsNS and clusterDepl so it asserts the full openshift-gitops
workload set, matching the enable-path coverage by checking all relevant
deployments/workloads no longer have the infra toleration after RunOnInfra is
disabled.
---
Outside diff comments:
In `@test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go`:
- Around line 137-153: The cleanup assertions in the node selector test are too
weak because ShouldNot(HaveTemplateSpecNodeSelector(...)) still passes if the
default selector is removed entirely. Update the assertions in the loop over
deploymentNameList and the appControllerSS check to verify the final template
spec node selector matches the expected fallback/default selector exactly, using
the existing HaveTemplateSpecNodeSelector helper on the Deployment and
StatefulSet fixtures.
---
Nitpick comments:
In `@controllers/gitopsservice_controller_test.go`:
- Around line 643-650: Add a preservation test for custom
GitopsServiceSpec.NodeSelector in the RunOnInfra path, since the current
gitopsservice_controller_test only verifies the empty-selector case. Update the
relevant test setup around the GitopsService reconciliation and the ArgoCD
assertions so a populated NodeSelector is provided, then verify the backend
Deployment and ArgoCD.Spec.NodePlacement.NodeSelector both retain that map while
the namespace annotation still reflects infra placement. Use the existing
GitopsService, ArgoCD, and deployment checks to ensure user-provided selectors
are not cleared.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7fa859f0-e8d7-4ffd-a261-4e3fd2dd3cfc
📒 Files selected for processing (9)
common/common.gocontrollers/consoleplugin.gocontrollers/consoleplugin_test.gocontrollers/gitopsservice_controller.gocontrollers/gitopsservice_controller_test.gotest/e2e/gitopsservice_test.gotest/openshift/e2e/ginkgo/fixture/namespace/fixture.gotest/openshift/e2e/ginkgo/sequential/1-028-validate_run_on_infra_test.gotest/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
💤 Files with no reviewable changes (1)
- controllers/consoleplugin.go
Contributor
Author
|
/retest |
anandf
reviewed
Jul 6, 2026
anandf
requested changes
Jul 6, 2026
anandf
left a comment
anandf
left a comment
Member
There was a problem hiding this comment.
posted a minor comment otherwise looks good to me.
…pod infra NodeSelector with openshift.io/node-selector annotation on default namespace. OpenShift admission controller applies selector to all pods in namespace automatically. Tolerations and custom NodeSelector remain at pod level. assited-by: claude-code Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
…o/infra=' Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
assisted-by: claude-code Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
anandrkskd
force-pushed
the
infra-annotation-support
branch
from
3303425 to
f7b1867
Compare
Member
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svghadi 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 |
|
New changes are detected. LGTM label has been removed. |
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
This PR changes
RunOnInfrafield implementation it addsopenshift.io/node-selector="node-role.kubernetes.io/infra=“annotation on namespace and let openshift make sure the pods are scheduled on infra nodes. Previous implementation used to add node-role label to resources directly.Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes GITOPS-9926
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
make install runspec.runOnInfra: trueopenshift.io/node-selector="node-role.kubernetes.io/infra=“