Support VMI eviction also for external infra clusters #242

nunnatsa · 2023-06-07T11:26:52Z

What this PR does / why we need it:
CAPK now can drain a node when its VMI is evicted, even if the VMI is running at an external cluster.

CAPK now can drain a node when its VMI is evicted, even if the VMI is running at an external cluster.

nunnatsa · 2023-06-07T11:27:20Z

/ok-to-test

coveralls · 2023-06-07T11:29:03Z

Pull Request Test Coverage Report for Build 5199391506

62 of 127 (48.82%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.8%) to 50.836%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/kubevirt/machine.go	55	120	45.83%

Totals
Change from base Build 4993600089:	-1.8%
Covered Lines:	912
Relevant Lines:	1794

💛 - Coveralls

davidvossel

looks great, i left two minor comments

davidvossel · 2023-06-12T13:38:39Z

templates/cluster-template-kccm.yaml

+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  labels:
+    capk.cluster.x-k8s.io/template-kind: extra-resource
+    cluster.x-k8s.io/cluster-name: ${CLUSTER_NAME}
+  name: manager
+  namespace: ${NAMESPACE}
+---


why do we need this new serviceAccount?

davidvossel · 2023-06-12T13:40:58Z

e2e/create-cluster_test.go

+			// debug
+			buff.Reset()
+			_ = enc.Encode(machineList.Items)


debug needs removal

davidvossel · 2023-06-12T14:07:49Z

controllers/kubevirtmachine_controller.go

@@ -65,7 +65,7 @@ type KubevirtMachineReconciler struct {
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;machines,verbs=get;list;watch
 // +kubebuilder:rbac:groups="",resources=secrets;,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines;,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances;,verbs=get;list;watch
+// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances;,verbs=get;list;watch;patch;update;delete


I think you want to remove the watch item here.

@davidvossel - the test was constantly failed without this watch. I put it back.

davidvossel · 2023-06-12T14:38:34Z

config/kccm/kustomization.yaml

+- ../rbac
+


I think this is adding the rbac used to install capk to the template used to create a guest cluster with kccm

davidvossel · 2023-06-20T15:02:06Z

config/rbac/role.yaml

  - list
-  - patch
-  - update
  - watch


can we also remove the list and watch rbac here? This helps validate that the controller can operator entirely without those two rbac permissions

davidvossel

looks great, I just had a couple of minor comments.

davidvossel · 2023-06-23T20:20:50Z

config/rbac/role.yaml

-  - list
-  - patch
-  - update
-  - watch


Can you also remove list and watch from the virtualmachines entry that's slightly below this one

davidvossel · 2023-06-23T20:39:09Z

e2e/create-cluster_test.go

+			setFakeAnnotationOnMachine(g)
+			defer removeFakeAnnotationFromMachine(g)
+


I can see how forcing the reconciliation improves the test time and consistency, but my concern is that it might mask real issues caused by the reconcile loop not popping when we need it to.

Is it possible for us to test this without forcing the reconciliation? it's okay if the wait time needs to increase?

nunnatsa · 2023-06-26T13:20:29Z

Closing and reopening. Maybe this will release the CI?

CAPK now can drain a node when its VMI is evicted, even if the VMI is running at an external cluster. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

davidvossel

/lgtm

k8s-ci-robot · 2023-06-26T15:03:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, nunnatsa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [davidvossel,nunnatsa]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 7, 2023

k8s-ci-robot requested review from davidvossel and qinqon June 7, 2023 11:27

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 7, 2023

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 7, 2023

nunnatsa force-pushed the node-drain-external branch 11 times, most recently from 5a8c14e to 84c9e65 Compare June 11, 2023 12:59

davidvossel reviewed Jun 12, 2023

View reviewed changes

nunnatsa force-pushed the node-drain-external branch 6 times, most recently from e303e1a to fc21fe8 Compare June 14, 2023 07:39

nunnatsa closed this Jun 14, 2023

nunnatsa reopened this Jun 14, 2023

nunnatsa force-pushed the node-drain-external branch from fc21fe8 to c1dd6e4 Compare June 14, 2023 11:46

nunnatsa force-pushed the node-drain-external branch 4 times, most recently from bce14c4 to 32d5f40 Compare June 15, 2023 07:06

davidvossel reviewed Jun 20, 2023

View reviewed changes

nunnatsa force-pushed the node-drain-external branch 5 times, most recently from adadef9 to 020a265 Compare June 23, 2023 16:01

davidvossel reviewed Jun 23, 2023

View reviewed changes

nunnatsa force-pushed the node-drain-external branch 8 times, most recently from 3c173b5 to faea995 Compare June 26, 2023 12:27

nunnatsa closed this Jun 26, 2023

nunnatsa reopened this Jun 26, 2023

Support VMI eviction also for external infra clusters

87a8fb7

CAPK now can drain a node when its VMI is evicted, even if the VMI is running at an external cluster. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

nunnatsa force-pushed the node-drain-external branch from faea995 to 87a8fb7 Compare June 26, 2023 14:29

davidvossel approved these changes Jun 26, 2023

View reviewed changes

k8s-ci-robot assigned davidvossel Jun 26, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023

k8s-ci-robot merged commit 44140e4 into kubernetes-sigs:main Jun 26, 2023

nunnatsa deleted the node-drain-external branch June 26, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VMI eviction also for external infra clusters #242

Support VMI eviction also for external infra clusters #242

nunnatsa commented Jun 7, 2023

nunnatsa commented Jun 7, 2023

coveralls commented Jun 7, 2023

davidvossel left a comment

davidvossel Jun 12, 2023

davidvossel Jun 12, 2023

davidvossel Jun 12, 2023

nunnatsa Jun 15, 2023

davidvossel Jun 12, 2023

davidvossel Jun 20, 2023

davidvossel left a comment

davidvossel Jun 23, 2023

davidvossel Jun 23, 2023

nunnatsa commented Jun 26, 2023

davidvossel left a comment

k8s-ci-robot commented Jun 26, 2023

		setFakeAnnotationOnMachine(g)
		defer removeFakeAnnotationFromMachine(g)

Support VMI eviction also for external infra clusters #242

Support VMI eviction also for external infra clusters #242

Conversation

nunnatsa commented Jun 7, 2023

nunnatsa commented Jun 7, 2023

coveralls commented Jun 7, 2023

Pull Request Test Coverage Report for Build 5199391506

💛 - Coveralls

davidvossel 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

davidvossel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunnatsa commented Jun 26, 2023

davidvossel left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 26, 2023