Pre-pull custom kernel / initrd as an init container #6589

iholder101 · 2021-10-13T10:39:43Z

What this PR does / why we need it:
Currently when one specifies custom initrd / kernel image to boot from (a feature that was enabled in this PR #5416) it is not being prepulled as an init container - this is now changed.

This is important because if the image is not pre-pulled it'll be pulled during compute container runtime which can lead to virt-launcher exceeding timeouts.

Addresses this issue: #6552
Fixes this Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2017251

Special notes for your reviewer:

Release note:

custom kernel / initrd to boot from is now pre-pulled which improves stability

iholder101 · 2021-10-13T10:39:57Z

/cc @rmohr

maiqueb

I'm OK with the approach, it makes sense to me.

I'm not OK with the implementation; I think having 2 distinct functions would make this clearer.

maiqueb · 2021-10-13T11:22:35Z

pkg/container-disk/container-disk.go

@@ -212,9 +212,9 @@ func GenerateContainers(vmi *v1.VirtualMachineInstance, podVolumeName string, bi
 	return generateContainersHelper(vmi, podVolumeName, binVolumeName, false)
 }

-func GenerateKernelBootContainer(vmi *v1.VirtualMachineInstance, podVolumeName string, binVolumeName string) *kubev1.Container {
+func GenerateKernelBootContainers(vmi *v1.VirtualMachineInstance, podVolumeName string, binVolumeName string) (container, initContainer *kubev1.Container) {


I think having 2 different functions would make this a lot clearer.

For instance, why is the init container the second container to be returned ? I - not sure if I'm a weird person ... - would have assumed it to be the first, because I need it first (yes, weak argument).

Actually, in code, regular containers are used first :)
But you have a point, I'll change that.

Thanks!

maiqueb · 2021-10-13T11:23:03Z

pkg/container-disk/container-disk.go

+	createContainer := func(isInit bool) *kubev1.Container {
+		return generateContainerFromVolume(vmi, podVolumeName, binVolumeName, isInit, true, &kernelBootVolume, fakeVolumeIdx)
+	}
+	return createContainer(false), createContainer(true)


Also having 2 functions would prevent having boolean arguments, which are basically a code smell.

maiqueb

Thanks.

maiqueb · 2021-10-13T11:58:46Z

pkg/container-disk/container-disk.go

-		return generateContainerFromVolume(vmi, podVolumeName, binVolumeName, isInit, true, &kernelBootVolume, fakeVolumeIdx)
-	}
-	return createContainer(false), createContainer(true)
+	return generateContainerFromVolume(vmi, podVolumeName, binVolumeName, isInit, true, &kernelBootVolume, fakeVolumeIdx)


I also don't like the booleans here, but, this is clearly refactor-land. :)

maiqueb · 2021-10-13T12:00:45Z

pkg/virt-controller/services/template.go

@@ -1375,6 +1375,7 @@ func (t *templateService) renderLaunchManifest(vmi *v1.VirtualMachineInstance, t
 		// this causes containerDisks to be pre-pulled before virt-launcher starts.
 		initContainers = append(initContainers, containerdisk.GenerateInitContainers(vmi, "container-disks", "virt-bin-share-dir")...)

+		kernelBootInitContainer := containerdisk.GenerateKernelBootInitContainer(vmi, "container-disks", "virt-bin-share-dir")


I think you could inline this below:

if kernelBootInitContainer != nil { initContainers = append( initContainers, *containerdisk.GenerateKernelBootInitContainer(vmi, "container-disks", "virt-bin-share-dir")) }

This is only an opinionated suggestion. You're welcome to disagree and / or not accept it.

It's possible :)
The only caveat is that we assume containerdisk.GenerateKernelBootInitContainer(vmi, "container-disks", "virt-bin-share-dir") is never nil if if kernelBootContainer is not nil and if we're wrong we'll be brutally killed.

on the other hand maybe it's better to be killed right away than continue executing wrong behavior.

So sure, I'll change it :)

iholder101 · 2021-10-14T06:34:54Z

/test pull-kubevirt-e2e-k8s-1.21-sig-compute

iholder101 · 2021-10-14T09:30:19Z

/retest

rmohr · 2021-10-15T08:37:08Z

pkg/virt-controller/services/template_test.go


-				Expect(hasContainerWithName(containers, "kernel-boot")).To(BeTrue())
-				Expect(hasContainerWithName(pod.Spec.InitContainers, "container-disk-binary")).To(BeTrue())
+				for _, containerArray := range [][]kubev1.Container{initContainers, containers} {


This looks slightly over-engineered. How about simply doing this:

Expect(hasContainerWithName(initContainers, "container-disk-binary")).To(BeTrue()) Expect(hasContainerWithName(initContainers, "kernel-boot")).To(BeTrue()) Expect(hasContainerWithName(containers, "kernel-boot")).To(BeTrue())

it is much easier to understand and runtime efficiency is really not a concern.

Thanks @rmohr!
Done.

Currently when one specifies custom initrd / kernel image to boot from it is not being prepulled as an init container. Therefore, virt-launcher might exceed timeouts. Therefore kernel-boot container is now being prepulled. Signed-off-by: Itamar Holder <iholder@redhat.com>

Signed-off-by: Itamar Holder <iholder@redhat.com>

kubevirt-bot · 2021-10-17T10:40:46Z

@iholder-redhat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-arm64	`b5df147`	link	false	`/test pull-kubevirt-e2e-arm64`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

iholder101 · 2021-10-17T12:20:54Z

/retest

rmohr · 2021-10-19T07:44:42Z

/approve

thanks.

kubevirt-bot · 2021-10-19T07:44:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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~~ [rmohr]

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

xpivarc · 2021-11-19T12:43:20Z

/cherrypick release-0.44

kubevirt-bot · 2021-11-19T12:43:56Z

@xpivarc: #6589 failed to apply on top of branch "release-0.44":

Applying: Prepull kernel/initrd custom boot container as initContainer
Using index info to reconstruct a base tree...
M	pkg/container-disk/container-disk.go
M	pkg/virt-controller/services/template.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/services/template.go
CONFLICT (content): Merge conflict in pkg/virt-controller/services/template.go
Auto-merging pkg/container-disk/container-disk.go
CONFLICT (content): Merge conflict in pkg/container-disk/container-disk.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Prepull kernel/initrd custom boot container as initContainer
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.44

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Oct 13, 2021

kubevirt-bot requested a review from rmohr October 13, 2021 10:39

iholder101 mentioned this pull request Oct 13, 2021

Kernel image is not prepulled as an initConainer #6552

Closed

kubevirt-bot requested review from maiqueb and zcahana October 13, 2021 10:41

maiqueb suggested changes Oct 13, 2021

View reviewed changes

kubevirt-bot assigned maiqueb Oct 13, 2021

maiqueb approved these changes Oct 13, 2021

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021

rmohr reviewed Oct 15, 2021

View reviewed changes

iholder101 added 4 commits October 17, 2021 09:49

Adjust test to expect kernel boot as an init container

ee7295f

Signed-off-by: Itamar Holder <iholder@redhat.com>

Split function to create container and init container to 2 functions

e163398

Signed-off-by: Itamar Holder <iholder@redhat.com>

Make unit test easier to reason about

292d778

Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the feaure/kernel-boot-container-as-init branch from 27f1583 to 292d778 Compare October 17, 2021 08:14

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021

xpivarc approved these changes Oct 19, 2021

View reviewed changes

kubevirt-bot assigned xpivarc Oct 19, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2021

kubevirt-bot merged commit 5d06fbd into kubevirt:main Oct 19, 2021

xpivarc mentioned this pull request Nov 24, 2021

[release-0.44] Pre-pull custom kernel / initrd as an init container #6843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-pull custom kernel / initrd as an init container #6589

Pre-pull custom kernel / initrd as an init container #6589

iholder101 commented Oct 13, 2021 •

edited

iholder101 commented Oct 13, 2021

maiqueb left a comment

maiqueb Oct 13, 2021

iholder101 Oct 13, 2021

maiqueb Oct 13, 2021

maiqueb left a comment

maiqueb Oct 13, 2021

maiqueb Oct 13, 2021

iholder101 Oct 13, 2021

iholder101 commented Oct 14, 2021

iholder101 commented Oct 14, 2021

rmohr Oct 15, 2021 •

edited

iholder101 Oct 17, 2021

kubevirt-bot commented Oct 17, 2021 •

edited

iholder101 commented Oct 17, 2021

rmohr commented Oct 19, 2021

kubevirt-bot commented Oct 19, 2021

xpivarc commented Nov 19, 2021

kubevirt-bot commented Nov 19, 2021

Pre-pull custom kernel / initrd as an init container #6589

Pre-pull custom kernel / initrd as an init container #6589

Conversation

iholder101 commented Oct 13, 2021 • edited

iholder101 commented Oct 13, 2021

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb Oct 13, 2021

Choose a reason for hiding this comment

iholder101 Oct 13, 2021

Choose a reason for hiding this comment

maiqueb Oct 13, 2021

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb Oct 13, 2021

Choose a reason for hiding this comment

maiqueb Oct 13, 2021

Choose a reason for hiding this comment

iholder101 Oct 13, 2021

Choose a reason for hiding this comment

iholder101 commented Oct 14, 2021

iholder101 commented Oct 14, 2021

rmohr Oct 15, 2021 • edited

Choose a reason for hiding this comment

iholder101 Oct 17, 2021

Choose a reason for hiding this comment

kubevirt-bot commented Oct 17, 2021 • edited

iholder101 commented Oct 17, 2021

rmohr commented Oct 19, 2021

kubevirt-bot commented Oct 19, 2021

xpivarc commented Nov 19, 2021

kubevirt-bot commented Nov 19, 2021

iholder101 commented Oct 13, 2021 •

edited

rmohr Oct 15, 2021 •

edited

kubevirt-bot commented Oct 17, 2021 •

edited