Add concurrent VM boot checkup #38
Conversation
We would like to validate a CSI driver can handle parallelization, so we start 10 VMs simultaneously where each VM has one cloned boot disk and one empty data disk. The test passes if all VMs successfully boot. We allow setting the number of concurrent VMs to boot. Signed-off-by: Arnon Gilboa <[email protected]>
Signed-off-by: Arnon Gilboa <[email protected]>
arnongilboa
force-pushed
the
multiple_vm_boot_check
branch
from
216489b
to
0fc30c6
Compare
| log.Print(ErrBootFailedOnSomeVMs) | ||
| c.results.ConcurrentVMBoot = ErrBootFailedOnSomeVMs | ||
| appendSep(errStr, ErrBootFailedOnSomeVMs) | ||
| return nil |
There was a problem hiding this comment.
I see all the returns from this function are nil, should you return some error when boot fails to abort the checkup? Or is this expected?
There was a problem hiding this comment.
Since we concurrently boot the VMs via goroutines, and we don't really have a tight timing expectations here (we have a timeout for the whole checkup), I think the best way is to wait for the goroutines to complete (they have early defer for the case of failure). We would like to continue to following checks (in the future), so I see no reason to return a error.
| wg.Wait() | ||
| if !isBootOk { | ||
| log.Print(ErrBootFailedOnSomeVMs) | ||
| c.results.ConcurrentVMBoot = ErrBootFailedOnSomeVMs |
There was a problem hiding this comment.
Maybe also keep track of the failed VMs and report how many of them errored
There was a problem hiding this comment.
It's not useful imho, as we are not interested in partial success, and you have already have each VMI boot status in the log anyway.
pkg/internal/checkup/vmi/spec.go
Outdated
| @@ -82,6 +76,11 @@ func WithDataVolume(volumeName string, pvc *corev1.PersistentVolumeClaim, snap * | |||
| Namespace: snap.Namespace, | |||
| Name: snap.Name, | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
I think you want to pass options to WithDataVolume similarly to https://github.com/kubevirt/kubevirt/blob/main/pkg/libvmi/cloudinit.go#L31
There was a problem hiding this comment.
Changed, but not sure how much I like it. wdyt?
| return nil | ||
| } | ||
|
|
||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
if you wanted to simplify, you could get rid of sync.WaitGroup and just have 2 loops
- loop over creation
- loop over waiting for ready
definitely not a must, just something to consider
There was a problem hiding this comment.
I personally prefer the goroutine-way. Looping over creation is less "concurrent", as it's serialized and you will see the last VM created long after the first one when you create for example 100 VMs. Tried that in the first shot but didn't really like this behavior. Looping over the wait for ready is ok (although it's cheaper from the goroutine if we already have it), but since it's serial we won't have the nice indication in the log whenever each VM is ready.
There was a problem hiding this comment.
right the creation http request will block. nvm
arnongilboa
force-pushed
the
multiple_vm_boot_check
branch
from
8305ffe
to
55daeb2
Compare
pkg/internal/checkup/vmi/spec.go
Outdated
| return func(dvSpec *cdiv1.DataVolumeSpec) { | ||
| dvSpec.Source.Blank = &cdiv1.DataVolumeBlankImage{} | ||
| dvSpec.Storage.Resources.Requests = corev1.ResourceList{ | ||
| corev1.ResourceStorage: resource.MustParse("1G"), |
There was a problem hiding this comment.
1G -> 1Gi will look better, normally k8s is standardized on gibibytes so you'll rarely see this
| if pvc != nil { | ||
| dvOpts = append(dvOpts, vmi.WithDataVolumePvcSource(pvc)) | ||
| } else if snap != nil { | ||
| dvOpts = append(dvOpts, vmi.WithDataVolumeSnapshotSource(snap)) | ||
| } |
There was a problem hiding this comment.
if you want you can also parametrize newVMUnderTest with options, I guess this is what you didn't like, but could be done later
There was a problem hiding this comment.
right, it will wait for another pr
Signed-off-by: Arnon Gilboa <[email protected]>
arnongilboa
force-pushed
the
multiple_vm_boot_check
branch
from
55daeb2
to
88a0323
Compare
|
/lgtm |
We would like to validate a CSI driver can handle parallelization, so we start 10 VMs simultaneously where each VM has one cloned boot disk and one empty data disk. The test passes if all VMs successfully boot. We allow setting the number of concurrent VMs to boot.
jira-ticket: https://issues.redhat.com/browse/CNV-44285