From dc96c2c9e0695fa26ec41b26ce8b3985f52d18e6 Mon Sep 17 00:00:00 2001 From: Tobias Giese Date: Mon, 15 Jul 2024 16:27:22 +0200 Subject: [PATCH] chore: add spec.ConfigDaemonEnvVars to the SriovOperatorConfig Signed-off-by: Tobias Giese --- api/v1/sriovoperatorconfig_types.go | 3 + api/v1/zz_generated.deepcopy.go | 8 ++ bindata/manifests/daemon/daemonset.yaml | 3 + cmd/sriov-network-config-daemon/start.go | 4 + ...ork.openshift.io_sriovoperatorconfigs.yaml | 113 ++++++++++++++++++ controllers/sriovoperatorconfig_controller.go | 14 +++ .../sriovoperatorconfig_controller_test.go | 27 +++++ ...ork.openshift.io_sriovoperatorconfigs.yaml | 113 ++++++++++++++++++ go.mod | 2 +- pkg/plugins/mellanox/mellanox_plugin.go | 16 ++- pkg/vars/vars.go | 3 + 11 files changed, 299 insertions(+), 7 deletions(-) diff --git a/api/v1/sriovoperatorconfig_types.go b/api/v1/sriovoperatorconfig_types.go index 5e2e011f8..d3c29d929 100644 --- a/api/v1/sriovoperatorconfig_types.go +++ b/api/v1/sriovoperatorconfig_types.go @@ -17,6 +17,7 @@ limitations under the License. package v1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -65,6 +66,8 @@ type SriovOperatorConfigSpec struct { DisablePlugins PluginNameSlice `json:"disablePlugins,omitempty"` // FeatureGates to enable experimental features FeatureGates map[string]bool `json:"featureGates,omitempty"` + // ConfigDaemonEnvVars specifies a set of environment variables to be passed to the sriov-network-config-daemon. + ConfigDaemonEnvVars []corev1.EnvVar `json:"configDaemonEnvVars,omitempty"` } // SriovOperatorConfigStatus defines the observed state of SriovOperatorConfig diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index fc9477593..7f44c580a 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -21,6 +21,7 @@ limitations under the License. package v1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" @@ -1039,6 +1040,13 @@ func (in *SriovOperatorConfigSpec) DeepCopyInto(out *SriovOperatorConfigSpec) { (*out)[key] = val } } + if in.ConfigDaemonEnvVars != nil { + in, out := &in.ConfigDaemonEnvVars, &out.ConfigDaemonEnvVars + *out = make([]corev1.EnvVar, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SriovOperatorConfigSpec. diff --git a/bindata/manifests/daemon/daemonset.yaml b/bindata/manifests/daemon/daemonset.yaml index 1c3d7d618..d9d743ac8 100644 --- a/bindata/manifests/daemon/daemonset.yaml +++ b/bindata/manifests/daemon/daemonset.yaml @@ -157,6 +157,9 @@ spec: value: "{{.ClusterType}}" - name: DEV_MODE value: "{{.DevMode}}" + {{- if .ConfigDaemonEnvVars }} + {{ .ConfigDaemonEnvVars | nindent 10 }} + {{- end }} resources: requests: cpu: 100m diff --git a/cmd/sriov-network-config-daemon/start.go b/cmd/sriov-network-config-daemon/start.go index 4311db11b..d3b86e2f8 100644 --- a/cmd/sriov-network-config-daemon/start.go +++ b/cmd/sriov-network-config-daemon/start.go @@ -109,6 +109,10 @@ func runStartCmd(cmd *cobra.Command, args []string) error { vars.ParallelNicConfig = startOpts.parallelNicConfig + if strings.ToLower(os.Getenv("MLX_PLUGIN_FW_RESET")) == "true" { + vars.MlxPluginFwReset = true + } + if startOpts.nodeName == "" { name, ok := os.LookupEnv("NODE_NAME") if !ok || name == "" { diff --git a/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml b/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml index 5d944910d..a0d571d52 100644 --- a/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml +++ b/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml @@ -40,6 +40,119 @@ spec: spec: description: SriovOperatorConfigSpec defines the desired state of SriovOperatorConfig properties: + configDaemonEnvVars: + description: ConfigDaemonEnvVars specifies a set of environment variables + to be passed to the sriov-network-config-daemon. + items: + description: EnvVar represents an environment variable present in + a Container. + properties: + name: + description: Name of the environment variable. Must be a C_IDENTIFIER. + type: string + value: + description: |- + Variable references $(VAR_NAME) are expanded + using the previously defined environment variables in the container and + any service environment variables. If a variable cannot be resolved, + the reference in the input string will be unchanged. Double $$ are reduced + to a single $, which allows for escaping the $(VAR_NAME) syntax: i.e. + "$$(VAR_NAME)" will produce the string literal "$(VAR_NAME)". + Escaped references will never be expanded, regardless of whether the variable + exists or not. + Defaults to "". + type: string + valueFrom: + description: Source for the environment variable's value. Cannot + be used if value is not empty. + properties: + configMapKeyRef: + description: Selects a key of a ConfigMap. + properties: + key: + description: The key to select. + type: string + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid? + type: string + optional: + description: Specify whether the ConfigMap or its key + must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic + fieldRef: + description: |- + Selects a field of the pod: supports metadata.name, metadata.namespace, `metadata.labels['']`, `metadata.annotations['']`, + spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP, status.podIPs. + properties: + apiVersion: + description: Version of the schema the FieldPath is + written in terms of, defaults to "v1". + type: string + fieldPath: + description: Path of the field to select in the specified + API version. + type: string + required: + - fieldPath + type: object + x-kubernetes-map-type: atomic + resourceFieldRef: + description: |- + Selects a resource of the container: only resources limits and requests + (limits.cpu, limits.memory, limits.ephemeral-storage, requests.cpu, requests.memory and requests.ephemeral-storage) are currently supported. + properties: + containerName: + description: 'Container name: required for volumes, + optional for env vars' + type: string + divisor: + anyOf: + - type: integer + - type: string + description: Specifies the output format of the exposed + resources, defaults to "1" + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + resource: + description: 'Required: resource to select' + type: string + required: + - resource + type: object + x-kubernetes-map-type: atomic + secretKeyRef: + description: Selects a key of a secret in the pod's namespace + properties: + key: + description: The key of the secret to select from. Must + be a valid secret key. + type: string + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid? + type: string + optional: + description: Specify whether the Secret or its key must + be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic + type: object + required: + - name + type: object + type: array configDaemonNodeSelector: additionalProperties: type: string diff --git a/controllers/sriovoperatorconfig_controller.go b/controllers/sriovoperatorconfig_controller.go index 11cfdfc36..0da38a12b 100644 --- a/controllers/sriovoperatorconfig_controller.go +++ b/controllers/sriovoperatorconfig_controller.go @@ -23,6 +23,7 @@ import ( "sort" "strings" + "gopkg.in/yaml.v2" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -190,6 +191,19 @@ func (r *SriovOperatorConfigReconciler) syncConfigDaemonSet(ctx context.Context, } data.Data["ParallelNicConfig"] = r.FeatureGate.IsEnabled(consts.ParallelNicConfigFeatureGate) + // If ConfigDaemonEnvVars has been passed we have to marshal it. + // The go-template will be able to print and indent it properly. + // As the Kubernetes API is validing the EnvVars before there should never be an error. + data.Data["ConfigDaemonEnvVars"] = "" + if dc.Spec.ConfigDaemonEnvVars != nil { + envVarsYaml, err := yaml.Marshal(dc.Spec.ConfigDaemonEnvVars) + if err != nil { + logger.Error(err, "Fail to marshal ConfigDaemonEnvVars") + return err + } + data.Data["ConfigDaemonEnvVars"] = string(envVarsYaml) + } + envCniBinPath := os.Getenv("SRIOV_CNI_BIN_PATH") if envCniBinPath == "" { data.Data["CNIBinPath"] = "/var/lib/cni/bin" diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index 0dcc985f2..dc838c688 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/controllers/sriovoperatorconfig_controller_test.go @@ -43,6 +43,7 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() { EnableOperatorWebhook: true, ConfigDaemonNodeSelector: map[string]string{}, LogLevel: 2, + ConfigDaemonEnvVars: []corev1.EnvVar{}, } Expect(k8sClient.Create(context.Background(), config)).Should(Succeed()) DeferCleanup(func() { @@ -475,5 +476,31 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() { To(Equal(expectedAffinity)) }, "3s", "1s").Should(Succeed()) }) + It("should be able to update the ConfigDaemonEnvVars for the sriov-network-config-daemon", func() { + By("adding env vars to the SriovOperatorConfig") + config := &sriovnetworkv1.SriovOperatorConfig{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "default"}, config)).NotTo(HaveOccurred()) + + config.Spec.ConfigDaemonEnvVars = []corev1.EnvVar{ + {Name: "MLX_PLUGIN_FW_RESET", Value: "True"}, + } + err := k8sClient.Update(ctx, config) + Expect(err).NotTo(HaveOccurred()) + + daemonSet := &appsv1.DaemonSet{} + Eventually(func() bool { + // By("wait for DaemonSet NodeSelector") + err := k8sClient.Get(ctx, types.NamespacedName{Name: "sriov-network-config-daemon", Namespace: testNamespace}, daemonSet) + if err != nil { + return false + } + for _, envVar := range daemonSet.Spec.Template.Spec.Containers[0].Env { + if envVar.Name == "MLX_PLUGIN_FW_RESET" && envVar.Value == "True" { + return true + } + } + return false + }, util.APITimeout, util.RetryInterval).Should(BeTrue()) + }) }) }) diff --git a/deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml b/deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml index 5d944910d..a0d571d52 100644 --- a/deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml +++ b/deployment/sriov-network-operator-chart/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml @@ -40,6 +40,119 @@ spec: spec: description: SriovOperatorConfigSpec defines the desired state of SriovOperatorConfig properties: + configDaemonEnvVars: + description: ConfigDaemonEnvVars specifies a set of environment variables + to be passed to the sriov-network-config-daemon. + items: + description: EnvVar represents an environment variable present in + a Container. + properties: + name: + description: Name of the environment variable. Must be a C_IDENTIFIER. + type: string + value: + description: |- + Variable references $(VAR_NAME) are expanded + using the previously defined environment variables in the container and + any service environment variables. If a variable cannot be resolved, + the reference in the input string will be unchanged. Double $$ are reduced + to a single $, which allows for escaping the $(VAR_NAME) syntax: i.e. + "$$(VAR_NAME)" will produce the string literal "$(VAR_NAME)". + Escaped references will never be expanded, regardless of whether the variable + exists or not. + Defaults to "". + type: string + valueFrom: + description: Source for the environment variable's value. Cannot + be used if value is not empty. + properties: + configMapKeyRef: + description: Selects a key of a ConfigMap. + properties: + key: + description: The key to select. + type: string + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid? + type: string + optional: + description: Specify whether the ConfigMap or its key + must be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic + fieldRef: + description: |- + Selects a field of the pod: supports metadata.name, metadata.namespace, `metadata.labels['']`, `metadata.annotations['']`, + spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP, status.podIPs. + properties: + apiVersion: + description: Version of the schema the FieldPath is + written in terms of, defaults to "v1". + type: string + fieldPath: + description: Path of the field to select in the specified + API version. + type: string + required: + - fieldPath + type: object + x-kubernetes-map-type: atomic + resourceFieldRef: + description: |- + Selects a resource of the container: only resources limits and requests + (limits.cpu, limits.memory, limits.ephemeral-storage, requests.cpu, requests.memory and requests.ephemeral-storage) are currently supported. + properties: + containerName: + description: 'Container name: required for volumes, + optional for env vars' + type: string + divisor: + anyOf: + - type: integer + - type: string + description: Specifies the output format of the exposed + resources, defaults to "1" + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + resource: + description: 'Required: resource to select' + type: string + required: + - resource + type: object + x-kubernetes-map-type: atomic + secretKeyRef: + description: Selects a key of a secret in the pod's namespace + properties: + key: + description: The key of the secret to select from. Must + be a valid secret key. + type: string + name: + description: |- + Name of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid? + type: string + optional: + description: Specify whether the Secret or its key must + be defined + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic + type: object + required: + - name + type: object + type: array configDaemonNodeSelector: additionalProperties: type: string diff --git a/go.mod b/go.mod index a74fd1ddb..4ddac2388 100644 --- a/go.mod +++ b/go.mod @@ -35,6 +35,7 @@ require ( github.com/vishvananda/netns v0.0.4 go.uber.org/zap v1.25.0 golang.org/x/time v0.3.0 + gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.28.3 k8s.io/apiextensions-apiserver v0.28.3 @@ -151,7 +152,6 @@ require ( google.golang.org/grpc v1.56.3 // indirect google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect - gopkg.in/yaml.v2 v2.4.0 // indirect howett.net/plist v1.0.0 // indirect k8s.io/apiserver v0.28.3 // indirect k8s.io/cli-runtime v0.28.3 // indirect diff --git a/pkg/plugins/mellanox/mellanox_plugin.go b/pkg/plugins/mellanox/mellanox_plugin.go index c7ce83fbf..3a7ab1a08 100644 --- a/pkg/plugins/mellanox/mellanox_plugin.go +++ b/pkg/plugins/mellanox/mellanox_plugin.go @@ -8,6 +8,7 @@ import ( sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/helper" plugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" mlx "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vendors/mellanox" ) @@ -19,7 +20,7 @@ type MellanoxPlugin struct { helpers helper.HostHelpersInterface } -var pciAddressesToChange []string +var pciAddressesToReset []string var attributesToChange map[string]mlx.MlxNic var mellanoxNicsStatus map[string]map[string]sriovnetworkv1.InterfaceExt var mellanoxNicsSpec map[string]sriovnetworkv1.Interface @@ -52,7 +53,7 @@ func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeS needDrain = false needReboot = false err = nil - pciAddressesToChange = []string{} + pciAddressesToReset = []string{} attributesToChange = map[string]mlx.MlxNic{} mellanoxNicsStatus = map[string]map[string]sriovnetworkv1.InterfaceExt{} mellanoxNicsSpec = map[string]sriovnetworkv1.Interface{} @@ -135,7 +136,7 @@ func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeS } if needReboot { - pciAddressesToChange = append(pciAddressesToChange, ifaceSpec.PciAddress) + pciAddressesToReset = append(pciAddressesToReset, ifaceSpec.PciAddress) } } @@ -195,9 +196,12 @@ func (p *MellanoxPlugin) Apply() error { return nil } log.Log.Info("mellanox plugin Apply()") - err := p.helpers.MlxResetFW(pciAddressesToChange) - if err != nil { - return err + // Only mstfwreset if the featuregate is enabled. + if vars.MlxPluginFwReset { + err := p.helpers.MlxResetFW(pciAddressesToReset) + if err != nil { + return err + } } return p.helpers.MlxConfigFW(attributesToChange) } diff --git a/pkg/vars/vars.go b/pkg/vars/vars.go index c4ff9b9cd..cf6ca3db1 100644 --- a/pkg/vars/vars.go +++ b/pkg/vars/vars.go @@ -51,6 +51,9 @@ var ( // ParallelNicConfig global variable to perform NIC configuration in parallel ParallelNicConfig = false + // MlxPluginFwReset global variable enables mstfwreset before rebooting a node on VF changes + MlxPluginFwReset = false + // FilesystemRoot used by test to mock interactions with filesystem FilesystemRoot = ""