Add disable plugins #569

adrianchiris · 2023-12-31T14:35:16Z

Add option to disable sriov network config daemon plugins

some plugins perform vendor specific firmware configuration
to enable SR-IOV (e.g mellanox plugin). certain deployment environments may prefer to perform such configuration
once during node provisioning, while ensuring the configuration will be compatible with any sriov network node policy
defined for the particular environment. This will reduce or completely eliminate the need for reboot of nodes during SR-IOV
configurations by the operator.

This can be done by setting SriovOperatorConfig default CR spec.disablePlugins with the list of desired plugins
to disable.

github-actions · 2023-12-31T14:35:27Z

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-12-31T14:41:00Z

Pull Request Test Coverage Report for Build 7744576897

-64 of 124 (48.39%) changed or added relevant lines in 11 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.9%) to 25.912%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/plugins/intel/intel_plugin.go	0	2	0.0%
pkg/daemon/daemon.go	6	10	60.0%
pkg/plugins/mellanox/mellanox_plugin.go	0	4	0.0%
pkg/daemon/plugin.go	34	39	87.18%
pkg/plugins/virtual/virtual_plugin.go	0	6	0.0%
pkg/plugins/k8s/k8s_plugin.go	0	9	0.0%
api/v1/zz_generated.deepcopy.go	5	18	27.78%
pkg/plugins/generic/generic_plugin.go	4	25	16.0%

Files with Coverage Reduction	New Missed Lines	%
controllers/sriovnetworkpoolconfig_controller.go	6	69.7%

Totals
Change from base Build 7742637229:	0.9%
Covered Lines:	2913
Relevant Lines:	11242

💛 - Coveralls

SchSeba · 2024-01-01T11:49:21Z

general question do we want to have this as part of SriovOperatorConfig CR instead of another env variable in the operator? :)

adrianchiris · 2024-01-01T11:54:14Z

general question do we want to have this as part of SriovOperatorConfig CR instead of another env variable in the operator?

im not sure, i see this decision (deciding if/which plugins to disable) is made during deployment time and should not change.

moreover deleting sriov operator config CR will trigger creation of the default config which will have no plugins disabled by default which may be problematic.

maybe we need to revisit the creation of default sriov operator config CR ?

SchSeba · 2024-01-01T12:06:38Z

I am not fully familiar with helm but can't we request a CR also when we do the helm install?

bn222 · 2024-01-03T07:47:07Z

im not sure, i see this decision (deciding if/which plugins to disable) is made during deployment time and should not change.

Even if the decision is not made, I also prefer to have it be part of the CR since that would cover more use cases. It feels cleaner.

adrianchiris · 2024-01-03T10:31:19Z

I also prefer to have it be part of the CR since that would cover more use cases.

what other use-cases ?

bn222 · 2024-01-03T10:35:13Z

I also prefer to have it be part of the CR since that would cover more use cases.

what other use-cases ?

Suppose the customer decides that they want to disable some plugin, then at a later point in time, they change their mind.

adrianchiris · 2024-01-03T10:40:19Z

Suppose the customer decides that they want to disable some plugin, then at a later point in time, they change their mind.

then IMO they need to redeploy. our (NVIDIA) use case that it should not change, depends on specific datacenter configuration, moreover accidently changing it may have catastrophic effect to workers (firmware parameters resetting).

my concern is creating the default operator config obj in the operator. if you are ok with dropping its creation, we can set this in operator config CR.

we discussed this in community meeting. Sebastian needs to double check internally.

mlguerrero12 · 2024-01-03T12:55:35Z

cmd/sriov-network-config-daemon/start.go

