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.

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

Podvm builder version removal #2008

Conversation

stevenhorsman
Copy link
Member

@stevenhorsman stevenhorsman commented Aug 15, 2024

Just before the 0.9.0 release we found that the rust version in
the builder image hadn't been updated when the versions.yaml
was, which caused issues which certain builds that weren't in our
CI tests, so had to create #1939 to fix it

In there I discussed the idea that if we remove the defaults from the Dockerfiles and enforce them to be specified, we
can avoid this happening and stop having to maintain versions in multiple places.

@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Aug 15, 2024
@@ -53,12 +51,12 @@ RUN chmod a+x /usr/local/bin/yq && \
rm -f go${GO_VERSION}.linux-${YQ_ARCH}.tar.gz
ENV PATH="/usr/local/go/bin:${PATH}"

# Install packer. Packer doesn't does not have prebuilt s390x arch binaries above Packer version 0.1.5
# Install packer. Packer doesn't does not have prebuilt s390x arch binaries above Packer version 0.1.5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Install packer. Packer doesn't does not have prebuilt s390x arch binaries above Packer version 0.1.5
# Install packer. Packer doesn't have prebuilt s390x arch binaries beyond Packer version 0.1.5

ARG PROTOC_VERSION
ARG RUST_VERSION
ARG YQ_VERSION
ARG YQ_CHECKSUM
Copy link
Member

Choose a reason for hiding this comment

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

PACKER_VERSION ?

Copy link
Member

Choose a reason for hiding this comment

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

ok.. I see it's only needed for rhel

ARG PROTOC_VERSION
ARG RUST_VERSION
ARG YQ_VERSION
ARG PACKER_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

do you need ENV setting for PACKER_VERSION later in the code?

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'm not sure if it's strictly required as that seems to be GHA related and I don't see us ever building RHEL in the open-source pipelines, but I can add for consistency

@stevenhorsman
Copy link
Member Author

Hmm, some of the e2e test are failing in the CI, which I think might be an environmental things as they initially pass and then once one fails the rest also all fail. I've tried running this locally with the same podvm and everything passed here, so I'm not sure what is the problem

Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

```bash
$ docker build -t podvm_builder \
-f Dockerfile.podvm_builder .
$ make podvm-builder
Copy link
Member

Choose a reason for hiding this comment

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

Hi @stevenhorsman !

Make should be executed from parent folder, so maybe make -C .. podvm-builder

Copy link
Member Author

@stevenhorsman stevenhorsman Sep 9, 2024

Choose a reason for hiding this comment

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

So in the line above I mentioned having to run it in the

src/cloud-api-adaptor folder
but I can swap to this approach if you prefer (and I'll update Beraldo's recent change to match)?

Copy link
Member

Choose a reason for hiding this comment

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

that's ok, thanks Steve!

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 had to rebase to resolve conflicts, so I updated this. This will change when we ditch packer for mkosi anyway!

-f Dockerfile.podvm_builder .
e.g. to produce an s390x architecture builder image
```
ARCH=s390x make podvm-builder
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Just before the 0.9.0 release we found that the rust version in
the builder image hadn't been updated when the `versions.yaml`
was, which caused issues which certain builds that weren't in our
CI tests.

If we remove the defaults and enforce them to be specified, we
can avoid this happening and stop having to maintain versions in
multiple places.

See confidential-containers#1939 for the example

Signed-off-by: stevenhorsman <steven@uk.ibm.com>
Similar to confidential-containers#1990, update the podvm_builder documentation
to use the `make` command, rather than the docker file directly
to hide the complexity and allow the versions to be picked
up automatically

Signed-off-by: stevenhorsman <steven@uk.ibm.com>
@wainersm wainersm merged commit 453846c into confidential-containers:main Sep 10, 2024
28 checks passed
@stevenhorsman stevenhorsman deleted the podvm-builder-version-removal branch September 10, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants