Implement mass machine type transition job WIP #2

prnaraya · 2023-05-31T13:21:08Z

The mass machine type job is implemented with a Docker image built locally from a golang file, and creating and applying a Job using that image. Working on adding way for user to specify namespace(s) to run the Job on rather than running on all namespaces, and a flag for the Job to force restart all running vms when changing the machine type instead of letting the user do it manually.

Signed-off-by: prnaraya <[email protected]>

After going back and forth on whether to implement using nested jobs or not, I settled for having a single job, with the calculation of the completion count being done in a go function. There are also some sections still commented out as they are WIP, notably dealing with the actual machine type strings themselves. Signed-off-by: prnaraya <[email protected]>

Signed-off-by: prnaraya <[email protected]>

iholder101

Thanks @prnaraya! This is surely a step forward!

Left some comments below.

iholder101 · 2023-05-31T13:44:32Z

informers.go

+		machineType := vm.Spec.Template.Spec.Domain.Machine.Type
+		machineTypeSubstrings := strings.Split(machineType, "-")
+		version := machineTypeSubstrings[2]


Are we sure that every VMI would have a machine type of this format?
I've created a /examples/vmi-fedora.yaml vmi and saw:

spec: domain: machine: type: q35

In this case this job would crash.
I'd recommend to validate it's the right format.

I do see

machine: type: pc-q35-rhel9.2.0

under status though

Okay so after some investigation I think I understand what needs to be done exactly.
Let's take the following as an example:

spec: domain: machine: type: q35 status: machine: type: pc-q35-rhel8.2.0

So, what would need to be done is:

