Skip to content
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.

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

Add NodeFeatureRules to label Nvidia NICs #571

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

wmousa
Copy link
Contributor

@wmousa wmousa commented Jul 12, 2023

This patch does the following:

  • Add "Nvidia NICs PCI" NFR to lable the node with pci-15b3.present": "true" if any of Nvidia NICs is exist
  • Add "Nvidia NICs SRIOV" NFR to lable the node with pci-15b3.sriov.capable": "true" if any of Nvidia NICs has SRIOV enabled
  • Add nfd.nodefeaturerules flag and set it true by default to allow creating NFRs above
  • Remove the networking PCI class from the source configuration

@wmousa wmousa force-pushed the add_nfrs branch 4 times, most recently from 63b2b75 to 6486b0c Compare July 13, 2023 08:55
Comment on lines -55 to -57
- "02"
- "0200"
- "0207"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave this, in case the users decide to set deployNodeFeatureRules to false. NFD can handle the duplicated config giving NodeFeatureRule priority

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggested to waleed to remove to avoid having duplicate places that label the same thing.

if this is left here, then why create nodefeaturerule by default. i think we want to rely on one mechanism for most cases.

since we may have external NFD deployed (which we require to have nodefeaturerules enabled) which then will require us to use nodefeaturerules created. i prefer to always rely on it.

if user decides to set this one to false, he needs to be aware of the implications. IMO.
in that case the user either:

  1. has another NFD deployed with all needed configs
  2. user would need to change this entry to add the values needed
  3. or simply keep deployNodeFeatureRules true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, Done

README.md Show resolved Hide resolved
@adrianchiris
Copy link
Collaborator

we need to add a section in README on the different ways NFD can be deployed.

  1. externally
  2. via network operator

if externally, we need to specify requirements. i.e version, expected features to be enabled and install example using helm from upstream repo

@wmousa wmousa force-pushed the add_nfrs branch 2 times, most recently from ee7cbe0 to dec8fc4 Compare July 16, 2023 08:22
README.md Outdated Show resolved Hide resolved
This patch does the following:
 - Add "Nvidia NICs PCI" NFR to lable the node with `pci-15b3.present": "true"`
   if any of Nvidia NICs is exist
 - Add nfd.deployNodeFeatureRules flag and set it true by default to allow creating NFRs above
 - Remove the networking PCI class from the source configuration
 - Enhance READE documentation

Signed-off-by: Waleed Mousa <waleedm@nvidia.com>
@adrianchiris adrianchiris merged commit ce741a8 into Mellanox:master Jul 17, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants