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 nic feature discovery #589

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

adrianchiris
Copy link
Collaborator

No description provided.

@adrianchiris adrianchiris changed the title Add nic feature discovery [DNM] Add nic feature discovery Aug 21, 2023
@adrianchiris
Copy link
Collaborator Author

adrianchiris commented Aug 21, 2023

Still need to check this one on openshift cluster

@adrianchiris adrianchiris force-pushed the add-nic-feature-discovery branch 2 times, most recently from 31064a6 to 7dd84bf Compare August 21, 2023 16:20
This state will deploy nic-feature-discovery daemon

- Extend NicClusterPolicy CR with a new `nicFeatureDiscovery`
  field
- Add nic-feature-discovery manifests (yamls)
- Implement nic-feature-discovery state

Signed-off-by: adrianc <adrianc@nvidia.com>
@adrianchiris adrianchiris changed the title [DNM] Add nic feature discovery Add nic feature discovery Aug 22, 2023
@adrianchiris
Copy link
Collaborator Author

tested on both K8s and Openshift.

@e0ne @rollandf PTAL

Copy link
Member

@rollandf rollandf left a comment

Choose a reason for hiding this comment

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

LGTM

@adrianchiris
Copy link
Collaborator Author

@e0ne @ykulazhenkov PTAL, id like to try and merge this one by EOW.

Update Helm chart and templates tot support nic-feature-discovery
In addition remove imagePullPolicy for operator and fallback to
k8s defaults.

Signed-off-by: adrianc <adrianc@nvidia.com>
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -121,12 +121,18 @@ func newNicClusterPolicyStates(k8sAPIClient client.Client, scheme *runtime.Schem
nvIpamCniState, err := NewStateNVIPAMCNI(
k8sAPIClient, scheme, filepath.Join(manifestBaseDir, "state-nv-ipam-cni"))
if err != nil {
return nil, errors.Wrapf(err, "failed to create ib-kubernetes State")
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the fix!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

np, while not related to this PR, it just bothered me :) plus im the one who originally added it.

@adrianchiris
Copy link
Collaborator Author

@abdallahyas shouldnt we have CI reporting ?

@e0ne e0ne merged commit 1d6b5de into Mellanox:master Sep 22, 2023
9 checks passed
e0ne added a commit to e0ne/network-operator that referenced this pull request Sep 25, 2023
This commit reverts a change from Mellanox#589 to fix failing CI.

Signed-off-by: Ivan Kolodiazhny <ikolodiazhny@nvidia.com>
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

4 participants