external control plane #309

zshi-redhat · 2022-05-26T09:28:19Z

Enable sriov operator in the cluster w/o master nodes (k8s apiserver and other control plane components are managed externally).

github-actions · 2022-05-26T09:28:34Z

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 · 2022-05-26T09:53:17Z

pkg/utils/cluster.go

 	infra := &configv1.Infrastructure{}
 	err := c.Get(context.TODO(), types.NamespacedName{Name: infraResourceName}, infra)
 	if err != nil {
-		return false, err
+		return "", err


Please, add context information to the error:

Suggested change

return "", err

return "", fmt.Errorf("can't get Infrastructure [%s]: %w, infraResourceName, err)

github-actions · 2022-05-26T10:28: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 · 2022-05-26T11:26:56Z

@zshi-redhat can you explain abit more about the use-case ?

zshi-redhat · 2022-05-26T12:18:20Z

@zshi-redhat can you explain abit more about the use-case ?

@adrianchiris The use case is for managed kubernetes services (similar to aws EKS or google GKE), where the kube control plane components (apiserver, etcd, kube-scheduler etc) are hosted by cloud service provider in the provider managed kubernetes cluster and only workload components (worker nodes) are visible to end user. In such case, end user can still deploy sriov operator on the worker node by accessing k8s apiserver managed externally.

DPU two-cluster design can take advantage of this as well, where infra and tenant control planes are hosted in a separate k8s cluster (management cluster) to reduce the footprint of master nodes. dpu-network-operator runs in the management cluster which has access to both infra and tenant APIs.

pkg/utils/cluster.go

github-actions · 2022-06-13T05:53: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.

github-actions · 2022-06-13T05:56: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 · 2022-06-13T06:03:10Z

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 · 2022-06-21T15:56:42Z

bindata/manifests/operator-webhook/server.yaml

                operator: Exists
            - matchExpressions:
                - key: node-role.kubernetes.io/control-plane
                  operator: Exists
      tolerations:
+      {{ if .ExternalControlPlane }}
+      - key: "node-role.kubernetes.io/worker"


worker taint ? that's a thing ?

adrianchiris · 2022-06-21T16:07:34Z

bindata/manifests/operator-webhook/server.yaml

@@ -32,13 +32,21 @@ spec:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
+              {{ if .ExternalControlPlane }}


in case of external control plane maybe drop the entire 'requiredDuringScheduling....'

or alternatively just add another - matchExpression as these are ORed

replacing one with another is not good IMO

dropped requiredDuringScheduling in External mode.

adrianchiris · 2022-06-21T16:09:04Z

bindata/manifests/webhook/server.yaml

@@ -35,13 +35,21 @@ spec:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
+              {{ if .ExternalControlPlane }}


same comment as above

adrianchiris · 2022-06-21T16:09:47Z

bindata/manifests/webhook/server.yaml

                operator: Exists
            - matchExpressions:
                - key: node-role.kubernetes.io/control-plane
                  operator: Exists
      tolerations:
+      {{ if .ExternalControlPlane }}
+      - key: "node-role.kubernetes.io/worker"


same question about worker taint

removed "node-role.kubernetes.io/worker"

adrianchiris · 2022-06-21T16:16:25Z

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

@@ -47,6 +47,10 @@ spec:
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
+            - name: K8S_POD_NAME


you already have POD_NAME defined below

you can use spec.nodeName to expose Node name directly and avoid logic added to get node name from pod

removed K8S_POD_NAME and added NODE_NAME

adrianchiris · 2022-06-21T16:34:17Z

pkg/utils/cluster.go

+		if err != nil {
+			return false, err
+		}
+		switch role {


no need for switch here imo

if role == worker { return true, nil }

adrianchiris · 2022-06-21T16:35:57Z

pkg/utils/cluster.go

+	}
+
+	nodeList := &corev1.NodeList{}
+	err = c.List(context.TODO(), nodeList)


you gen get node by name no ? no need to list and iterate here

adrianchiris · 2022-06-21T16:37:42Z

pkg/utils/cluster.go

-	infraResourceName = "cluster"
+	infraResourceName  = "cluster"
+	workerNodeLabelKey = "node-role.kubernetes.io/worker"
+	masterNodeLabelKey = "node-role.kubernetes.io/master"


what about node-role.kubernetes.io/control-plane ?
in k8s 1.24 kubeadm now replaced master with control-plane (master label deprecated)

added support for both.

github-actions · 2022-07-01T06:55: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 · 2022-07-01T07:20: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.

pkg/utils/cluster.go

bn222 · 2022-07-04T08:08:19Z

Can you put your first comment in the commit message? Or something more elaborate?

github-actions · 2022-07-06T01:03:15Z

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.

zshi-redhat · 2022-07-06T01:03:33Z

Can you put your first comment in the commit message? Or something more elaborate?

added the comment in the commit message.

bn222 · 2022-07-06T11:58:57Z

/lgtm

SchSeba · 2022-08-11T15:14:07Z

Hi @zshi-redhat will you please be able to rebase this PR?

github-actions · 2022-08-18T08:40:35Z

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.

zshi-redhat · 2022-08-18T08:41:15Z

Hi @zshi-redhat will you please be able to rebase this PR?

rebased, thanks for reminding!

coveralls · 2022-08-18T08:47:02Z

Pull Request Test Coverage Report for Build 3011830441

17 of 54 (31.48%) changed or added relevant lines in 2 files are covered.
8 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.1%) to 15.993%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/cluster.go	11	48	22.92%

Files with Coverage Reduction	New Missed Lines	%
controllers/sriovibnetwork_controller.go	2	67.33%
controllers/sriovnetwork_controller.go	6	65.0%

Totals
Change from base Build 3010210970:	0.1%
Covered Lines:	1176
Relevant Lines:	7353

💛 - Coveralls

bn222 · 2022-09-01T07:53:41Z

@zshi-redhat We could label the workers in guest cluster with label "master" (such that the role will be master,worker). In that case, this would not be needed. For the sake of my understanding, is that correct? I still think it's nice to allow to run on workers so we will still want to have this PR in.

zshi-redhat · 2022-09-01T08:02:49Z

@zshi-redhat We could label the workers in guest cluster with label "master" (such that the role will be master,worker). In that case, this would not be needed. For the sake of my understanding, is that correct?

@bn222 This may not be desired from my understanding, since the node pool is supposed to only contain worker nodes, not sure if there will be any side effects to other pods.

btw, do we still need this PR? or is it going to be integreted to Tyler's?

github-actions · 2022-09-01T08:04: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.

zshi-redhat · 2022-09-01T09:53:06Z

@adrianchiris PTAL.

adrianchiris

a couple of nits otherwise LGTM

pkg/utils/cluster.go

github-actions · 2022-09-08T00:42:34Z

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 · 2022-09-08T00:55:54Z

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.

The use case is for managed kubernetes services (similar to aws EKS or google GKE), where the kubernetes control plane components (apiserver, etcd, kube-scheduler etc) are hosted by cloud service provider in the provider managed kubernetes cluster and only workload components (worker nodes) are visible to end user. In such case, end user can still deploy sriov operator on the worker node by accessing k8s apiserver hosted externally. Signed-off-by: Zenghui Shi <zshi@redhat.com>

github-actions · 2022-09-08T01:17:35Z

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 · 2022-09-08T06:02:56Z

/test-all

adrianchiris

LGTM, lets wait for e2e test to pass

zeeke reviewed May 26, 2022

View reviewed changes

zshi-redhat force-pushed the external-control-plane branch from 519ff98 to 65be425 Compare May 26, 2022 10:28

adrianchiris reviewed Jun 1, 2022

View reviewed changes

pkg/utils/cluster.go Show resolved Hide resolved

zshi-redhat force-pushed the external-control-plane branch from 65be425 to 7e0a91d Compare June 13, 2022 05:53

zshi-redhat force-pushed the external-control-plane branch from 7e0a91d to 6fcedfa Compare June 13, 2022 05:55

zshi-redhat force-pushed the external-control-plane branch from 6fcedfa to 569a819 Compare June 13, 2022 06:02

adrianchiris reviewed Jun 21, 2022

View reviewed changes

zshi-redhat force-pushed the external-control-plane branch from 569a819 to 39b8086 Compare July 1, 2022 06:54

zshi-redhat force-pushed the external-control-plane branch from 39b8086 to b4f74fc Compare July 1, 2022 07:19

zshi-redhat requested review from bn222 and SchSeba July 1, 2022 10:42

bn222 reviewed Jul 4, 2022

View reviewed changes

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

zshi-redhat force-pushed the external-control-plane branch from b4f74fc to 51e05b3 Compare July 6, 2022 01:03

github-actions bot added the lgtm label Jul 6, 2022

zshi-redhat force-pushed the external-control-plane branch from 51e05b3 to c045a1f Compare August 18, 2022 08:40

relyt0925 mentioned this pull request Aug 27, 2022

ensure network daemon checks if machineConfigPool resource registered before attempting to pause the MachineConfigPool #350

Merged

zshi-redhat force-pushed the external-control-plane branch from c045a1f to a034880 Compare September 1, 2022 08:03

adrianchiris reviewed Sep 7, 2022

View reviewed changes

pkg/utils/cluster.go Show resolved Hide resolved

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

zshi-redhat force-pushed the external-control-plane branch from a034880 to 8136a82 Compare September 8, 2022 00:42

zshi-redhat force-pushed the external-control-plane branch from 8136a82 to 34e7ec1 Compare September 8, 2022 00:55

zshi-redhat force-pushed the external-control-plane branch from 34e7ec1 to 711302e Compare September 8, 2022 01:17

adrianchiris approved these changes Sep 8, 2022

View reviewed changes

bn222 merged commit 54ba95d into k8snetworkplumbingwg:master Sep 8, 2022

	return "", err
	return "", fmt.Errorf("can't get Infrastructure [%s]: %w, infraResourceName, err)

external control plane #309

external control plane #309

Conversation

zshi-redhat commented May 26, 2022 • edited

github-actions bot commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 26, 2022

adrianchiris commented May 26, 2022

zshi-redhat commented May 26, 2022

github-actions bot commented Jun 13, 2022

github-actions bot commented Jun 13, 2022

github-actions bot commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Jun 21, 2022 • edited

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

github-actions bot commented Jul 1, 2022

github-actions bot commented Jul 1, 2022

bn222 commented Jul 4, 2022

github-actions bot commented Jul 6, 2022

zshi-redhat commented Jul 6, 2022

bn222 commented Jul 6, 2022

SchSeba commented Aug 11, 2022

github-actions bot commented Aug 18, 2022

zshi-redhat commented Aug 18, 2022

coveralls commented Aug 18, 2022 • edited

Pull Request Test Coverage Report for Build 3011830441

💛 - Coveralls

bn222 commented Sep 1, 2022

zshi-redhat commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

zshi-redhat commented Sep 1, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

adrianchiris commented Sep 8, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

zshi-redhat commented May 26, 2022 •

edited

adrianchiris Jun 21, 2022 •

edited

coveralls commented Aug 18, 2022 •

edited