Easy fixes #2169
Easy fixes #2169
Conversation
kubevirt-bot
added
release-note-none
This patch changes the way the default value for memory-request is set so it won't clear the cpu-request, if the latter is set. This fixes the following bug: 1. Have a namespace with no memory limits set. 2. Start a VM with cpu-request=X and no memory-request set. 3. The VM would start with memory-request=<default value> and cpu-request will not be set. Signed-off-by: Arik Hadas <ahadas@redhat.com>
When creating a VMI from a VM, the Namespace that the VM resides in should be copied to the VMI in order to fetch the namespace-limits properly in the mutating webhook. Signed-off-by: Arik Hadas <ahadas@redhat.com>
| @@ -112,9 +112,10 @@ func SetDefaults_Firmware(obj *Firmware) { | |||
| func SetDefaults_VirtualMachineInstance(obj *VirtualMachineInstance) { | |||
| // FIXME we need proper validation and configurable defaulting instead of this | |||
| if _, exists := obj.Spec.Domain.Resources.Requests[v1.ResourceMemory]; !exists { | |||
| obj.Spec.Domain.Resources.Requests = v1.ResourceList{ | |||
| v1.ResourceMemory: resource.MustParse("8192Ki"), | |||
| if obj.Spec.Domain.Resources.Requests == nil { | |||
There was a problem hiding this comment.
can requests even be nil here? wouldn't the if statement above this one cause a crash if it was?
There was a problem hiding this comment.
yeah, I was surprised by that as well but I found out that:
type mm map[string]int
func main() {
var aa mm
fmt.Println(aa["b"])
if aa == nil {
fmt.Println("nil")
}
}returns:
0
nil
I discovered that because without this block (lines 115-117), tests that don't set requests explode with panic on the assignment at line 118
There was a problem hiding this comment.
@davidvossel Just for reference from
https://blog.golang.org/go-maps-in-action
Map types are reference types, like pointers or slices, and so the value of m above is nil; it doesn't point to an initialized map. A nil map behaves like an empty map when reading, but attempts to write to a nil map will cause a runtime panic; don't do that.
| } | ||
| obj.Spec.Domain.Resources.Requests[v1.ResourceMemory] = resource.MustParse("8192Ki") |
There was a problem hiding this comment.
oh wow, so we were clobbering the whole requests map if memory wasn't specified previously. good catch 👍
| @@ -527,6 +527,7 @@ func (c *VMController) setupVMIFromVM(vm *virtv1.VirtualMachine) *virtv1.Virtual | |||
| vmi.ObjectMeta = vm.Spec.Template.ObjectMeta | |||
| vmi.ObjectMeta.Name = basename | |||
| vmi.ObjectMeta.GenerateName = basename | |||
| vmi.ObjectMeta.Namespace = vm.ObjectMeta.Namespace | |||
There was a problem hiding this comment.
it's fine to set this I suppose. it shouldn't make any difference though. We create the vmi in the same namespace as its parent vm, so namespace gets assigned regardless
There was a problem hiding this comment.
right, eventually the VMI would be created within the same namespace - I didn't investigate the whole flow but this is probably set relatively late since when not setting it here, the mutating-hook receives the VMI without the namespace and so the relevant namespace-limits are not applied.
|
/assign @cynepco3hahue |
What this PR does / why we need it:
This PR introduces easy fixes to issues that were discovered while testing namespace-limits:
Special notes for your reviewer:
There is an on-going discussion in parallel on whether or not default memory should be set by Kubevirt. It is debatable and therefore IMO it is better to fix for the current code, even if it would be changed/removed anytime soon.
Release note: