-
Notifications
You must be signed in to change notification settings - Fork 393
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.
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
docker: support qemu target builds #1092
docker: support qemu target builds #1092
Conversation
Hi @tormath1. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
images/capi/Dockerfile
Outdated
&& apt-get purge --auto-remove -y \ | ||
&& rm -rf /var/lib/apt/lists/* | ||
|
||
ARG ARCH | ||
ARG PASSED_IB_VERSION | ||
|
||
USER imagebuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to use the default root
user, because we want to use this container image in our pipelines and that's an Azure DevOps requirement. (See #991.)
But using a specific non-root user in a Docker container is still generally considered a "best practice" I believe, so I'd like to hear from others as to whether this change is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But using a specific non-root user in a Docker container is still generally considered a "best practice" I believe, so I'd like to hear from others as to whether this change is acceptable.
That was my conclusion too - as I don't see any other indications to use a non-root user. 🤷 Let's hear from the maintainers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we default it but make it overridable for the use cases it is needed for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @jsturtevant, you mean make it overridable at the build time? It won't be possible at the runtime, as the dependencies (packer, ansible, etc.) are installed in the home directory of the user used during the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to see the user changes in a separate PR. Could we reduce this one to just adding qemu?
images/capi/Dockerfile
Outdated
@@ -17,26 +17,22 @@ | |||
ARG BASE_IMAGE=docker.io/library/ubuntu:latest | |||
FROM $BASE_IMAGE | |||
|
|||
RUN apt-get update && apt-get install -y apt-transport-https build-essential ca-certificates curl git jq python3-pip rsync unzip vim wget \ | |||
&& useradd -ms /bin/bash imagebuilder \ | |||
RUN apt-get update && apt-get install -y apt-transport-https build-essential ca-certificates curl git jq python3-pip rsync unzip vim wget qemu-system-x86 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we also add qemu-kvm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added qemu-kvm
and dropped the user changes.
/ok-to-test |
fe6c970
to
6070aef
Compare
this commit adds qemu amd64 binaries to build qemu images from the Docker image. example: ``` docker run -it --rm -e PACKER_FLAGS="--var 'accelerator=none'" -e OEM_ID=openstack cluster-node-image-builder-amd64 build-qemu-flatcar ``` Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
6070aef
to
6b2a0ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/assign @jsturtevant @kkeshavamurthy
/lgtm |
/approve |
1 similar comment
/approve |
maybe someone in the qemu approvers list need to approve this? |
Pretty sure I can't, but /approve |
Despite what the bot says, I think it needs an approver from https://github.com/kubernetes-sigs/image-builder/blob/master/images/capi/OWNERS, which is just @CecileRobertMichon |
This really shouldn't be the case, see See #1110 for an example of how it was working about two weeks ago. |
Yup top-level approval should 100% be enough. Can we open an issue in e.g. test-infra to get some awareness / maybe someone has time to investigate? (maybe we found a recently introduced bug in the approve plugin) Let's see if that helps for now |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kkeshavamurthy, mdbooth, sbueringer, tormath1 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 |
this commit adds qemu amd64 binaries to build qemu images from the Docker image.
example:
Note: For now it can only run builds without acceleration because it still uses the
imagebuilder
user but asimagebuilder
needs to be in the KVM group and we don't know in advance the group ID ofkvm
group on the host: we would need to build and run the image withroot
user to allow KVM acceleration, this can be done in a follow-up PR.Additional context
We could preserve the
imagebuilder
user by creating it with the uid/gid 0 but in the end it's creating another "root" user...