Parse status.machine.type, specifically the last bit (split by - like you did, take the 3rd string, split by ., take the first string (which in this case is 8), then parse it to a number and check if it's <= 8, like in this case)*.

If the machine type is of rhel8 or below, update spec.domain.machine.type to a RHEL9 version**.

Add the label.

Open questions:
* Can we count on the fact that all machine types at the status level look like pc-q35-rhel8.2.0? Do we support other machine types with different formats?
** Which RHEL version should we update to? 9.0.0? 9.2.0?

iholder101 · 2023-05-31T14:45:53Z

informers.go

+		machineTypeSubstrings := strings.Split(machineType, "-")
+		version := machineTypeSubstrings[2]
+
+		if strings.Contains(version, "rhel") && version < "rhel9.0.0" {


What did you try to achieve with version < "rhel9.0.0"? Did you mean version != "rhel9.0.0"?
What if the version is "rhel9.2.0" or something similar?
Do all machine types have a version?

Basically the requirements for updating the machine types is updating the machine type of any VM with a machine type that is prior to RHEL 9. There are some other formats of machine type I believe so I will need validate to prevent this from crashing though.

iholder101 · 2023-05-31T14:48:54Z

informers.go

+
+func addWarningLabel (virtCli kubecli.KubevirtClient, vm *k6tv1.VirtualMachine) {
+	addLabel := fmt.Sprint(`{"metadata": {"labels": {"restart-vm-required": "true"}}}}}`)
+	virtCli.VirtualMachine(vm.Namespace).Patch(vm.Name, types.StrategicMergePatchType, []byte(addLabel), &k8sv1.PatchOptions{})


WDYT about catching an error and logging it if exists?

iholder101 · 2023-05-31T14:50:03Z

informers.go

+
+	removeLabel := fmt.Sprint(`[{"op": "remove", "path": "metadata/labels/restart-vm-required"}]`)
+	virtCli.VirtualMachine(vmi.Namespace).Patch(vmi.Name, types.StrategicMergePatchType, []byte(removeLabel), &k8sv1.PatchOptions{})
+	needsRestart--


This occurs for every VMI that's being deleted.
So it can decrease the counter for a VMI that didn't need any updates in the first place.

iholder101 · 2023-05-31T14:57:58Z

main.go

+	// for now this uses the number of vms to be restarted to determine
+	// when to exit the job rather than seeing if the actual label exists
+	// on any VMs before exiting the job
+
+	for (needsRestart > 0) {
+	}


This would busy-wait for the variable to become zero, which is pretty costly in terms of performance. A better approach would be to sleep for a few seconds between iterations. Or even better, use a channel to indicate when to wake up and exit.

But, currently, this mechanism is broken since the counter decreases when every VMI is deleted, not necessarily a relevant one. I think you need to build a structure of VMIs that should be restarted (in updateMachineTypes) to really track the relevant ones. Then, when the structure is empty, you can close a channel that would indicate to exit the job.

Yeah, this was a temporary fix that I implemented just so I had something there to use when testing against a small cluster, but I wasn't exactly sure how to implement this in a better way.

iholder101 · 2023-05-31T14:58:58Z

informers.go

+	needsRestart++
+}
+
+func removeWarningLabel(obj interface{}) {


no need to remove a label from a deleted VMI.
I think the main purpose of this event handler is to update that we have one less VMI to track

iholder101 · 2023-05-31T15:00:49Z

informers.go

+	vmiInformer.AddEventHandler(cache.ResourceEventHandlerFuncs {
+		DeleteFunc: removeWarningLabel,
+	})


WDYT about adding an event handler for newly created VMIs?
That way if a new VMI is being created with an unsupported machine type it could be added to the list of VMIs that need update, and a label could be added.

Although I'm not entirely sure it's necessary.

I could add that but I am also not sure if it is something that we would need.

Let's skip that for now. we can always add that in follow-ups

iholder101 · 2023-06-07T07:56:22Z

informers.go

@@ -49,9 +54,12 @@ func updateMachineTypes(virtCli kubecli.KubevirtClient) error {
 		machineType := vm.Spec.Template.Spec.Domain.Machine.Type
 		machineTypeSubstrings := strings.Split(machineType, "-")
 		version := machineTypeSubstrings[2]
+		if len(machineTypeSubstrings != 3) {
+			return nil
+		}

 		if strings.Contains(version, "rhel") && version < "rhel9.0.0" {


We need to change version < "rhel9.0.0".
Basically, we need to check if the x-stream version (9 in this case) is >8. In other words, if it's less or equal to 8 (e.g. 8.4.0) it needs to be updated

I am not sure I understand. Since we only want to update machine types with versions prior to the specified version (in this case RHEL 9), wouldn't a lexographical comparison of the versions be sufficient? Unless there is a format of machine type that I am missing.

In this case, "rhel8.4.0" < "rhel9.0.0" returns true, which satisfies the conditions I believe.

Yup, you have a point.

As long as we're sure this is indeed always the format, your approach is completely valid. Saying that, it is never too bad to add some more validations, as this can go silently wrong if something changes (what if the string is empty for some reason? or if the format changes slightly, like rhel-8.0.0 instead of rhel8.0.0).

But again, if you're certain that this is always the format, feel free to leave it as it 👍

and a stop channel for the job to wait until vmiList is empty Signed-off-by: prnaraya <[email protected]>

…running vmis immediately. Also added the base for the namespace and potentially labels flags to implement filtering of VMs in the future. Signed-off-by: prnaraya <[email protected]>

iholder101

Awesome progress @prnaraya! Well done!

iholder101 · 2023-06-13T06:57:04Z

main.go

+	stopCh := make(chan struct{})
+	go vmiInformer.Run(stopCh)


You can use exitJob for that purpose too

Suggested change

stopCh := make(chan struct{})

go vmiInformer.Run(stopCh)

exitJob = make(chan struct{})

go vmiInformer.Run(exitJob)

This will also initialize exitJob which is never initialized currently

iholder101 · 2023-06-13T06:57:39Z

main.go

+	go vmiInformer.Run(stopCh)
+
+
+	updateMachineTypes(virtCli)


Please handle the error (by either printing it to stdout or logging it)

iholder101 · 2023-06-13T06:58:47Z

informers.go

+		return err
+	}
+	for _, vm := range vmList.Items {
+		machineType := vm.Spec.Template.Spec.Domain.Machine.Type


This needs to be fetched from status instead of spec

I've tried to fetch this but neither VirtualMachineStatus nor VirtualMachineInstanceStatus have a field for machine types.

You can find it here. As mentioned in this comment, this is how it looks like:

spec: domain: machine: type: q35 status: machine: type: pc-q35-rhel8.2.0

I looked into this a little more, and I believe the reason why the VMI you created has just "q35" in the machine type in spec is because that is the alias for the default machine type (which is the most recent RHEL version I believe) if a machine type is not explicitly specified in the yaml. I created a VM where I explicitly set the machine type, and was able to see the full machine type string in the Spec for both the VM and the VMI. This also works if you modify examples/vmi-fedora.yaml to have an explicitly stated machine type.

That is to say, I think it is still correct to retrieve the machine type from the VM spec, and there already exists logic that will catch any machine types that do not fit the "pc-q35-rhelx.x.x" version, so VMs with a machine type of just "q35" will be ignored.

so VMs with a machine type of just "q35" will be ignored

In my opinion this is a bug.
What if the VMIs I've created were created at a time where 8.2.0 was the default and latest version? Then you're going to ignore these, although they need to be updated.

Am I missing anything?

Ah I see what you mean. I still am not sure how to access the VMI status.machine.type because I just don't see it in any of the documentation anywhere, but it might be good then in the case of the machine type being q35 to update the machine type anyway to be safe.

iholder101 · 2023-06-13T06:59:42Z

informers.go

 )

+// using this as a const allows us to easily modify the program to update if a newer version is released
+// we generally want to be updating the machine types to the most recent version
+const latestVersion = "rhel9.2.0"


nit:

Suggested change

const latestVersion = "rhel9.2.0"

const latestMachineTypeVersion = "rhel9.2.0"

iholder101 · 2023-06-13T07:06:57Z

informers.go

+	if err != nil {
+		return err
+	}
+	vmiList = append(vmiList, vm.Name)


Please all add the VM's namespace, that way it's a unique identifier.
BTW, you have MetaNamespaceKeyFunc function from Kubernetes (that you can import) that returns that string for you (i.e. <namespace>/<name>)

iholder101 · 2023-06-13T07:13:11Z

informers.go

 	listWatcher := cache.NewListWatchFromClient(virtCli.RestClient(), "virtualmachineinstances", k8sv1.NamespaceAll, fields.Everything())
 	vmiInformer := cache.NewSharedIndexInformer(listWatcher, &k6tv1.VirtualMachineInstance{}, 1*time.Hour, cache.Indexers{})
+
+	vmiInformer.AddEventHandler(cache.ResourceEventHandlerFuncs {
+		DeleteFunc: removeWarningLabel,


I'm not entirely sure, but I think that reboots might not delete the vmi entirely, but rather leave in a Succeeded state. Just to be on the safe side, I'd also add an update handler and re-use the same function you already have.

I would also add a small validation to removeWarningLabel() to make sure the VMI is either deleted or in a non-running phase.

Suggested change

DeleteFunc: removeWarningLabel,

DeleteFunc: removeWarningLabel,

UpdateFunc: func(_, newObj interface{}) {

removeWarningLabel(newObj)

},

iholder101 · 2023-06-13T07:18:45Z

informers.go

+	//check if deleted VMI is in list of VMIs that need to be restarted
+	vmiIndex := searchVMIList(vmi.Name)
+	if  vmiIndex == -1 {
+		return
+	}
+
+	// remove deleted VMI from list
+	vmiList = append(vmiList[:vmiIndex], vmiList[vmiIndex+1:]...)


Not that it's critical, but I think this could have been much easier with a map.

The map could be of type vmisPendingUpdate := make(map[string]struct{}).
Then, to search: _, exists := vmisPendingUpdate[<vm>]
To delete: delete vmisPendingUpdate[vmisPendingUpdate, <vm>

Signed-off-by: prnaraya <[email protected]>

iholder101

Awesome progress @prnaraya!! Well done! 👏

iholder101 · 2023-06-14T06:11:11Z

informers.go

+			}
+
+			// add label to running VMs that a restart is required for change to take place
+			if vm.Status.Ready {


Suggested change

if vm.Status.Ready {

if vm.Status.Created {

There's a slight difference between Created, which means that a VMI object exists for this VM, and Ready, which means that the VMI is up and ready (e.g. booted, connected, etc).

If we use Ready here we might miss a VM that had just started but did not fully boot yet. However, at this point, the machine type is already determined.

iholder101 · 2023-06-14T06:15:20Z

informers.go

+func handleVmiDeletion(obj interface{}) {
+	vmi, ok := obj.(*k6tv1.VirtualMachineInstance)
+	if !ok {
+		return


Please log / print an error in this case, since it never should happen

iholder101 · 2023-06-14T06:17:45Z

informers.go

+	vmiInformer := cache.NewSharedIndexInformer(listWatcher, &k6tv1.VirtualMachineInstance{}, 1*time.Hour, cache.Indexers{})
+
+	vmiInformer.AddEventHandler(cache.ResourceEventHandlerFuncs {
+		UpdateFunc: handleVmiUpdate,


Sorry but it seems I've mislead you :(
I've tested this, and it seems that whenever a VMI is either stopped or restarted the VMI object is being deleted:

metadata: deletionTimestamp: "2023-06-14T06:13:01Z"

So I guess we can drop the update handler after all

iholder101 · 2023-06-14T06:23:18Z

informers.go

-	vmiInformer := cache.NewSharedIndexInformer(listWatcher, &k6tv1.VirtualMachineInstance{}, 1*time.Hour, cache.Indexers{})
+func addWarningLabel (virtCli kubecli.KubevirtClient, vm *k6tv1.VirtualMachine) error {
+	addLabel := fmt.Sprint(`{"metadata": {"labels": {"restart-vm-required": "true"}}}}}`)
+	_, err := virtCli.VirtualMachineInstance(vm.Namespace).Patch(vm.Name, types.StrategicMergePatchType, []byte(addLabel), &k8sv1.PatchOptions{})


IMO we better add the label to the VM object instead of the VMI.
There are two reasons for that:

Technically, a VMI object cannot be restarted, only a VM object

The user-facing object is the VM object. This is meaningful, since most users are aware (in terms of monitoring, querying data, etc) of the VM object, and the VMI object is treated as an "implementation detail", similarly to the virt-launcher pod.

WDYT?

This makes sense to me. I think this might have been what I was originally planning to do (I can't remember why I decided not to but I can't think of a good reason for it).

iholder101 · 2023-06-14T06:24:58Z

informers.go

+	removeWarningLabel(vmi)
+}
+
+func removeWarningLabel(vmi *k6tv1.VirtualMachineInstance) {


nit: I'd make the function name a bit more accurate, since it doesn't only remove the label, but also removed the VMI from the map, ends the job if necessary, etc.

WDYT about something like handleDeletedVmi?

iholder101 · 2023-06-14T06:25:51Z

main.go

+	restartEnv, exists := os.LookupEnv("RESTART_NOW")
+	if exists {
+		restartNow, err = strconv.ParseBool(restartEnv)
+		if err != nil {
+			fmt.Println(err)
+		}
+	}
+
+	// update namespace if env is set
+	namespaceEnv, exists := os.LookupEnv("NAMESPACE")
+	if exists {
+		namespace = namespaceEnv
+	}
+


Awesome! 👍

iholder101 · 2023-06-14T06:26:52Z

main.go

 		os.Exit(1)
 	}
-
+
+	exitJob = make(chan struct{})


This is already initialized above:

var ( vmisPendingUpdate = make(map[string]struct{}) exitJob = make(chan struct{}) ...

iholder101 · 2023-06-14T06:29:34Z

mass_machine_type_transition_job.yaml

+spec:
+  template:
+    metadata:
+      name: mass-machine-type-transition
+    spec:
+      containers:
+      - name: machine-type-transition
+        image: machine-type-transition:latest
+        imagePullPolicy: IfNotPresent
+      restartPolicy: OnFailure


Please add the environment variables with default values to the job manifest. It will help guide users to use them properly.

apiVersion: batch/v1 kind: Job metadata: name: mass-machine-type-transition spec: template: metadata: name: mass-machine-type-transition spec: containers: - name: machine-type-transition image: machine-type-transition:latest imagePullPolicy: IfNotPresent env: - name: NAMESPACE value: "" # default is having no selector - name: RESTART_NOW value: false restartPolicy: OnFailure

iholder101 · 2023-06-14T06:30:59Z

main.go

-	vmiInformer, err := getVmiInformer()
+	var err error
+	// update restartNow if env is set
+	restartEnv, exists := os.LookupEnv("RESTART_NOW")


WDYT?

Suggested change

restartEnv, exists := os.LookupEnv("RESTART_NOW")

restartEnv, exists := os.LookupEnv("FORCE_RESTART")

Added environment variables to the job yaml Signed-off-by: prnaraya <[email protected]>

Signed-off-by: prnaraya <[email protected]>

…he VMI Also updated HandleDeletedVmi to remove the label from the VM accordingly. Signed-off-by: prnaraya <[email protected]>

prnaraya added 4 commits May 8, 2023 12:48

WIP commit, adding the base functions for the job and the informers

763ec77

Signed-off-by: prnaraya <[email protected]>

Updated README

1febd5e

Signed-off-by: prnaraya <[email protected]>

Implemented job using docker image created from golang code

7233349

Signed-off-by: prnaraya <[email protected]>

iholder101 reviewed May 31, 2023

View reviewed changes

iholder101 reviewed Jun 7, 2023

View reviewed changes

prnaraya force-pushed the dev branch from 4c04b27 to 9cb856c Compare June 8, 2023 21:02

Adding vmiList to track VMs that need to be restarted

87dbd43

and a stop channel for the job to wait until vmiList is empty Signed-off-by: prnaraya <[email protected]>

prnaraya force-pushed the dev branch from 9cb856c to 87dbd43 Compare June 8, 2023 21:21

Adding restartnow flag and implemented logic to restart all affected …

49b1e41

…running vmis immediately. Also added the base for the namespace and potentially labels flags to implement filtering of VMs in the future. Signed-off-by: prnaraya <[email protected]>

iholder101 reviewed Jun 13, 2023

View reviewed changes

prnaraya added 3 commits June 13, 2023 17:05

Addressing Itamar's comments

e7d4d01

Signed-off-by: prnaraya <[email protected]>

Adding env variables for restart now flag and namespace flag

906d592

Signed-off-by: prnaraya <[email protected]>

Merge branch 'restart-now-flag' into dev

d72c2e7

Signed-off-by: prnaraya <[email protected]>

iholder101 reviewed Jun 14, 2023

View reviewed changes

Removed update function for the VMIs as it is not necessary

b0b2b56

Added environment variables to the job yaml Signed-off-by: prnaraya <[email protected]>

prnaraya marked this pull request as draft June 14, 2023 17:10

prnaraya added 3 commits June 16, 2023 15:59

Restructured files to move non-informer functions into their own file.

e97b107

Signed-off-by: prnaraya <[email protected]>

Updating machine type of all VMs who have machine type of "q35"

aec2a17

Signed-off-by: prnaraya <[email protected]>

Update the addWarningLabel to apply the label to the VM rather than t…

88419b9

…he VMI Also updated HandleDeletedVmi to remove the label from the VM accordingly. Signed-off-by: prnaraya <[email protected]>

iholder101 mentioned this pull request Aug 16, 2023

Implement mass machine type transition: virtctl update machine-types cmd kubevirt/kubevirt#10222

Closed

	const latestVersion = "rhel9.2.0"
	const latestMachineTypeVersion = "rhel9.2.0"

	restartEnv, exists := os.LookupEnv("RESTART_NOW")
	restartEnv, exists := os.LookupEnv("FORCE_RESTART")

Implement mass machine type transition job WIP #2

Are you sure you want to change the base?

Implement mass machine type transition job WIP #2

Conversation

prnaraya commented May 31, 2023

iholder101 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

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

iholder101 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

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

iholder101 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

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