Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reference containerDisks and kernel boot images in reproducible format during migrations #6535

Merged
merged 4 commits into from Oct 22, 2021
Merged

Reference containerDisks and kernel boot images in reproducible format during migrations #6535

merged 4 commits into from Oct 22, 2021

Conversation

rmohr
Copy link
Member

@rmohr rmohr commented Oct 6, 2021

What this PR does / why we need it:

Tags pointing to a container digest can change after a VMI was started (especially :latest). If that happens we pull the wrong containerDisk or kernel boot image in the new pod, leading to potentially corrupt state or to a failed migration because qemu refuses to start on a different disk.

This is a long know issue which is now resolved the following way: Once a migration starts, the migration controller looks at the source pod and extracts the digest of images (always visible in the pod status) and uses that digest instead of the originally user-supplied value on the target pod.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Migrations use digests to reference containerDisks and kernel boot images to ensure disk consistency

@kubevirt-bot 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/L labels Oct 6, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 7, 2021

/retest

@rmohr
Copy link
Member Author

rmohr commented Oct 7, 2021 •
edited

/retest

bazel cache server got restarted.

@rmohr
Copy link
Member Author

rmohr commented Oct 7, 2021

/retest

@rmohr
Copy link
Member Author

rmohr commented Oct 7, 2021

/hold

It seems that if we migrate to a node which already has the image pulled via tag, then even if we pull with shasum, we would see in the status in image the tag and not the shasum. I need to adjust the e2e test to cope with that.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021
@rmohr rmohr force-pushed the reproducible-images-for-migrations branch from 2394a51 to 88f977e Compare October 7, 2021 15:04
@rmohr
Copy link
Member Author

rmohr commented Oct 7, 2021

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 8, 2021

/retest

@rmohr
Copy link
Member Author

rmohr commented Oct 8, 2021

/cc @davidvossel

ready for review.

@rmohr
Copy link
Member Author

rmohr commented Oct 8, 2021

/cc @vasiliy-ul

Maybe you want to do an initial review on this one.

vasiliy-ul
vasiliy-ul reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@vasiliy-ul vasiliy-ul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for pinging me here. This looks good. Just left a minor comment (can be ignored) and a general question.

