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 flag to enable SwitchDev mode in SriovNetworkNodePolicy CRD #379

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

pliurh
Copy link
Contributor

@pliurh pliurh commented Oct 26, 2020

The generic plugin will skip configuring swichdev devices. It shall be done
through a systemd services on the host.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2020
@pliurh
Copy link
Contributor Author

pliurh commented Oct 26, 2020

@moshe010 @Mmduh-483 @adrianchiris PTAL.

@pliurh pliurh force-pushed the upgrade-sdk-101 branch 4 times, most recently from 84800e7 to 862a49e Compare October 26, 2020 08:22
Copy link
Contributor

@zshi-redhat zshi-redhat left a comment

Choose a reason for hiding this comment

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

How do we handle nodeState syncStatus for switchdev policy?
Until MCO reboots the node, will syncStatus be succeeded?

pkg/utils/utils.go Outdated Show resolved Hide resolved
pkg/plugins/generic/generic_plugin.go Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
pkg/plugins/generic/generic_plugin.go Outdated Show resolved Hide resolved
api/v1/helper.go Outdated Show resolved Hide resolved
api/v1/sriovnetworknodepolicy_types.go Outdated Show resolved Hide resolved
api/v1/sriovnetworknodestate_types.go Outdated Show resolved Hide resolved
api/v1/sriovnetworknodestate_types.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@pliurh pliurh left a comment

Choose a reason for hiding this comment

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

How do we handle nodeState syncStatus for switchdev policy?
Until MCO reboots the node, will syncStatus be succeeded?

That's a good question. In this patch, the config daemon doesn't handle the syncStatus for switchdev policy. It will just become 'Succeed'. If we make it return a failure, the config daemon would keep retrying. I am wondering whether we have a better way to handle it.

@zshi-redhat
Copy link
Contributor

How do we handle nodeState syncStatus for switchdev policy?
Until MCO reboots the node, will syncStatus be succeeded?

That's a good question. In this patch, the config daemon doesn't handle the syncStatus for switchdev policy. It will just become 'Succeed'. If we make it return a failure, the config daemon would keep retrying. I am wondering whether we have a better way to handle it.

If cluster nodes are applied with mixed policies (switchdev and non-switchdev), would there be a problem that MCO and sriov config daemon are not synced on which node is being rebooted, which may result in interrupted workloads?

Maybe we need a sync mechenism between Operator applying MachineConfig and config daemon?
Shall we use sriov config daemon to write the device switchdev systemd config?

@@ -1,6 +1,6 @@

---
apiVersion: apiextensions.k8s.io/v1beta1
apiVersion: apiextensions.k8s.io/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not related to the switchdev

Copy link
Contributor

Choose a reason for hiding this comment

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

There are several CRD files changes not related to eswitch mode, it seems you are not using the same version of the operator-sdk, IMO it can be in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it can be in a separate PR. Those changes were mistakenly introduced by the previous operator upgrade.

@@ -186,6 +186,11 @@ func needDrainNode(desired sriovnetworkv1.Interfaces, current sriovnetworkv1.Int
for _, iface := range desired {
if iface.PciAddress == ifaceStatus.PciAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to continue the loop after finding the device, maybe adding break at end of if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -595,3 +603,12 @@ func generateRandomGuid() net.HardwareAddr {

return guid
}

func GetNicSriovMode(pciAddress string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pliurh Will SetNicSriovMode be in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, at least not for openshift. The plan is to use the Machine Config Operator to insert a new systemd service which will configure the VF and set NIC to switchdev mode on the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this release, we only use switchdev for OVN offloading use case. And we want users can use VF bond with it. In openshift-sdn, bond is created by NetworkManager configure file inserted by MCO. So we have to configure switchdev before NetworkManager starts on the host.

Copy link
Contributor

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

So i would be happy if the commit message provided more context on:

  1. The problem this PR is trying to solve
  2. How it attempts to solve it (general outline of the changes)

Also an outline on how this will work in the full solution is appreciated.

Makefile Outdated
@@ -39,7 +39,7 @@ BUNDLE_METADATA_OPTS ?= $(BUNDLE_CHANNELS) $(BUNDLE_DEFAULT_CHANNEL)
# Image URL to use all building/pushing image targets
IMG ?= controller:latest
# Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
CRD_OPTIONS ?= "crd:trivialVersions=true"
CRD_OPTIONS ?= "crd:crdVersions={v1},trivialVersions=true"
Copy link
Contributor

Choose a reason for hiding this comment

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

can the CRD version bump be in a separate commit in this PR ? or a separate PR ?

i.e recreate CRDs with the new operator SDK without switchdev change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed by #389

MANIFESTS_PATH = "./bindata/manifests/cni-config"
LASTNETWORKNAMESPACE = "operator.sriovnetwork.openshift.io/last-network-namespace"
FINALIZERNAME = "netattdef.finalizers.sriovnetwork.openshift.io"
ESWITCHMODE_LEGACY = "legacy"
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self These should really be constants in netlink package.

@adrianchiris
Copy link
Contributor

Adding this for reference. openshift/enhancements#296

@adrianchiris
Copy link
Contributor

adrianchiris commented Oct 28, 2020

Also an outline on how this will work in the full solution is appreciated.

as well as more information on the systemd service, what it will do and who will deploy it.

@pliurh pliurh force-pushed the upgrade-sdk-101 branch 2 times, most recently from 0bdeff9 to 3affa72 Compare November 3, 2020 02:19
@pliurh
Copy link
Contributor Author

pliurh commented Nov 3, 2020

as well as more information on the systemd service, what it will do and who will deploy it.

PTAL #393

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2020
@zshi-redhat zshi-redhat changed the title Add flag to enalbe SwitchDev mode in SriovNetworkNodePolicy CRD Add flag to enable SwitchDev mode in SriovNetworkNodePolicy CRD Nov 13, 2020
@pliurh
Copy link
Contributor Author

pliurh commented Nov 13, 2020

@zshi PTAL

@@ -115,6 +118,11 @@ func SyncNodeState(newState *sriovnetworkv1.SriovNetworkNodeState) error {
for _, iface := range newState.Spec.Interfaces {
if iface.PciAddress == ifaceStatus.PciAddress {
configured = true
if iface.EswitchMode == sriovnetworkv1.ESWITCHMODE_SWITCHDEV {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pliurh this part will be updated in the following PR, right? e.g. To not skip switchdev device in daemon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2020
1. Add 'eSwitchMode' flag in CRD SriovNetworkNodePoliy can SriovNetworkNodeState.
2. Make config daemon to be able to detect the eswitch mode of NIC, when switchedv
is enabled.

The VF and switchdev configuration shall be done with a systemd service on host. That
will be covered in a separate PR.
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2020
@zshi-redhat
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pliurh, zshi-redhat

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

The pull request process is described here

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

@pliurh
Copy link
Contributor Author

pliurh commented Nov 16, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@pliurh
Copy link
Contributor Author

pliurh commented Nov 18, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit 20e3460 into openshift:master Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants