Network Operator deployment support in vanilla K8S air gap environment #375

e0ne · 2022-07-03T08:53:55Z

No description provided.

e0ne · 2022-07-03T09:47:37Z

/retest-nic_operator_kind

ykulazhenkov

I think overall logic is fine, I added few nit comments.

Q: do we need to support use-case when user updates content of configMap with repo or CA configuration?

api/v1alpha1/nicclusterpolicy_types.go

pkg/state/state_ofed.go

adrianchiris

@e0ne should we have under docs/ a document about disconnected env covering these configurations ? or do you think its better to just have it in the official docs ? as its not regarded as a "common" flow

adrianchiris · 2022-07-04T10:24:25Z

pkg/state/state_ofed.go

+	for filename := range cm.Data {
+		filenames = append(filenames, filename)
+	}
+	// sort so volume mounts are added to spec in deterministic order


can you extend this comment to explain why this is needed ?

api/v1alpha1/nicclusterpolicy_types.go

pkg/state/state_ofed.go

adrianchiris · 2022-07-04T10:51:41Z

deployment/network-operator/README.md

@@ -339,6 +339,8 @@ imagePullSecrets:
 | `ofedDriver.version` | string | `5.5-1.0.3.2` | Mellanox OFED driver version                                                                                                                                              |
 | `ofedDriver.imagePullSecrets` | list | `[]` | An optional list of references to secrets to use for pulling any of the Mellanox OFED driver image                                                                        |
 | `ofedDriver.env` | list | `[]` | An optional list of [environment variables](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#envvar-v1-core) passed to the Mellanox OFED driver image |
+| `ofedDriver.repoConfig.configMapName` | string | `` | Private mirror repository configuration configMap name |


i feel like these two should be named the same : either name or configMapName WDYT ?

need to align thoroughout the PR (helm, CRD, code)

I think this is to align with NVIDIA gpu-operator, they have such names for parameters

these two parameters are very similar, and should be named the same for consistency.

we do not have a strict requirement to have the same api as gpu operator. where we can improve / provide a more consistent api , i think we should.

changed to 'name' in both places

@e0ne please also change here in the README

adrianchiris · 2022-07-04T10:54:29Z

pkg/state/state_ofed.go

@@ -295,7 +295,7 @@ func (s *stateOFED) getManifestObjects(
 		if err != nil {
 			return nil, fmt.Errorf("ERROR: failed to create ConfigMap VolumeMounts for custom repo config: %v", err)
 		}
-		volume := s.createConfigMapVolume(cr.Spec.OFEDDriver.CertConfig.Name, itemsToInclude)
+		volume := s.createConfigMapVolume(cr.Spec.OFEDDriver.RepoConfig.ConfigMapName, itemsToInclude)


this needs to be in previous commit

adrianchiris · 2022-07-05T07:39:48Z

do we need to support use-case when user updates content of configMap with repo or CA configuration

ATM this is not a defined use-case.
however, keeping in mind that these are used only during driver compilation (pre-step to install kernel-headers) then even if these change and driver is already compiled (or rather after kernel-header install phase) then it will have no effect.

in case driver container/node restarted, it would have new data (repo/creds)

These options are broken at the moment and was never used. To pass environment variable to OFED driver you can use ofedDriver.env parameters. HTTP_PROXY/HTTPS_PROXY environment variabes are not confiurable for operator and nv-peer-mem pod anymore. Signed-off-by: Ivan Kolodiazhny <ikolodiazhny@nvidia.com>

adrianchiris · 2022-07-06T10:50:56Z

config/crd/bases/mellanox.com_nicclusterpolicies.yaml

@@ -405,6 +412,12 @@ spec:
                    - initialDelaySeconds
                    - periodSeconds
                    type: object
+                  repoConfig:
+                    description: 'Optional: Custom repo configuration for OFED container'


nit: here you use OFED container and above you use driver container. pick one :)

adrianchiris · 2022-07-06T10:52:28Z

api/v1alpha1/nicclusterpolicy_types.go

@@ -69,6 +81,10 @@ type OFEDDriverSpec struct {
 	Env []v1.EnvVar `json:"env,omitempty"`
 	// Ofed auto-upgrade settings
 	OfedUpgradePolicy *OfedUpgradePolicySpec `json:"upgradePolicy,omitempty"`
+	// Optional: Custom certificates configuration for driver container
+	CertConfig *DriverCertConfigSpec `json:"certConfig,omitempty"`


General question.

can this also be used for package manager related certificates (e.g when signing repo packages with custom gpg key)

i guess not, so if later on we will need to support it, then we can add RepoCertConfig.

i think the description here should be updated to explicitly state that these are SSL certificates

adrianchiris · 2022-07-06T10:56:53Z

api/v1alpha1/nicclusterpolicy_types.go

@@ -55,6 +55,18 @@ type PodProbeSpec struct {
 	PeriodSeconds       int `json:"periodSeconds"`
 }

+// DriverCertConfigSpec defines custom certificates configuration for driver container
+type DriverCertConfigSpec struct {


The two are essentially the same.

i.e they (*ConfigSpec) point to a configmap

maybe define single type: ConfigMapSpec
an use it as the type of both CertConfig and RepoConfig below

or name it ConfigMapNameReference like in openshift :)

https://github.com/openshift/api/blob/8324d657dee1d594a8a7768e5569fea6a8f887a9/config/v1/types.go#L18

ok, let's rename it

adrianchiris · 2022-07-06T10:58:03Z

deployment/network-operator/crds/mellanox.com_nicclusterpolicies.yaml

@@ -405,6 +412,12 @@ spec:
                    - initialDelaySeconds
                    - periodSeconds
                    type: object
+                  repoConfig:
+                    description: 'Optional: Custom repo configuration for OFED container'


same comment as above , use either "OFED container" or "driver container"

pkg/state/state_ofed.go

adrianchiris · 2022-07-06T11:24:08Z

pkg/state/state_ofed.go

+
+		err = s.mountAdditionalVolumesFromConfigMap(&additionalVolMounts, cr.Spec.OFEDDriver.RepoConfig.Name, destinationDir)
+		if err != nil {
+			return nil, fmt.Errorf("failed to custom repositories configuration: %v", err)


same comment as above

api/v1alpha1/nicclusterpolicy_types.go

adrianchiris · 2022-07-07T07:53:49Z

pkg/state/state_ofed.go

@@ -152,6 +239,22 @@ func (s *stateOFED) GetWatchSources() map[string]*source.Kind {
 	return wr
 }

+func (s *stateOFED) mountAdditionalVolumesFromConfigMap(


nit: this method doesnt actually mount anything but rather generate volumes and volume mounts. also it mainly revolves around additionalVolumeMounts type so perhaps we should have a method in that struct.

suggestion:

define:
(am *additionalMounts) FromConfigMap(cm *v1.ConfigMap, destDir) error

which will populate additionalMounts from configmap.

then you just need to rename this method to handleAdditionalMounts which will get configmap and call am.FromConfigMap(....)

or this logic (Getting config map and populating additionalMount struct can be called directly from getManifestObjects

wdyt ?

also please add docstring to added methods above :) if we go this route.

adrianchiris · 2022-07-07T07:58:19Z

@e0ne PTAL

adrianchiris · 2022-07-07T13:55:21Z

pkg/state/state_ofed.go

+func (a *additionalVolumeMounts) FromConfigMap(configMap *v1.ConfigMap, destDir string) error {
+	volumeMounts, itemsToInclude, err := a.createConfigMapVolumeMounts(configMap, destDir)
+	if err != nil {
+		return fmt.Errorf("ERROR: failed to create ConfigMap VolumeMounts for custom repo config: %v", err)


i think its clear its an error so no need for ERROR: prefix.

also error should not reference custom repo config but rather something generic like:
"failed to create Volume mounts from configmap. %v, err"

adrianchiris · 2022-07-07T13:58:38Z

pkg/state/state_ofed.go

+	objKey := client.ObjectKey{Namespace: namespace, Name: configMapName}
+	err := s.client.Get(context.TODO(), objKey, configMap)
+	if err != nil {
+		return fmt.Errorf("ERROR: could not get ConfigMap %s from client: %v", configMapName, err)


nit: Drop ERROR: prefix the object returned here is an error

adrianchiris · 2022-07-07T13:58:41Z

pkg/state/state_ofed.go

+
+	err = volMounts.FromConfigMap(configMap, destDir)
+	if err != nil {
+		return fmt.Errorf("ERROR: could not create volume mounts for ConfigMap: %s", configMapName)


nit: Drop ERROR: prefix the object returned here is an error

This patch adds certConfig and custom repo configuration options for OFED driver. Signed-off-by: Ivan Kolodiazhny <ikolodiazhny@nvidia.com>

adrianchiris · 2022-07-07T14:04:22Z

LGTM, thanks @e0ne !

just need CI to pass and this can be merged

ykulazhenkov reviewed Jul 4, 2022

View reviewed changes

api/v1alpha1/nicclusterpolicy_types.go Show resolved Hide resolved

pkg/state/state_ofed.go Outdated Show resolved Hide resolved

pkg/state/state_ofed.go Show resolved Hide resolved

pkg/state/state_ofed.go Outdated Show resolved Hide resolved

e0ne force-pushed the proxy branch from a7e4307 to 62f7974 Compare July 4, 2022 09:03

adrianchiris reviewed Jul 4, 2022

View reviewed changes

ykulazhenkov mentioned this pull request Jul 5, 2022

Add support for proxy and custom CA certificate for Openshift #378

Merged

e0ne marked this pull request as draft July 5, 2022 12:04

e0ne force-pushed the proxy branch from 9633909 to 2213762 Compare July 5, 2022 13:14

e0ne marked this pull request as ready for review July 5, 2022 13:14

e0ne force-pushed the proxy branch from 2213762 to 7d1815d Compare July 5, 2022 14:32

adrianchiris requested changes Jul 6, 2022

View reviewed changes

e0ne force-pushed the proxy branch 2 times, most recently from d012260 to b201d8e Compare July 6, 2022 12:33

adrianchiris reviewed Jul 7, 2022

View reviewed changes

api/v1alpha1/nicclusterpolicy_types.go Outdated Show resolved Hide resolved

e0ne force-pushed the proxy branch from b201d8e to 9eac13b Compare July 7, 2022 06:55

adrianchiris reviewed Jul 7, 2022

View reviewed changes

e0ne force-pushed the proxy branch from 9eac13b to 8559d99 Compare July 7, 2022 10:55

e0ne marked this pull request as draft July 7, 2022 10:55

e0ne marked this pull request as ready for review July 7, 2022 11:02

e0ne force-pushed the proxy branch from 8559d99 to 109b842 Compare July 7, 2022 11:04

adrianchiris reviewed Jul 7, 2022

View reviewed changes

e0ne force-pushed the proxy branch from 109b842 to 3b8d14c Compare July 7, 2022 13:59

Add support of air gap OFED deployment

a9c787f

This patch adds certConfig and custom repo configuration options for OFED driver. Signed-off-by: Ivan Kolodiazhny <ikolodiazhny@nvidia.com>

e0ne force-pushed the proxy branch from 3b8d14c to a9c787f Compare July 7, 2022 14:01

adrianchiris approved these changes Jul 7, 2022

View reviewed changes

ykulazhenkov approved these changes Jul 7, 2022

View reviewed changes

e0ne merged commit 4a67542 into Mellanox:master Jul 7, 2022

e0ne mentioned this pull request Jul 8, 2022

Release v1.3.0-beta.2 #383

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Operator deployment support in vanilla K8S air gap environment #375

Network Operator deployment support in vanilla K8S air gap environment #375

e0ne commented Jul 3, 2022

e0ne commented Jul 3, 2022

ykulazhenkov left a comment

adrianchiris left a comment

adrianchiris Jul 4, 2022

e0ne Jul 5, 2022

adrianchiris Jul 4, 2022

ykulazhenkov Jul 4, 2022 •

edited

Loading

adrianchiris Jul 4, 2022

e0ne Jul 4, 2022

rollandf Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 4, 2022

adrianchiris commented Jul 5, 2022 •

edited

Loading

adrianchiris Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 6, 2022

adrianchiris Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 6, 2022

e0ne Jul 6, 2022

adrianchiris Jul 7, 2022 •

edited

Loading

adrianchiris Jul 7, 2022

adrianchiris commented Jul 7, 2022

adrianchiris Jul 7, 2022

e0ne Jul 7, 2022

adrianchiris Jul 7, 2022

adrianchiris Jul 7, 2022

adrianchiris commented Jul 7, 2022

Network Operator deployment support in vanilla K8S air gap environment #375

Network Operator deployment support in vanilla K8S air gap environment #375

Conversation

e0ne commented Jul 3, 2022

e0ne commented Jul 3, 2022

ykulazhenkov left a 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

ykulazhenkov Jul 4, 2022 • 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

adrianchiris commented Jul 5, 2022 • 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

adrianchiris Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Jul 7, 2022

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 7, 2022

ykulazhenkov Jul 4, 2022 •

edited

Loading

adrianchiris commented Jul 5, 2022 •

edited

Loading

adrianchiris Jul 7, 2022 •

edited

Loading