pkg/container-disk/container-disk.go
diskContainerImage := volume.ContainerDisk.Image
if img, exists := imageIDs[volume.Name]; exists {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: In ExtractImageIDsFromSourcePod the returned map is initialized with empty strings "" for each volume. Maybe it would be safer to check:

Suggested change
if img, exists := imageIDs[volume.Name]; exists {
if img, _ := imageIDs[volume.Name]; len(img) > 0 {

pkg/container-disk/container-disk.go Outdated
if _, exists := imageIDs[key]; !exists {
continue
}
imageIDs[key] = status.ImageID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that ContainerStatus.ImageID is always in the form of name@sha256:<digest>? I wasnt able to find some specific info about that in the docs but after googling a bit it seems that it may depend on the container runtime. E.g. here https://stackoverflow.com/questions/45612310/get-the-image-and-sha-image-id-of-images-in-pod-on-kubernetes-deployment it is

"imageID": "docker://sha256:b8efb18f159bd948486f18bd8940b56fd2298b438229f5bd2bcf4cedcf037448"

If that is the case then can't it cause issues when migrating VMs between nodes with different runtimes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that is a good point. If so it may be necessary to grep for sha256:<> and extract it, but not sure if this is something i would consider stable enough ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, on kind it look like this: imageID: sha256:e422121c9c5f97623245b7e600eeb5e223ee623f21fa04da985ae71057d8d70b

But there seems to be always the sha256 present at least.

Copy link
Member Author

@rmohr rmohr Oct 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So

  • cri-o: the full containerID <registry>/<name>@sha256:<>
  • docker (not directly containerd): docker://sha256:<>
  • containerd: sha256:<>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that yes, the digest is always there. Probably then better to extract it and combine with the image name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. PTAL.

@rmohr
Copy link
Member Author

rmohr commented Oct 8, 2021

/hold

Not fit for all container runtimes.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2021
vasiliy-ul
vasiliy-ul reviewed Oct 8, 2021
View reviewed changes
pkg/container-disk/container-disk.go Outdated
@@ -53,6 +54,9 @@ const KernelBootVolumeName = KernelBootName + "-volume"

const ephemeralStorageOverheadSize = "50M"

var digestRegex = regexp.MustCompile(`sha256:([a-zA-Z0-9]+)`)
var containerBaseRegex = regexp.MustCompile(`^[^@:]+`)
Copy link
Contributor

@vasiliy-ul vasiliy-ul Oct 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... not sure the regexp will work correctly if a port number is specified for the registry: registry:5000/kubevirt/cirros-container-disk-demo:devel.

Also I can specify an image with digest directly in the VMI spec: registry:5000/kubevirt/cirros-container-disk-demo@sha256:<digest>. Maybe worth adding a unit testcase for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it does not :) Also looking for a utility function in our vendor dependencies but did not find one yet.

Copy link
Contributor

@vasiliy-ul vasiliy-ul Oct 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe makes sense to just trim the string from the right up to the first (last since its from the right) : (this will remove the tag or the digest). Then just TrimSuffix("@sha256"). I guess should work. UPD: nah, an image can be specified without tag (assuming 'latest')...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have it now.PTAL.

@rmohr rmohr force-pushed the reproducible-images-for-migrations branch from 0afb870 to d6256d3 Compare October 11, 2021 07:41
vasiliy-ul
vasiliy-ul reviewed Oct 11, 2021
View reviewed changes
Copy link
Contributor

@vasiliy-ul vasiliy-ul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a small question inline.

pkg/container-disk/container-disk.go Outdated
Comment on lines 405 to 411
parts := strings.Split(image, "/")
if strings.Contains(parts[len(parts)-1], "@") {

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm... what is the point of this parts and an empty if? maybe a leftover?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha. Indeed. Fixed.

@rmohr rmohr force-pushed the reproducible-images-for-migrations branch 2 times, most recently from 15cc1ee to d411325 Compare October 11, 2021 08:01
@vasiliy-ul
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 12, 2021

@davidvossel I would love to hear a second opinion on this.

@rmohr
Cope with different container runtimes
7c6231e 
Every container runtime reports the imageID slightly different. However
they all contain the digest in a form which can be extracted via regexp
by searching for `sha256:<digest>`.

If the digest can't be extracted the migration will not proceed to avoid
corrupting guests.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr rmohr force-pushed the reproducible-images-for-migrations branch from d411325 to 7c6231e Compare October 20, 2021 13:20
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 20, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 20, 2021

@vasiliy-ul @davidvossel rebased. PTAL.

@vasiliy-ul
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 21, 2021

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2021
@rmohr
Copy link
Member Author

rmohr commented Oct 21, 2021

/retest

davidvossel
davidvossel reviewed Oct 21, 2021
View reviewed changes
Copy link
Member

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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:

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

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

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

4 similar comments
@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Oct 22, 2021 •
edited

@rmohr: The following tests 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-k8s-1.19-operator 2394a51 link true /test pull-kubevirt-e2e-k8s-1.19-operator
pull-kubevirt-e2e-k8s-1.19-sig-compute 2394a51 link true /test pull-kubevirt-e2e-k8s-1.19-sig-compute
pull-kubevirt-e2e-k8s-1.19-sig-network 2394a51 link true /test pull-kubevirt-e2e-k8s-1.19-sig-network
pull-kubevirt-e2e-arm64 d411325 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.

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit 901f705 into kubevirt:main Oct 22, 2021
@fgimenez fgimenez mentioned this pull request Oct 25, 2021
Merged
@rmohr
Copy link
Member Author

rmohr commented Oct 27, 2021

/cherrypick release-044

@rmohr
Copy link
Member Author

rmohr commented Oct 27, 2021

/cherrypick release-0.44

@kubevirt-bot
Copy link
Contributor

@rmohr: cannot checkout release-044: error checking out release-044: exit status 1. output: error: pathspec 'release-044' did not match any file(s) known to git

In response to this:

/cherrypick release-044

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
Copy link
Contributor

@rmohr: #6535 failed to apply on top of branch "release-0.44":

Applying: Prepare RenderMigrationTarget flows on the template service
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 Prepare RenderMigrationTarget flows on the template service
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.

rmohr pushed a commit to rmohr/kubevirt that referenced this pull request Nov 2, 2021
@kubevirt-bot @rmohr
Merge pull request kubevirt#6535 from rmohr/reproducible-images-for-m…
1c6c593 
…igrations

Reference containerDisks and kernel boot images in reproducible format during migrations

(cherry picked from commit 901f705)
Signed-off-by: Roman Mohr <rmohr@redhat.com>
This was referenced Nov 2, 2021
Closed
Merged
@akalenyu akalenyu mentioned this pull request Nov 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidvossel davidvossel davidvossel left review comments

@vasiliy-ul vasiliy-ul vasiliy-ul left review comments

@ILpinto ILpinto Awaiting requested review from ILpinto

@phoracek phoracek Awaiting requested review from phoracek

Assignees

@vasiliy-ul vasiliy-ul

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rmohr @vasiliy-ul @kubevirt-bot @kubevirt-commenter-bot @davidvossel