+	allowedDisablePlugins := map[string]struct{}{"mellanox": {}}
+	for _, p := range startOpts.disabledPlugins {
+		if _, ok := allowedDisablePlugins[p]; !ok {
+			return fmt.Errorf("%s plugin is cannot be disabled", p)


nit: remove is

mlguerrero12 · 2024-01-03T13:23:37Z

deployment/sriov-network-operator/README.md

+
+| Name | Type | Default | description |
+| ---- | ---- | ------- | ----------- |
+| `sriovConfigDaemon.disablePlugins` | []string | `nil` | List of plugins to disable, currently only `mellanox` plugin is allowed |


shouldn't we add "use-systemd" now that you're adding this?

im not sure, that one can be configured during runtime.
this one is during deployment time.

Right, didn't notice that this readme is for the helm chart

mlguerrero12 · 2024-01-03T13:27:42Z

deployment/sriov-network-operator/values.yaml

+#sriovConfigDaemon:
+#  disablePlugins:
+#    - mellanox
+#    - intel


remove intel?

deployment/sriov-network-operator/templates/operator.yaml

pkg/daemon/plugin.go

Eoghan1232 · 2024-01-08T12:50:42Z

cmd/sriov-network-config-daemon/start.go

@@ -43,6 +43,29 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/version"
 )

+// stringList is a list of strings, implements pflag.Vaue interface


Suggested change

// stringList is a list of strings, implements pflag.Vaue interface

// stringList is a list of strings, implements pflag.Value interface

addressed will update PR soon

Eoghan1232 · 2024-01-08T12:55:58Z

cmd/sriov-network-config-daemon/start.go

@@ -81,6 +106,14 @@ func runStartCmd(cmd *cobra.Command, args []string) error {
 		startOpts.nodeName = name
 	}

+	// TODO(adrianc): move allowed disable plugins to globals pkg
+	allowedDisablePlugins := map[string]struct{}{"mellanox": {}}


allowedDisablePlugins -> DisableablePlugins

WDYT?

What about deactivatablePlugins or activatablePlugins if we switch to a positive instead (which is maybe better to avoid double negatives and have shorter names throughout).

addressed, will upload PR soon

github-actions · 2024-01-10T12:11:16Z

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 · 2024-01-12T15:17:13Z

README.md

+configurations by the operator.
+
+This can be done by providing the names of plugins to disable as a comma separated list of strings via
+`SRIOV_NETWORK_CONFIG_DAEMON_DISABLE_PLUGINS` environment variable to the operator deployment or using


nit: I don't know what will be better, maybe _DISABLED_PLUGINS

i prefer to keep it the same name as the flag (disable-plugins)

e0ne · 2024-01-12T15:18:58Z

README.md

+defined for the particular environment. This will reduce or completely eliminate the need for reboot of nodes during SR-IOV
+configurations by the operator.
+
+This can be done by providing the names of plugins to disable as a comma separated list of strings via


nit: "comma-separated" here and below

e0ne · 2024-01-12T15:19:12Z

cmd/sriov-network-config-daemon/start.go

@@ -66,6 +90,7 @@ func init() {
 	startCmd.PersistentFlags().StringVar(&startOpts.kubeconfig, "kubeconfig", "", "Kubeconfig file to access a remote cluster (testing only)")
 	startCmd.PersistentFlags().StringVar(&startOpts.nodeName, "node-name", "", "kubernetes node name daemon is managing")
 	startCmd.PersistentFlags().BoolVar(&startOpts.systemd, "use-systemd-service", false, "use config daemon in systemd mode")
+	startCmd.PersistentFlags().VarP(&startOpts.disabledPlugins, "disable-plugins", "", "comma separated list of plugins")


comma separated => comma-separated

e0ne · 2024-01-12T15:23:57Z

my concern is creating the default operator config obj in the operator. if you are ok with dropping its creation, we can set this in operator config CR.

+1 on this. this option isn't supposed to be changed during runtime

github-actions · 2024-01-28T19:05: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.

adrianchiris · 2024-01-29T13:27:38Z

@SchSeba PTAL

mlguerrero12 · 2024-01-30T12:59:00Z

please rebase

LGTM

zeeke · 2024-01-30T13:14:00Z

pkg/daemon/plugin_test.go

+		prevK8sPluginFn := K8sPlugin
+
+		BeforeEach(func() {
+			vars.ClusterType = consts.ClusterTypeKubernetes


nit: can you reset global variables to its origin value?

Suggested change

vars.ClusterType = consts.ClusterTypeKubernetes

backupClusterType := vars.ClusterType

vars.ClusterType = consts.ClusterTypeKubernetes

DeferCleanup(func() {

vars.ClusterType = backupClusterType

})

mlguerrero12 · 2024-01-30T13:15:16Z

api/v1/sriovoperatorconfig_types.go

@@ -45,6 +45,8 @@ type SriovOperatorConfigSpec struct {
 	ConfigurationMode ConfigurationModeType `json:"configurationMode,omitempty"`
 	// Flag to enable Container Device Interface mode for SR-IOV Network Device Plugin
 	UseCDI bool `json:"useCDI,omitempty"`
+	// DisablePlugins is a list of sriov-network-config-daemon plugins to disable
+	DisablePlugins []string `json:"disablePlugins,omitempty"`


you can add a validation at the api level

// +kubebuilder:validation:Enum={"mellanox"}

tried this out, it doesnt work for lists just for scalar values

spec.disablePlugins: Unsupported value: []interface {}{"mellanox"}: supported values: "mellanox"

I see, perhaps this kubernetes-sigs/kubebuilder#2812?

works. PR updated.

github-actions · 2024-01-30T14:42:58Z

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 · 2024-01-30T15:24:02Z

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 · 2024-01-30T15:28:08Z

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.

mlguerrero12

LGTM

Failing tests are not related. They are also failing in other PRs.
This seems to help #600

github-actions · 2024-01-31T15:05:58Z

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 · 2024-01-31T15:07:08Z

rebased. @zeeke PTAL :)

- remove _plugin suffix from plugin names as it doesnt add any important information. - rename logs Signed-off-by: adrianc <adrianc@nvidia.com>

This better reflects the meaning of whats being done. In addition it serves as prep work to support disable plugins as having both enabledPlugins and disabledPlugins attr in Daemon is confusing. Signed-off-by: adrianc <adrianc@nvidia.com>

- Add --disable-plugins CLI flag - parse and check flag values and pass to daemon - extend plugin load logic to skip disabled plugins Signed-off-by: adrianc <adrianc@nvidia.com>

- Make fake plugin name configurable via attribute - adjust daemon test code - add daemon plugins tests to cover plugin loading logic Signed-off-by: adrianc <adrianc@nvidia.com>

- Extend sriovOperatorConfig CRD with disablePlugins list attribute which contains list of plugins to disable in sriov-network-config-daemon - sriovOperatorConfig controller: Update template data - Set disable-plugin flag in daemonset yaml - Add tests Signed-off-by: adrianc <adrianc@nvidia.com>

update documentation for disable plugin feature Signed-off-by: adrianc <adrianc@nvidia.com>

github-actions · 2024-02-01T16: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.

zeeke

LGTM

Changes to CustomResourceDefinition have been introduced by - k8snetworkplumbingwg/sriov-network-operator#497 - k8snetworkplumbingwg/sriov-network-operator#569 Signed-off-by: Andrea Panattoni <apanatto@redhat.com>

adrianchiris requested review from SchSeba and Eoghan1232 December 31, 2023 14:35

mlguerrero12 reviewed Jan 3, 2024

View reviewed changes

Eoghan1232 reviewed Jan 8, 2024

View reviewed changes

adrianchiris force-pushed the add-disable-plugins branch from 44a77c7 to 528b0c0 Compare January 10, 2024 12:11

e0ne reviewed Jan 12, 2024

View reviewed changes

adrianchiris force-pushed the add-disable-plugins branch from 528b0c0 to 59ddaa9 Compare January 28, 2024 19:05

adrianchiris requested a review from mlguerrero12 January 30, 2024 08:38

zeeke reviewed Jan 30, 2024

View reviewed changes

mlguerrero12 reviewed Jan 30, 2024

View reviewed changes

adrianchiris force-pushed the add-disable-plugins branch from 59ddaa9 to d1fdcaf Compare January 30, 2024 14:42

adrianchiris force-pushed the add-disable-plugins branch from d1fdcaf to cb0cd55 Compare January 30, 2024 15:23

adrianchiris force-pushed the add-disable-plugins branch from cb0cd55 to f0637f6 Compare January 30, 2024 15:27

mlguerrero12 approved these changes Jan 30, 2024

View reviewed changes

adrianchiris force-pushed the add-disable-plugins branch from f0637f6 to f279398 Compare January 31, 2024 15:05

adrianchiris added 6 commits February 1, 2024 18:21

Rename plugin names

9162bf5

- remove _plugin suffix from plugin names as it doesnt add any important information. - rename logs Signed-off-by: adrianc <adrianc@nvidia.com>

Add disable plugins to config daemon

45884f7

- Add --disable-plugins CLI flag - parse and check flag values and pass to daemon - extend plugin load logic to skip disabled plugins Signed-off-by: adrianc <adrianc@nvidia.com>

Add unit-tests for daemon plugin

65530fe

- Make fake plugin name configurable via attribute - adjust daemon test code - add daemon plugins tests to cover plugin loading logic Signed-off-by: adrianc <adrianc@nvidia.com>

doc updates

baa18d0

update documentation for disable plugin feature Signed-off-by: adrianc <adrianc@nvidia.com>

adrianchiris force-pushed the add-disable-plugins branch from f279398 to baa18d0 Compare February 1, 2024 16:36

zeeke approved these changes Feb 1, 2024

View reviewed changes

zeeke merged commit e409337 into k8snetworkplumbingwg:master Feb 2, 2024
11 checks passed

zeeke mentioned this pull request Feb 22, 2024

OCPBUGS-25986: Downstream sync 2024-02-20 openshift/sriov-network-operator#887

Merged

adrianchiris deleted the add-disable-plugins branch July 16, 2024 11:12

	// stringList is a list of strings, implements pflag.Vaue interface
	// stringList is a list of strings, implements pflag.Value interface

-			vars.ClusterType = consts.ClusterTypeKubernetes
+			backupClusterType := vars.ClusterType
+			vars.ClusterType = consts.ClusterTypeKubernetes
+			DeferCleanup(func() {
+						vars.ClusterType = backupClusterType
+			})

Add disable plugins #569

Add disable plugins #569

Conversation

adrianchiris commented Dec 31, 2023 • edited Loading

github-actions bot commented Dec 31, 2023

coveralls commented Dec 31, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7744576897

💛 - Coveralls

SchSeba commented Jan 1, 2024

adrianchiris commented Jan 1, 2024 • edited Loading

SchSeba commented Jan 1, 2024

bn222 commented Jan 3, 2024

adrianchiris commented Jan 3, 2024

bn222 commented Jan 3, 2024

adrianchiris commented Jan 3, 2024 • edited Loading

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

github-actions bot commented Jan 10, 2024

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

e0ne commented Jan 12, 2024

github-actions bot commented Jan 28, 2024

adrianchiris commented Jan 29, 2024

mlguerrero12 commented Jan 30, 2024

Choose a reason for hiding this comment

adrianchiris Jan 30, 2024 • edited Loading

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

github-actions bot commented Jan 30, 2024

github-actions bot commented Jan 30, 2024

github-actions bot commented Jan 30, 2024

mlguerrero12 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024

adrianchiris commented Jan 31, 2024

github-actions bot commented Feb 1, 2024

zeeke left a comment

Choose a reason for hiding this comment

adrianchiris commented Dec 31, 2023 •

edited

Loading

coveralls commented Dec 31, 2023 •

edited

Loading

adrianchiris commented Jan 1, 2024 •

edited

Loading

adrianchiris commented Jan 3, 2024 •

edited

Loading

adrianchiris Jan 30, 2024 •

edited

Loading