Skip to content

Conversation

@kquinn1204
Copy link
Contributor

@kquinn1204 kquinn1204 commented Sep 8, 2025

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Sep 8, 2025

Copy link
Contributor

@amolnar-gh amolnar-gh left a comment

Choose a reason for hiding this comment

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

Nice work, lost of good info here! I only have one important question that I mentioned in one of the comments. Has the NUMA Resources Operator name been approved by branding and legal? I couldn't find it in the official product name list. Also, why is the abbreviation NROP and not just NRO?

Everything else is small but recurring:

  • If you introduce an abbreviation (e.g. NROP), use it in the same module. I found that you introduce them but then don't use them in the rest of the module or not consistently. If you don't prefer using it, just don't introduce the abbreviation.
  • Use backticks for YAML elements consistently.
  • Define what things are (nodeGroups field or object or whatever)
  • It's more like a preference for me but the "made schedulable" phrase doesn't sound right to me. I also wonder if it could cause issues during translation? Would it be possible to use "change to or updated to schedulable". "made" could be vague

Copy link
Contributor

@slovern slovern left a comment

Choose a reason for hiding this comment

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

Great stuff here. I just found a few things that might be worth a look.

Copy link

@shajmakh shajmakh left a comment

Choose a reason for hiding this comment

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

Thanks @kquinn1204 for the work here, appreciated!
I left few comments. Overall I think it would be sufficient to point out that master nodes can now be part of the nodeGroups managed by the NRO CR under the condition they fulfill all the rules in order for the topology-aware scheduling to operate properly. Another point is regarding the verification section, when configuring "master" among the nodeGroups, we can avoid duplication and simply direct the user to the usual verification steps.
Most importantly, masters being supported in NROP is requires no special preparation apart from the usual one done for the worker pools and the operation of making the master nodes schedulable.

Comment on lines 197 to 198
- name: topologyManagerPolicy
value: none

Choose a reason for hiding this comment

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

this is an example of a bad configuration. essentioally whatever applied to worker nodegroup should also apply to any node group, masters are no special from NROP perspective.

Comment on lines 214 to 213
resources:
- allocatable: "0"
available: "0"
capacity: "4"
name: cpu
type: Node

Choose a reason for hiding this comment

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

another example for a misconfiguration of the nodeGroup MCP, memory resource is missing

@kquinn1204
Copy link
Contributor Author

@shajmakh I have revised the PR hopefully ready to be merged now. I have tried to install NUMA but facing some diffs with move to konflux

@kquinn1204
Copy link
Contributor Author

Thanks @shajmakh I believe I have captured your feedback, really appreciate your feedback on this and once again learned a lot.

@rshemtov13
Copy link

Thanks @shajmakh for the review here I have nothing more to add to it.
From QE
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2025
Copy link

@shajmakh shajmakh left a comment

Choose a reason for hiding this comment

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

Thanks @kquinn1204 for all the updates!
it LGTM with a small comment.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 6, 2025

New changes are detected. LGTM label has been removed.

Copy link
Contributor

@slovern slovern left a comment

Choose a reason for hiding this comment

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

Looking good. A few comments

@openshift-ci
Copy link

openshift-ci bot commented Oct 13, 2025

@kquinn1204: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@slovern slovern added this to the Planned for 4.20 GA milestone Oct 13, 2025
@slovern slovern merged commit 07fd717 into openshift:main Oct 13, 2025
2 checks passed
@slovern
Copy link
Contributor

slovern commented Oct 13, 2025

/cherrypick enterprise-4.20

@openshift-cherrypick-robot

@slovern: new pull request created: #100429

In response to this:

/cherrypick enterprise-4.20

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.20 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants