-
Notifications
You must be signed in to change notification settings - Fork 79
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: update qemu for rhel s390x #1741
podvm: update qemu for rhel s390x #1741
Conversation
4ee8ace
to
22da7b0
Compare
@Saripalli-lavanya - can you post the command you used (I guess |
22da7b0
to
883fe87
Compare
Hi @stevenhorsman , i've used below commands as per redhat doc here. on an s390x machine.
|
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, I think @wyuany and @Saripalli-lavanya both tested it on s390x OCP. Thanks! @Saripalli-lavanya
Thank you @huoqifeng, |
podvm/qcow2/rhel/qemu-rhel.pkr.hcl
Outdated
@@ -14,12 +14,13 @@ source "qemu" "rhel" { | |||
iso_checksum = "${var.cloud_image_checksum}" | |||
iso_url = "${var.cloud_image_url}" | |||
output_directory = "output" | |||
qemuargs = [["-m", "${var.memory}"], ["-smp", "cpus=${var.cpus}"], ["-cdrom", "${var.cloud_init_image}"], ["-serial", "mon:stdio"], ["-cpu", "Cascadelake-Server"]] |
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 might be worth noting that the
So I think #1531 was not valid. |
b8cea41
to
f7e84bc
Compare
Hi @stevenhorsman , could you please try with the latest code using below command? for s390x :-
for x86_64:-
|
@@ -14,12 +14,13 @@ source "qemu" "rhel" { | |||
iso_checksum = "${var.cloud_image_checksum}" | |||
iso_url = "${var.cloud_image_url}" | |||
output_directory = "output" | |||
qemuargs = [["-m", "${var.memory}"], ["-smp", "cpus=${var.cpus}"], ["-cdrom", "${var.cloud_init_image}"], ["-serial", "mon:stdio"], ["-cpu", "Cascadelake-Server"]] | |||
qemuargs = [["-m", "${var.memory}"], ["-smp", "cpus=${var.cpus}"], ["-cdrom", "${var.cloud_init_image}"], ["-serial", "mon:stdio"]] |
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.
Has this also been tested on Intel system? I recall issues when not specifying the cpu model
This doesn't work for me, I assume it's something to do with needing a license server key? |
ohh my bad, I missed that point we need to use RHEL subscribed machine or below command will work provided ORG_ID & ACTIVATION_KEY
|
I have the pdovm builder working with docker, not podman, for both s390x and x86, but I'm not really happy about overriding all the build-args. We have a versions.yaml that is supposed to set the go, rust etc versions, that are supported and tested with the latest code, so I'd rather we get this working in the standard way with |
a44e85c
to
89c644b
Compare
@huoqifeng, @stevenhorsman, @snir911, @bpradipt Thank you for reviewing the PR, Could you please review the latest changes and suggest? @stevenhorsman with these latest changes I was able to build image using command |
Test Results for amd64 podvm image:
|
I managed to run:
I've not got an environment to test it though. It's definitely better than it was, but the user experience is quite tricky to setting this up and there isn't any doc, but most of it is linked to the RHEL licensing issue, so I'm not sure if we should be worrying about this upstream. |
fda85ec
to
4b5a884
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 overall, thanks! added some comments for your consideration
@@ -14,14 +14,15 @@ source "qemu" "rhel" { | |||
iso_checksum = "${var.cloud_image_checksum}" | |||
iso_url = "${var.cloud_image_url}" | |||
output_directory = "output" | |||
qemuargs = [["-m", "${var.memory}"], ["-smp", "cpus=${var.cpus}"], ["-cdrom", "${var.cloud_init_image}"], ["-serial", "mon:stdio"], ["-cpu", "Cascadelake-Server"]] | |||
qemuargs = [["-m", "${var.memory}"], ["-smp", "cpus=${var.cpus}"], ["-cdrom", "${var.cloud_init_image}"], ["-serial", "mon:stdio"], ["-cpu", "${var.cpu_type}"]] |
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.
nit, if this cpu types are usually constants for the architectures, you can do something like the following instead:
cpu_model = "${var.os_arch}" == "x86_64" ? "Cascadelake-Server" : "max"
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 tried it but seems like there is a failure. I also tried passing os_arch = s390x as a variable, seems no luck
@@ -22,6 +22,7 @@ SOURCES := $(shell find $(SOURCEDIRS) -name '*.go' -print) | |||
TEST_E2E_TIMEOUT ?= 60m | |||
|
|||
RESOURCE_CTRL ?= true | |||
YQ_CHECKSUM_${ARCH} ?= $(YQ_CHECKSUM) |
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.
would it make sense to drop YQ_CHECKSUM in favor of YQ_CHECKSUM_x86_64 and YQ_CHECKSUM_s390x?
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.
Hi @snir911, Line 10 substitutes 'x86_64' with 'amd64' in the 'ARCH' value if it's not provided, potentially introducing inconsistency? Could you please suggest
ARCH ?= $(subst x86_64,amd64,$(shell uname -m))
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.
That's odd, isn't the Docker file always expects x86_64? we are passing it amd64
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.
Yeah, Docker file for RHEL always expects x86_64, for now i have updated code to use YQ_CHECKSUM_x86_64.
But, when i don't provide Arch value as x86_64, it is getting substituted by amd64 due to line-10
PODVM_DISTRO=rhel make podvm-builder podvm-binaries podvm-image PODVM_TAG=latest ORG_ID=*** ACTIVATION_KEY="***" CLOUD_PROVIDER=libvirt IMAGE_URL="rhel-9.2-x86_64-kvm.qcow2"
make: sha256sum: Command not found
docker buildx build -t quay.io/confidential-containers/podvm-builder-rhel:latest -f podvm/Dockerfile.podvm_builder.rhel \
--build-arg GO_VERSION=1.21.8 \
--build-arg ORG_ID=*** \
--build-arg ARCH=amd64 \
--build-arg ACTIVATION_KEY=*** \
--build-arg PACKER_VERSION=v1.9.4 \
--build-arg PROTOC_VERSION=3.15.0 \
--build-arg RUST_VERSION=1.72.0 \
--build-arg YQ_VERSION=v4.35.1 \
--build-arg YQ_CHECKSUM= \
--load .
make: *** [podvm-builder] Error 1
saripallilavanya@Lavanyas-MacBook-Pro cloud-api-adaptor %
Please suggest here.
4b5a884
to
6029f09
Compare
d3cee98
to
28c2c1d
Compare
Variable substitution is failing with ADD in docker file Fixes: confidential-containers#1740 Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
QEMU plugin installation for RHEL x86 podvm image build. Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
28c2c1d
to
32f8e4c
Compare
This change will help fix checksum failure issue & perform checksum work with Docker and Podman. Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
This fix streamlines RHEL PodVM image creation via Makefile integration with version.yaml. Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
32f8e4c
to
f8cdb2d
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.
I was able to get the build working for rhel and the tests are passing, so from my side this is okay to merge. Thanks
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
Thanks @Saripalli-lavanya
Packer is picking qemu-system-x86 instead of s390x as qemu binaries for rhel podvm image build
Fixes: #1740