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

Use hostNetwork for sriov-cni daemonset #149

Merged

Conversation

zshi-redhat
Copy link
Collaborator

@zshi-redhat zshi-redhat commented Jun 25, 2021

When ovn hardware offload is enabled with ovn-k8s as default
CNI plugin, pods using veth as default interface type can no
longer be created since OpenvSwitch is offloaded to the
SmartNIC system (with current design). sriov-cni pod is affected
in such case. This commit moves sriov-cni into config daemonset
which runs in host network and doesn't use veth interface.

This also remove the sriov-cni daemonset created by Operator.

@zshi-redhat zshi-redhat requested a review from pliurh June 25, 2021 06:37
@pliurh
Copy link
Collaborator

pliurh commented Jun 29, 2021

/lgtm

@github-actions github-actions bot added the lgtm label Jun 29, 2021
@pliurh
Copy link
Collaborator

pliurh commented Jun 30, 2021

Considering the job of the sriov-cni daemon is just to put the CNI binary to the node, it's a one-time job. Can we remove this DS, and put it as the init container of sriov network config daemon?

@martinkennelly
Copy link
Member

@zshi-redhat Could you explain why this is needed? I don't understand the need to give the DS this privilege since its only function is to place the binary in cni bin dir.

@zshi-redhat
Copy link
Collaborator Author

@zshi-redhat Could you explain why this is needed? I don't understand the need to give the DS this privilege since its only function is to place the binary in cni bin dir.

@martinkennelly This is related to the ovn hardware offload use case. When it is enable on the worker node with ovn-kubernetes CNI, we can no longer create regular veth pods on that particular node since openvswitch is offloaded to the SmartNIC system (vs on the host).

Discussed with @pliurh , we were thinking to delete sriov-cni daemonset and add it as a container in sriov-config-daemon pod.

@adrianchiris
Copy link
Collaborator

/lgtm

@zshi-redhat mind updating commit message why we are moving sriov-cni container to sriov-config-daemon ?

this also has the benefit of reducing the resources this operator creates (one less daemonset that k8s needs to monitor)

@zshi-redhat
Copy link
Collaborator Author

/lgtm

@zshi-redhat mind updating commit message why we are moving sriov-cni container to sriov-config-daemon ?

this also has the benefit of reducing the resources this operator creates (one less daemonset that k8s needs to monitor)

Updated commit message.

Copy link
Collaborator

@adrianchiris adrianchiris 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

I have not tested this change (no e2e test ci yet :( ), so @zshi-redhat if you have done some testing internally this can be merged IMO

@zshi-redhat
Copy link
Collaborator Author

I have not tested this change (no e2e test ci yet :( ), so @zshi-redhat if you have done some testing internally this can be merged IMO

I built customized image and tried to create a policy, it worked as expected: no sriov-cni daemonset was created, sriov-cofig-daemon was created with several containers, including sriov-cni.

@pliurh
Copy link
Collaborator

pliurh commented Jul 1, 2021

We also need to remove the legacy daemonsets of the CNI-plugins during an upgrade.

When ovn hardware offload is enabled with ovn-k8s as default
CNI plugin, pods using veth as default interface type can no
longer be created since OpenvSwitch is offloaded to the
SmartNIC system (with current design). sriov-cni pod is affected
in such case. This commit moves sriov-cni into config daemonset
which runs in host network and doesn't use veth interface.

This also remove the sriov-cni daemonset created by Operator.
@zshi-redhat
Copy link
Collaborator Author

We also need to remove the legacy daemonsets of the CNI-plugins during an upgrade.

Added code to remove sriov-cni manifests

@pliurh pliurh merged commit 4783061 into k8snetworkplumbingwg:master Jul 8, 2021
abdallahyas added a commit to abdallahyas/sriov-network-operator that referenced this pull request Jul 11, 2021
Following k8snetworkplumbingwg#149, sriov-cni
daemonset is no longer deployed separatly but as part of the config
daemon daemonset. This lead to break the E2E tests, since the E2E
tests wait for the sriov-cni daemonsets to be ready.

This patch fix that by replacing the wait condition from the sriov-cni
to the config-daemon daemonset.
abdallahyas added a commit to abdallahyas/sriov-network-operator that referenced this pull request Jul 11, 2021
Following k8snetworkplumbingwg#149, sriov-cni
daemonset is no longer deployed separatly but as part of the config
daemon daemonset. This breaks the E2E tests, since the E2E tests wait
for the sriov-cni daemonsets to be ready.

This patch fix that by replacing the wait condition from the sriov-cni
daemonset to the config-daemon daemonset.
pliurh pushed a commit to pliurh/sriov-network-operator-1 that referenced this pull request Jul 14, 2021
Following k8snetworkplumbingwg/sriov-network-operator#149, sriov-cni
daemonset is no longer deployed separatly but as part of the config
daemon daemonset. This breaks the E2E tests, since the E2E tests wait
for the sriov-cni daemonsets to be ready.

This patch fix that by replacing the wait condition from the sriov-cni
daemonset to the config-daemon daemonset.
pliurh pushed a commit to pliurh/sriov-network-operator-1 that referenced this pull request Jul 14, 2021
Following k8snetworkplumbingwg/sriov-network-operator#149, sriov-cni
daemonset is no longer deployed separatly but as part of the config
daemon daemonset. This breaks the E2E tests, since the E2E tests wait
for the sriov-cni daemonsets to be ready.

This patch fix that by replacing the wait condition from the sriov-cni
daemonset to the config-daemon daemonset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants