Support Externally Created virtual functions #436

SchSeba · 2023-05-08T14:29:04Z

this PR adds the new API to allow the operator to use virtual functions
that the user configures manually on the system

this will allow new use-cases like using a VF as the SDN primary nic.
the nic will be created before the operator starts by the user, for
example using nmstate or NetworkManager and the operator will allocate
all the remaining vfs to pods.

github-actions · 2023-05-08T14:29:18Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2023-05-08T14:36:10Z

Pull Request Test Coverage Report for Build 5753402663

77 of 444 (17.34%) changed or added relevant lines in 13 files are covered.
12 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.1%) to 24.537%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/daemon/plugin.go	0	2	0.0%
pkg/systemd/systemd.go	0	2	0.0%
pkg/webhook/validate.go	17	23	73.91%
api/v1/helper.go	0	7	0.0%
pkg/daemon/writer.go	0	9	0.0%
pkg/daemon/daemon.go	21	32	65.63%
pkg/plugins/mellanox/mellanox_plugin.go	0	12	0.0%
pkg/plugins/generic/generic_plugin.go	8	23	34.78%
pkg/utils/mock/mock_store.go	5	40	12.5%
pkg/utils/utils.go	1	54	1.85%

Files with Coverage Reduction	New Missed Lines	%
pkg/utils/utils.go	2	6.1%
pkg/daemon/daemon.go	3	41.03%
pkg/host/mock/mock_host.go	7	2.99%

Totals
Change from base Build 5740514141:	0.1%
Covered Lines:	2145
Relevant Lines:	8742

💛 - Coveralls

github-actions · 2023-05-09T08:54:50Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-05-09T11:01:30Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke

Overall, PR looks good to me. I added a few comments error messages

pkg/utils/utils.go

pkg/plugins/generic/generic_plugin.go

github-actions · 2023-05-10T13:24:47Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke · 2023-05-10T15:03:02Z

LGTM

github-actions · 2023-05-11T11:37:07Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-06-01T09:29:00Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-06-01T09:50:41Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris · 2023-06-13T12:33:26Z

api/v1/sriovnetworknodepolicy_types.go

@@ -59,6 +59,8 @@ type SriovNetworkNodePolicySpec struct {
 	VdpaType string `json:"vdpaType,omitempty"`
 	// Exclude device's NUMA node when advertising this resource by SRIOV network device plugin. Default to false.
 	ExcludeTopology bool `json:"excludeTopology,omitempty"`
+	// don't create the virtual function only allocated them to the device plugin. Defaults to false.
+	ExternallyCreated bool `json:"externallyCreated,omitempty"`


how does ExternallyCreated relate to other parameters here ?

we need to have this documented so users will know what to expect when using it.

I add documentation for it let me know if you need anything else :)

is there a reason to do it per interface and not global?
Do you have use-case when some SR-IOV will be create by SR-IOV opeartor and some not?

I read it now in the description. You want to use it for CNI primary network and for secondary VF will be managed by sriov operator

api/v1/sriovnetworknodestate_types.go

github-actions · 2023-06-14T14:09:09Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-06-19T09:49:04Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-06-19T10:06:19Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris · 2023-07-02T15:17:45Z

pkg/webhook/validate.go

+
+	// Externally created: we don't support  ExternallyCreated + EswitchMode
+	//TODO: if needed we will need to add this in the future as today EswitchMode is for HWOFFLOAD
+	if cr.Spec.ExternallyCreated && cr.Spec.EswitchMode == sriovnetworkv1.ESwithModeSwitchDev {


why do you want to block it ?

if its externally created and device is already in switchdev mode then its OK no ?

we should block right now because all the systemd and bash script we do for this will be hard to manage.
and right know I am not aware of any request for this.

adrianchiris · 2023-07-03T07:40:54Z

pkg/utils/host.go

+	_, err = os.Stat(PfAppliedConfig)
+	if err != nil {
+		if os.IsNotExist(err) {
+			err = os.Mkdir(PfAppliedConfig, os.ModeDir)


you can call MkdirAll[1] on PfAppliedConfg

[1[ https://pkg.go.dev/os#MkdirAll

adrianchiris

starting to wonder if we need a design doc for this one.

it feels that we need to get an agreement on what exactly ExternallyManaged means
then outline the changes in the different flows of sriov-network-operator.

that way:

we agree on what we want to achieve
make sure this PR is doing what we want

adrianchiris · 2023-07-03T07:43:28Z

pkg/utils/host.go

+	return nil
+}
+
+func ClearPCIAddressFolder() error {


nit: add docsting

adrianchiris · 2023-07-03T07:43:32Z

pkg/utils/host.go

+	return nil
+}
+
+func CreatePfAppliedStatusFromSpec(p *sriovnetworkv1.Interface) *PfStatus {


nit: add docsting

why not save/load sriovnetworkv1.Interface ?

i just wonder if we really need this intermediate struct.

also it would be nice if all these methods are bunched together in an interface.

essentially you are storing and loading information of a PF.
this "Store" once created can initialize its storage path.

adrianchiris · 2023-07-03T07:53:45Z

pkg/utils/host.go

+// returns false if the file doesn't exist.
+func LoadPfsStatus(pciAddress string, chroot bool) (*PfStatus, bool, error) {
+	if chroot {
+		exit, err := Chroot("/host")


why do we need to chroot ?

A: Because SyncNodeState is called with chroot

adrianchiris · 2023-07-03T08:19:29Z

pkg/daemon/daemon.go

+		return err
+	}
+	// we only update the status to avoid any race conditions where a new policy can be applied
+	latestState.Status = updatedState.Status


This is all starting to get complex.

can you move all of this "prep" logic to its own function and call it here ?
please keep comments so we understand why things are there .

I agree but I will like to do it in a followup PR if possible

adrianchiris · 2023-07-03T08:21:26Z

pkg/daemon/daemon.go

@@ -452,6 +458,15 @@ func (dn *Daemon) nodeStateSyncHandler() error {
 		syncStatus:    syncStatusInProgress,
 		lastSyncError: "",
 	}
+	// wait for writer to refresh status then pull again the latest node state
+	<-dn.syncCh
+	updatedState, err := dn.client.SriovnetworkV1().SriovNetworkNodeStates(namespace).Get(context.Background(), dn.name, metav1.GetOptions{})


why not just overwrite latestState here ?

because we may get a new spec that we don't expect.

adrianchiris · 2023-07-03T08:24:20Z

pkg/daemon/daemon.go

+		glog.Warningf("nodeStateSyncHandler(): Failed to fetch node state %s: %v", dn.name, err)
+		return err
+	}
+	// we only update the status to avoid any race conditions where a new policy can be applied


if generation changed from when we first got it. just fail WDYT ?

im not a fan of starting to patch the object

sure we can go with that

README.md

adrianchiris · 2023-07-03T09:00:55Z

pkg/utils/utils.go

@@ -270,6 +319,12 @@ func NeedUpdate(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetworkv1.Int
 							glog.V(2).Infof("NeedUpdate(): VF %d MTU needs update, desired=%d, current=%d", vf.VfID, group.Mtu, vf.Mtu)
 							return true
 						}
+
+						// this is needed to be sure the admin mac address is configured as expected


why can the admin mac be "externally configured" ? that should be part of sriov configuration performed by the external service.

we need this to be sure we maintain the same capability as before.

this means for nics that don't configure the admin mac address the operator configures them when externally manage is not requested.
so we need to have the same for externally manage true

adrianchiris · 2023-07-03T09:15:44Z

pkg/utils/utils.go

+				return err
+			}
+
+			if !exist {


if entry was not found, does this nessecarily mean that VFs were not created by sriov-operator ?

yes because we always going to create the PF info

adrianchiris · 2023-07-03T09:27:00Z

also UT need to be added where possible.

github-actions · 2023-07-03T12:15:05Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2023-09-04T13:47:30Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

zeeke

Left some nit. It would be great if we could make some unit test around daemon/generic-plugin logic

pkg/utils/store.go

Signed-off-by: Sebastian Sch <[email protected]>

github-actions · 2023-09-11T11:33:01Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

Signed-off-by: Sebastian Sch <[email protected]>

Signed-off-by: Sebastian Sch <[email protected]> WIP Signed-off-by: Sebastian Sch <[email protected]>

Signed-off-by: Sebastian Sch <[email protected]>

github-actions · 2023-09-12T15:42:41Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

SchSeba · 2023-09-12T15:44:29Z

Left some nit. It would be great if we could make some unit test around daemon/generic-plugin logic

I am working on a separate PR after we move the drain to a controller a refactor in the daemon that will use more interfaces so we can mock easily

zeeke

Left a few minor comments. Overall shape looks good to me.

pkg/webhook/validate.go

pkg/utils/utils.go

pkg/utils/store.go

github-actions · 2023-09-26T15:10:30Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

SchSeba · 2023-09-26T15:12:32Z

Hi @zeeke let me know if the changes are good and I will rebase the commit messages

zeeke

LGTM
Thanks for addressing my comments

pkg/utils/utils.go

Signed-off-by: Sebastian Sch <[email protected]>

github-actions · 2023-09-27T15:23:41Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

e0ne

LGTM. Thanks for addressing my comments

SchSeba · 2023-10-01T17:23:47Z

Thanks folks!

Merging after 2 maintainers approval

SchSeba force-pushed the externally_manage branch from a9bb702 to af05d3b Compare May 9, 2023 08:54

SchSeba force-pushed the externally_manage branch from af05d3b to df85137 Compare May 9, 2023 11:01

zeeke reviewed May 9, 2023

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

pkg/plugins/generic/generic_plugin.go Outdated Show resolved Hide resolved

SchSeba force-pushed the externally_manage branch from df85137 to 5cf1a83 Compare May 10, 2023 13:24

SchSeba force-pushed the externally_manage branch from 5cf1a83 to aed3c7b Compare May 11, 2023 11:36

adrianchiris mentioned this pull request May 31, 2023

Release SRIOV Network Operator V1.3.0 #450

Closed

6 tasks

SchSeba force-pushed the externally_manage branch from aed3c7b to a2c3ae9 Compare June 1, 2023 09:28

SchSeba force-pushed the externally_manage branch from a2c3ae9 to 294054c Compare June 1, 2023 09:50

adrianchiris reviewed Jun 13, 2023

View reviewed changes

api/v1/sriovnetworknodestate_types.go Show resolved Hide resolved

SchSeba force-pushed the externally_manage branch from 294054c to 4e5a5e0 Compare June 14, 2023 14:08

SchSeba mentioned this pull request Jun 19, 2023

Externally manage e2e tests #460

Closed

adrianchiris reviewed Jul 2, 2023

View reviewed changes

adrianchiris reviewed Jul 3, 2023

View reviewed changes

SchSeba force-pushed the externally_manage branch from 13835b6 to b6f321d Compare July 3, 2023 12:14

zeeke reviewed Sep 5, 2023

View reviewed changes

pkg/utils/store.go Outdated Show resolved Hide resolved

pkg/utils/store.go Show resolved Hide resolved

SchSeba added 3 commits September 11, 2023 12:05

externally managed api

bc4b371

Signed-off-by: Sebastian Sch <[email protected]>

documentation update

55ae84d

Signed-off-by: Sebastian Sch <[email protected]>

validation webhook update

455951e

Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the externally_manage branch from 45bb99c to e80a4e1 Compare September 11, 2023 11:32

SchSeba added 9 commits September 12, 2023 18:42

new store package to save per pci policy info

cf4a728

Signed-off-by: Sebastian Sch <[email protected]>

update host package and fix mocks

5a74fc3

Signed-off-by: Sebastian Sch <[email protected]>

update utils and systemd

bf6cecd

Signed-off-by: Sebastian Sch <[email protected]>

add externally manage to plugins

8ada397

Signed-off-by: Sebastian Sch <[email protected]> WIP Signed-off-by: Sebastian Sch <[email protected]>

update daemon to support new externally managed field

241f4d7

Signed-off-by: Sebastian Sch <[email protected]>

copy crds to deployment

3ebe5f6

Signed-off-by: Sebastian Sch <[email protected]>

update systemd service main go with new store and host interfaces

7edb9b8

Signed-off-by: Sebastian Sch <[email protected]>

functional tests

aeb60e2

Signed-off-by: Sebastian Sch <[email protected]>

improve udev rules to only disable nm on operator managed devices

835f5ac

Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the externally_manage branch from e80a4e1 to 835f5ac Compare September 12, 2023 15:42

zeeke reviewed Sep 14, 2023

View reviewed changes

pkg/webhook/validate.go Outdated Show resolved Hide resolved

pkg/utils/utils.go Outdated Show resolved Hide resolved

pkg/utils/store.go Show resolved Hide resolved

pkg/utils/store.go Outdated Show resolved Hide resolved

pkg/utils/store.go Outdated Show resolved Hide resolved

zeeke approved these changes Sep 26, 2023

View reviewed changes

e0ne reviewed Sep 27, 2023

View reviewed changes

pkg/utils/utils.go Show resolved Hide resolved

pkg/utils/utils.go Outdated Show resolved Hide resolved

Fix comments

53cfd3d

Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the externally_manage branch from 0399543 to 53cfd3d Compare September 27, 2023 15:23

e0ne approved these changes Sep 28, 2023

View reviewed changes

SchSeba merged commit d8a33a8 into k8snetworkplumbingwg:master Oct 1, 2023
9 checks passed

Support Externally Created virtual functions #436

Support Externally Created virtual functions #436

Conversation

SchSeba commented May 8, 2023

github-actions bot commented May 8, 2023

coveralls commented May 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5753402663

💛 - Coveralls

github-actions bot commented May 9, 2023

github-actions bot commented May 9, 2023

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented May 10, 2023

zeeke commented May 10, 2023

github-actions bot commented May 11, 2023

github-actions bot commented Jun 1, 2023

github-actions bot commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe010 Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 19, 2023

github-actions bot commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Jul 3, 2023

github-actions bot commented Jul 3, 2023

github-actions bot commented Sep 4, 2023

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 12, 2023

SchSeba commented Sep 12, 2023

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2023

SchSeba commented Sep 26, 2023

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2023

e0ne left a comment

Choose a reason for hiding this comment

SchSeba commented Oct 1, 2023

coveralls commented May 8, 2023 •

edited

Loading

moshe010 Jun 29, 2023 •

edited

Loading