Skip to content

Updated node type in testing doc, updated readme #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
k8s-ci-robot merged 3 commits into from
Dec 18, 2025
Merged

Updated node type in testing doc, updated readme #23

k8s-ci-robot merged 3 commits into from
Dec 18, 2025
+5 −3

Conversation

@sreeram-venkitesh
Copy link
Member

@sreeram-venkitesh sreeram-venkitesh commented Dec 7, 2025

Running the controller locally following the instructions here in the TEST_README.md file, it says that the controller pod should be scheduled to a platform node. But the controller manager is configured with an affinity for the control plane by default.

This PR updates TEST_README.md to mention the right node type and also links TEST_README.md in the main readme file under Ajay's demo video.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 7, 2025
@ajaysundark
Copy link
Contributor

ajaysundark commented Dec 8, 2025

Good catch! Thanks. I totally forgot about this change.
I think I handled this expecting it would make the 'kind' config simpler for testing. I wonder if that helps, should we keep control-plane installation for kind and just keep the old kind-config as reference? I expect any real k8s cluster deployment would be using a 'platform node' installation and we may have to capture the details somewhere.

@ajaysundark ajaysundark mentioned this pull request Dec 8, 2025
Closed
ajaysundark
ajaysundark reviewed Dec 8, 2025
View reviewed changes
ajaysundark
ajaysundark reviewed Dec 8, 2025
View reviewed changes
docs/TEST_README.md

The test demonstrates a realistic, production-aligned scenario where critical addons run on a dedicated platform node pool, and the NRG controller manages a network readiness taint on a separate application worker node.

### Test Topology
Copy link
Contributor

@ajaysundark ajaysundark Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need an update for this section?

Copy link
Member Author

@sreeram-venkitesh sreeram-venkitesh Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajaysundark Please take a look at the PR now. I've removed the update to the demo video and made all the other changes. In test_readme.md as well, I've updated our description of the nodes to mention that the controller would run in the control plane node by default unless configured otherwise.

Copy link
Contributor

@ajaysundark ajaysundark Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating on this, @sreeram-venkitesh. A minor observation from me, LGTM otherwise.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2025
@netlify
Copy link

netlify bot commented Dec 18, 2025 •
edited
Loading

✅ Deploy Preview for node-readiness-controller canceled.

Name Link
🔨 Latest commit c01f32c
🔍 Latest deploy log https://app.netlify.com/projects/node-readiness-controller/deploys/69444e6aca764700080a1016

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2025
ajaysundark
ajaysundark reviewed Dec 18, 2025
View reviewed changes
docs/TEST_README.md Outdated
```

Verify the controller is running on the platform node (`nrr-test-worker`):
Verify the controller is running on the control plane node (`nrg-test-control-plane`):
Copy link
Contributor

@ajaysundark ajaysundark Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this got renamed as nrr-test-* recently as part of rebranding (re: #37)

@ajaysundark ajaysundark self-assigned this Dec 18, 2025
@ajaysundark
Copy link
Contributor

ajaysundark commented Dec 18, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2025
ajaysundark
ajaysundark approved these changes Dec 18, 2025
View reviewed changes
Copy link
Contributor

@ajaysundark ajaysundark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! There's a minor typo that need a fix. Please see if you could fix it before your merge.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, sreeram-venkitesh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2025
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2025
@sreeram-venkitesh
Copy link
Member Author

sreeram-venkitesh commented Dec 18, 2025

@ajaysundark Thanks for catching that! I missed that since I had made this branch before the renaming. Looks like this needs a new lgtm

@ajaysundark
Copy link
Contributor

ajaysundark commented Dec 18, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2025
@sreeram-venkitesh
Copy link
Member Author

sreeram-venkitesh commented Dec 18, 2025

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit c5c51a3 into kubernetes-sigs:main Dec 18, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajaysundark ajaysundark ajaysundark approved these changes

@haircommander haircommander Awaiting requested review from haircommander

@tallclair tallclair Awaiting requested review from tallclair

Assignees

@ajaysundark ajaysundark

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sreeram-venkitesh @ajaysundark @k8s-ci-robot