neonvm: prevent migrating VMs to the nodes with non-matching architecture

[mikhail-sakhnov]

Add node affinity to the VM spec during the live migration pending state, if it doesn't have.

#1083

[github-actions]

No changes to the coverage.

HTML Report

Click to open

[sharnoff]

Some comments

[sharnoff]

question: does len(expr.Values) > 0 mean that this function will return true for VMs that have an architecture affinity for e.g. "either amd64 or arm64" ? IIUC, would that mean that we could accidentally migrate between x86 and ARM?

[mikhail-sakhnov]

Values are checked in the loop body, if I understand correctly your question

[Omrigan]

I think what Em means that for the following input

{
	Key:      "kubernetes.io/arch",
	Operator: corev1.NodeSelectorOpIn,
	Values:   []string{"x86", "arm64"},
}

The function how it is written will return true, even though this affinity doesn't prevent us from migration between x86 and arm.

[sharnoff]

question: IIUC, does this mean that the VM will permanently get affinity for the node it starts with?

Is there a reason we can't just do it on the target pod for the migration? (I guess, potential race conditions?)

[mikhail-sakhnov]

I don't think there is a particular reason, I just feel it is a bit safer to make the guard on the very first step of the migration.

Plus if we going to have multiarch clusters it is a bit unsafe to even have VM without architecture affinity, may be in later it should be even added during the VM creation, in the vmcontroller?

[Omrigan]

it is a bit unsafe to even have VM without architecture affinity

Do we know of any particular reason it might be unsafe? The same VM can create the pod first time on x86, and if it is deleted, it can be recreated on arm node. We can potentially utilize this to auto-balance between architectures.

Although, the regular restart of endpoints anyway means the recreation of the VM object (right?), so this might not matter much.

[mikhail-sakhnov]

We can potentially utilize this to auto-balance between architectures.

if we can do it, why to even prevent migration between architectures? I was under the impression that postgres can't safely migrate from one arch to another, because of the data files, need to double check that.

[Omrigan]

We can potentially utilize this to auto-balance between architectures.

if we can do it, why to even prevent migration between architectures?

Having VM objects without affinity doesn't mean migration between architectures. Pods would still have defined architecture, architecture would be undefined only when there is no pod. This would mean if a pod is deleted for whatever reason, we can recreate it with different architecture. But I am not sure this is a practical scenario.

postgres can't safely migrate from one arch to another

It's not even postgres, it is the whole VM: all memory, including executable code, registers, etc.

[Omrigan]

This function can be rewritten to reduce the max nested level from 7 to 4, smth like below.

This implementation also doesn't use sourceNode.

Suggested change

	func hasArchitectureAffinity(vm *vmv1.VirtualMachine, sourceNode *corev1.Node) bool {
	if vm.Spec.Affinity != nil && vm.Spec.Affinity.NodeAffinity != nil && vm.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil {
	for _, term := range vm.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms {
	for _, expr := range term.MatchExpressions {
	if expr.Key == "kubernetes.io/arch" && expr.Operator == corev1.NodeSelectorOpIn && len(expr.Values) > 0 {
	for _, value := range expr.Values {
	if value == sourceNode.Status.NodeInfo.Architecture {
	return true
	}
	}
	}
	}
	}
	}
	return false
	}
	func hasArchitectureAffinity(vm *vmv1.VirtualMachine, sourceNode *corev1.Node) bool {
	if vm.Spec.Affinity == nil || vm.Spec.Affinity.NodeAffinity == nil || vm.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil {
	return false
	}
	terms = vm.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms
	for _, term := range terms {
	for _, expr := range term.MatchExpressions {
	if expr.Key == "kubernetes.io/arch" &&
	expr.Operator == corev1.NodeSelectorOpIn &&
	len(expr.Values) == 1 {
	return true
	}
	}
	}
	return false
	}

[Omrigan]

I think what Em means that for the following input

{
	Key:      "kubernetes.io/arch",
	Operator: corev1.NodeSelectorOpIn,
	Values:   []string{"x86", "arm64"},
}

The function how it is written will return true, even though this affinity doesn't prevent us from migration between x86 and arm.

[Omrigan]

it is a bit unsafe to even have VM without architecture affinity

Do we know of any particular reason it might be unsafe? The same VM can create the pod first time on x86, and if it is deleted, it can be recreated on arm node. We can potentially utilize this to auto-balance between architectures.

Although, the regular restart of endpoints anyway means the recreation of the VM object (right?), so this might not matter much.