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

Create /usr/lib64/qemu-kvm with proper permissions #8433

Merged
merged 1 commit into from
Sep 10, 2022
Merged

Create /usr/lib64/qemu-kvm with proper permissions #8433

merged 1 commit into from
Sep 10, 2022

Conversation

vasiliy-ul
Copy link
Contributor

What this PR does / why we need it:

Workaround for moby/moby#44106

Need to create the directory upfront, otherwise it gets assigned wrong permissions when unpacked.

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 #8195, fixes #8362

Special notes for your reviewer:

The same directory with qemu modules is present in the aarch64 image. Applied the workaround there as well.

Release note:

NONE

@vasiliy-ul
Create /usr/lib64/qemu-kvm with proper permissions
bb18331 
Workaround for moby/moby#44106

Need to create the directory upfront, otherwise it gets assigned wrong
permissions when unpacked.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Sep 8, 2022
@kubevirt-bot kubevirt-bot added the kind/build-change Categorizes PRs as related to changing build files of virt-* components label Sep 8, 2022
@alicefr
Copy link
Member

alicefr commented Sep 8, 2022 •
edited
Loading

/lgtm
Make sense to me! Thanks :)

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022
@rmohr
Copy link
Member

rmohr commented Sep 9, 2022

/approve
/retest

@kubevirt-bot
Copy link
Contributor

[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:

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 Sep 9, 2022
@xpivarc
Copy link
Member

xpivarc commented Sep 9, 2022

Makes sense to me. This is what I see when I inspect the layers
---------- 0:0 258 kB │ │ ├── qemu-kvm

@vasiliy-ul
Copy link
Contributor Author

/retest-required

1 similar comment
@vasiliy-ul
Copy link
Contributor Author

/retest-required

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Sep 9, 2022 •
edited
Loading

@vasiliy-ul: 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-goveralls bb18331 link false /test pull-kubevirt-goveralls

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.

@vasiliy-ul
Copy link
Contributor Author

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

@kubevirt-commenter-bot
Copy link

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

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required 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 9f5ba1e into kubevirt:main Sep 10, 2022
@vasiliy-ul vasiliy-ul deleted the fix-qemu-kvm-modules-dir-perms branch September 12, 2022 05:26
@poojaghumre
Copy link

Thanks for the fix @vasiliy-ul! Is this something that will get backported to older releases at this point? We're currently on v0.55, does this require upgrading to v0.57 once its out with your fix?

@vasiliy-ul
Copy link
Contributor Author

vasiliy-ul commented Sep 14, 2022 •
edited
Loading

@poojaghumre, this fix hasn't landed in v0.57.0, unfortunately. It will be included only in the next v0.58.0 release. Though, it's not hard to backport it to previous versions. Hm... @rmohr, @xpivarc, maybe you could suggest if we could release a patch version v0.55.1 with that workaround? Cherry-picking the commit should be trivial.

@rmohr
Copy link
Member

rmohr commented Sep 14, 2022

sure, we can do that.

/cherry-pick release-0.57

@kubevirt-bot
Copy link
Contributor

@rmohr: new pull request created: #8470

In response to this:

sure, we can do that.

/cherry-pick release-0.57

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 kubevirt-bot mentioned this pull request Sep 14, 2022
Merged
@poojaghumre
Copy link

Thanks to you both for a quick backport!
Are we planning to do this for prior impacted releases too though?

@kubevirt-bot kubevirt-bot mentioned this pull request Sep 26, 2022
Merged
@shweta50
Copy link

@vasiliy-ul can we backport this to v0.55 as well?

@vasiliy-ul
Copy link
Contributor Author

/cherry-pick release-0.55

@kubevirt-bot kubevirt-bot mentioned this pull request Sep 27, 2022
Merged
@kubevirt-bot
Copy link
Contributor

@vasiliy-ul: new pull request created: #8526

In response to this:

/cherry-pick release-0.55

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.

@shweta50
Copy link

Thank you @vasiliy-ul !

@neersighted
Copy link

neersighted commented Sep 27, 2022 •
edited
Loading

Hey all, just wanted to make sure you're aware of the conclusions reached in moby/moby#44106 (moby/moby#44106 (comment) to be exact) -- essentially, permissions for 'implicit' directories are runtime-defined behavior as they are not captured in the OCI spec. Moby has been updated to behave more consistently here, but if you need that directory to be 0755, the only way to ensure it is so on a OCI image spec compliant runtime is to define it yourself (as you have done).

@vasiliy-ul
Copy link
Contributor Author

@neersighted, thank you for looking into this issue and for making it consistent further on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhiller dhiller Awaiting requested review from dhiller

@xpivarc xpivarc Awaiting requested review from xpivarc

Assignees

@alicefr alicefr

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. kind/build-change Categorizes PRs as related to changing build files of virt-* components lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failed to start QEMU binary /usr/libexec/qemu-kvm for probing Missing tcg support
9 participants
@vasiliy-ul @alicefr @rmohr @kubevirt-bot @xpivarc @kubevirt-commenter-bot @poojaghumre @shweta50 @neersighted