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: build rhel peer pod VM qcow2 image for s390x #1531

Merged

Conversation

redobed
Copy link
Contributor

@redobed redobed commented Oct 20, 2023

This PR updates the podvm/Dockerfile.podvm_binaries.rhel and podvm/Dockerfile.podvm_builder.rhel to enable building a rhel based peer pod VM image qcow2 image for s390x. Packer binary is conditionally built from source s390x. The PR also allows building the Builder image either on a subscribed or an unsubscribed rhel host.

Signed-off-by: Obed Tetteh obed.tetteh@ibm.com

@redobed
Copy link
Contributor Author

redobed commented Oct 20, 2023

@jensfr
@bpradipt

Copy link
Contributor

@CarstenLeue CarstenLeue left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@snir911 snir911 left a comment

Choose a reason for hiding this comment

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

Thanks!
I added some comments, however, while review it seems to me like many changes should have been done in the builder container Dockerfile rather than here.
So I'd generally check what can be moved there (unless i missed something and for some reason you have to do it here)

@@ -17,7 +17,11 @@ ARG AA_KBC="offline_fs_kbc"
ARG ARCH
ARG CAA_SRC
ARG CAA_SRC_REF
ARG IMAGE_URL="/tmp/rhel.qcow2"
ARG IMAGE_CHECKSUM
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's needed in the binaries dockerfile, only in the podvm one, and it's already being set there, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required by this line in the binaries dockerfile

Copy link
Contributor

@snir911 snir911 Oct 24, 2023

Choose a reason for hiding this comment

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

I think the right approch will be to add a default dummy link as done for x86 see here and here
(which is https://access.redhat.com/downloads/content/433/ver=/rhel---9/9.3%20Beta/s390x/product-software & d436ffeac7d468f75526ca8b8bab1cba64ba3ab2889ccd11fa107a7323b267db i suppose)

podvm/Dockerfile.podvm_builder.rhel Show resolved Hide resolved
podvm/Dockerfile.podvm_builder.rhel Outdated Show resolved Hide resolved
podvm/Dockerfile.podvm_builder.rhel Show resolved Hide resolved
podvm/Dockerfile.podvm_builder.rhel Show resolved Hide resolved
@redobed
Copy link
Contributor Author

redobed commented Oct 23, 2023

Thanks! I added some comments, however, while review it seems to me like many changes should have been done in the builder container Dockerfile rather than here. So I'd generally check what can be moved there (unless i missed something and for some reason you have to do it here)

@snir911 thanks for the review.
The changes were mostly done in that very file.
The binaries file only had:

ARG IMAGE_URL="/tmp/rhel.qcow2"
ARG IMAGE_CHECKSUM

ENV IMAGE_URL ${IMAGE_URL}
ENV IMAGE_CHECKSUM ${IMAGE_CHECKSUM}

which are required for the make target

podvm/Dockerfile.podvm_builder.rhel Outdated Show resolved Hide resolved
podvm/Dockerfile.podvm_builder.rhel Outdated Show resolved Hide resolved
podvm/Dockerfile.podvm_builder.rhel Outdated Show resolved Hide resolved
Copy link
Contributor

@snir911 snir911 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM overall add one comment to address
also it seems the latter commit "podvm: align commit with other files" is empty, pls remove it


# set a correspond qemu-system-* named link to qemu-kvm
# set a corrspond qemu-system-* named link to qemu-kvm
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo

subscription-manager register --org=${ORG_ID} --activationkey=${ACTIVATION_KEY}; \
fi

RUN subscription-manager repos --enable codeready-builder-for-rhel-9-${ARCH}-rpms; \
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC codeready expects x86_64 rather than amd64, how about treating the inconsistency using the following:

users will set only the ARCH value as done in other files:
ARG ARCH=x86_64 # set x86_64 or s390x
ENV ARCH=${ARCH}

then:
for protoc use: ${ARCH/s390x/s390_64}
for codeready use: ${ARCH}
for golang use: ${ARCH/x86_64/amd64}
for yq use: ${ARCH/x86_64/amd64}
for packer use: ${ARCH}

Copy link
Contributor

@snir911 snir911 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks @redobed !
I think some of the commits needs to be squashed

@bpradipt
Copy link
Member

@redobed please squash the commits and also remove the merge commits.
Here is a good tutorial on squashing the commits with git - https://www.git-tower.com/learn/git/faq/git-squash/

@tumberino
Copy link
Contributor

@redobed I don't think the merge from main kept the correct changes, i.e. it is undoing changes done as part of #1448

@redobed
Copy link
Contributor Author

redobed commented Nov 6, 2023

@redobed please squash the commits and also remove the merge commits. Here is a good tutorial on squashing the commits with git - https://www.git-tower.com/learn/git/faq/git-squash/

@bpradipt commit are squashed

@redobed
Copy link
Contributor Author

redobed commented Nov 6, 2023

@redobed I don't think the merge from main kept the correct changes, i.e. it is undoing changes done as part of #1448

@jtumber-ibm resolved

@redobed redobed force-pushed the peerpod-vm-image-s390x branch 2 times, most recently from e302e8b to 5b92633 Compare November 20, 2023 19:07
This PR updates the podvm dockerfiles to enable building a peerpod QCOW2 image for s390x.
It supports building on either a subscribed or an unsubscribed RHEL. Packer for s390x
is built from source

Signed-off-by: Obed Tetteh <obed.tetteh@ibm.com>
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
Thanks @redobed

@bpradipt bpradipt merged commit 8577093 into confidential-containers:main Nov 24, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants