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

podvm:Enable se image build for s390x rhel podvm #1924

Merged
merged 2 commits into from
Aug 3, 2024
Merged

podvm:Enable se image build for s390x rhel podvm #1924

merged 2 commits into from
Aug 3, 2024

Conversation

Saripalli-lavanya
Copy link
Contributor

@Saripalli-lavanya Saripalli-lavanya commented Jul 17, 2024 •
edited
Loading

This changes help build RHEL Se enabled podvm image with an extra Arg of SE_BOOT=true

Changes Made:-

Updated the build-s390x-se-image.sh script to generate SE PodVM images for both RHEL and Debian.
Used Dracut to update the initramfs on the RHEL machine, incorporating the newly created fstab, crypttab, and keys into the Dracut configuration file.
Revised the Packer configuration file (qemu-rhel.pkr.hcl) to include an additional disk for encryption purposes and to execute a script for enabling SE.
Updated the Makefile, Dockerfile.podvm.rhel, and variables.pkr.hcl to include the SE_BOOT variable option.
Also, as suggested in comments replaced all SE_BOOT=1 with SE_BOOT=true.

Temporary fix:

Due to issue https://gitlab.com/qemu-project/qemu/-/issues/2054, we can't install packages on RHEL s390x base machines with packer. As a temporary fix, we're downloading cryptsetup in a container and copying the binaries to the VM image.

Resulting images :-

[root@hpscc02 seeluser]# podman images
REPOSITORY                                                 TAG         IMAGE ID      CREATED       SIZE
quay.io/confidential-containers/podvm-libvirt-rhel-s390x   sl-final    8368079c41af  10 hours ago  3.04 GB
<none>                                                     <none>      55398315189a  10 hours ago  14.5 GB
quay.io/confidential-containers/podvm-binaries-rhel-s390x  sl-final    da36a893ed7e  10 hours ago  396 MB
<none>                                                     <none>      84685cf5f221  10 hours ago  15.4 GB
quay.io/confidential-containers/podvm-builder-rhel         sl-final    109f2a1c4338  11 hours ago  7.1 GB
registry.access.redhat.com/ubi9/ubi                        9.4         c78647ae33b8  11 days ago   217 MB

se-enabled podvm qcow2 image was able to bring up workload on OCP

[root@bastion-sl ~]# oc get pods -n default
NAME             READY   STATUS    RESTARTS   AGE
osc-caa-commit   1/1     Running   0          20m
[root@bastion-sl ~]#

below are the logs from libvirt peerpod deamon for reference which says LaunchSecurityType: S390PV

2024/07/30 03:28:43 [adaptor/cloud/libvirt] LaunchSecurityType: S390PV
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Checking if instance (podvm-osc-caa-commit-3c11b24d) exists
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Uploaded volume key /var/lib/libvirt/images/test-se-anj-dir/podvm-osc-caa-commit-3c11b24d-root.qcow2
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Create cloudInit iso
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Uploading iso file: podvm-osc-caa-commit-3c11b24d-cloudinit.iso
2024/07/30 03:28:43 [adaptor/cloud/libvirt] 47104 bytes uploaded
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Volume ID: /var/lib/libvirt/images/test-se-anj-dir/podvm-osc-caa-commit-3c11b24d-cloudinit.iso
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Create XML for 'podvm-osc-caa-commit-3c11b24d'
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Creating VM 'podvm-osc-caa-commit-3c11b24d'
2024/07/30 03:28:43 [adaptor/cloud/libvirt] Starting VM 'podvm-osc-caa-commit-3c11b24d'
2024/07/30 03:28:43 [adaptor/cloud/libvirt] VM id 1070
2024/07/30 03:29:05 [adaptor/cloud/libvirt] Instance created successfully
2024/07/30 03:29:05 [adaptor/cloud/libvirt] created an instance podvm-osc-caa-commit-3c11b24d for sandbox 3c11b24dca6d9ca7076eed23915ec8a1b15eb4b2a3d5f992b40ad2a5ec9c462c
2024/07/30 03:29:05 [util/k8sops] osc-caa-commit is now owning a PeerPod object

@Saripalli-lavanya Saripalli-lavanya force-pushed the rhel-se branch 2 times, most recently from 48e4f21 to ff58e4a Compare July 18, 2024 05:37
@Saripalli-lavanya Saripalli-lavanya marked this pull request as ready for review July 18, 2024 05:38
@Saripalli-lavanya Saripalli-lavanya force-pushed the rhel-se branch 7 times, most recently from 51881f9 to d472c6a Compare July 22, 2024 10:55
@stevenhorsman
Copy link
Member

@Saripalli-lavanya - it looks like you are still actively pushing to this PR frequently. Is in in progress and therefore should be marked as draft, or is it ready for review now?

@stevenhorsman stevenhorsman marked this pull request as draft July 22, 2024 11:53
@Saripalli-lavanya Saripalli-lavanya force-pushed the rhel-se branch 3 times, most recently from 1d2a173 to 4e4f390 Compare July 24, 2024 09:12
@Saripalli-lavanya Saripalli-lavanya marked this pull request as ready for review July 24, 2024 09:12
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Jul 24, 2024
@Saripalli-lavanya Saripalli-lavanya force-pushed the rhel-se branch 3 times, most recently from dcad2ab to a1bbac2 Compare July 25, 2024 11:57
@Saripalli-lavanya
Copy link
Contributor Author

Hi @stevenhorsman, @snir911, Thank you for the review. I have updated the code. could you please re-review.

stevenhorsman
stevenhorsman reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Can we get more information in the commit message about some of the changes and why they are required?
I can see that some are adding RHEL versions of existing debian commands, but there are extra dracut things and qemuargs?
Thanks

@Saripalli-lavanya
Copy link
Contributor Author

Sure @stevenhorsman, i've updated the commit message to provide more details. Please review the updated commit message, and let me know if further adjustments are needed. Thank you

@Saripalli-lavanya Saripalli-lavanya force-pushed the rhel-se branch 5 times, most recently from d1fb986 to 56e3cc3 Compare July 29, 2024 05:35
liudalibj
liudalibj approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@liudalibj liudalibj left a comment

Choose a reason for hiding this comment

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

the codes change overall LGTM, I built out the se-rhel image from one lpar.
only one thing, please fix the commit body too long issue.

snir911
snir911 approved these changes Jul 30, 2024
View reviewed changes
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, built also on x86, e2e test was failing, not sure if it's related, i re-run

@Saripalli-lavanya
Copy link
Contributor Author

As suggested tried reducing commit message length and pushed code after re-basing code with main branch.

bpradipt
bpradipt approved these changes Jul 30, 2024
View reviewed changes
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

@bpradipt
Copy link
Member

@liudalibj @snir911 @Saripalli-lavanya can you please check the e2e failure.. Not sure what's causing it..

@bpradipt bpradipt added the hold label Jul 30, 2024
@bpradipt
Copy link
Member

Looks like simple pod creation tests are failing as well.. @Saripalli-lavanya can you please try running libvirt tests on x86 in your setup ?

@Saripalli-lavanya
Copy link
Contributor Author

Looks like simple pod creation tests are failing as well.. @Saripalli-lavanya can you please try running libvirt tests on x86 in your setup ?

Sure @bpradipt

@bpradipt
Copy link
Member

The failure is due to missing auth.json. May be fixed by #1964 ?

@Saripalli-lavanya
Copy link
Contributor Author

@bpradipt I noticed libvirt test cases passed with the fix in that PR. so is this PR okay to go?

@bpradipt bpradipt removed the hold label Jul 30, 2024
@bpradipt
Copy link
Member

Waiting for #1964 to be merged and re-running the e2e test once before merging this one.

@liudalibj
Copy link
Member

@Saripalli-lavanya Can we replace all SE_BOOT=1 with SE_BOOT=true? check other codes, it seems that true/false is wide used than 1/0

@bpradipt
Copy link
Member

@Saripalli-lavanya Can we replace all SE_BOOT=1 with SE_BOOT=true? check other codes, it seems that true/false is wide used than 1/0

good suggestion.. @Saripalli-lavanya can you please make this minor change.

Rest looks good. The e2e passed successfully as well.

@bpradipt bpradipt removed the test_e2e_libvirt Run Libvirt e2e tests label Jul 31, 2024
@Saripalli-lavanya
Copy link
Contributor Author

hi @liudalibj , i have replaced all SE_BOOT=1 to SE_BOOT=true. could you please take a look and suggest. Thank you

@savitrilh
Copy link

@bpradipt Lavanya made the changes. Kindly merge the PR

huoqifeng
huoqifeng approved these changes Aug 1, 2024
View reviewed changes
Copy link

@huoqifeng huoqifeng left a comment

Choose a reason for hiding this comment

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

LGTM

@bpradipt
Copy link
Member

bpradipt commented Aug 1, 2024

@bpradipt Lavanya made the changes. Kindly merge the PR

Unfortunately the tree is in read-only mode. This might have to wait a bit.

Saripalli-lavanya and others added 2 commits August 2, 2024 15:35
@Saripalli-lavanya @ANJANA-A-R-K
Podvm:Enable se image build for rhel
d8c5ee2 
Made changes to build RHEL Se enabled podvm image with an extra Arg of SE_BOOT=true

Signed-off-by: ANJANA-A-R-K <ANJANA.A.R.K1@ibm.com>

Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
Co-Authored-By: ANJANA-A-R-K <149779123+ANJANA-A-R-K@users.noreply.github.com>
@Saripalli-lavanya @ANJANA-A-R-K
podvm: Temporary fix for installing cryptsetup in SE-enabled PodVM image
1690bd8 
As a temporary fix, we're downloading cryptsetup in a container and copying the binaries to the VM image.

Signed-off-by: ANJANA-A-R-K <ANJANA.A.R.K1@ibm.com>

Signed-off-by: Saripalli Lavanya <Saripalli.Lavanya@ibm.com>
Co-Authored-By: ANJANA-A-R-K <149779123+ANJANA-A-R-K@users.noreply.github.com>
@bpradipt
Copy link
Member

bpradipt commented Aug 2, 2024

@Saripalli-lavanya are there more changes needed ?

@Saripalli-lavanya
Copy link
Contributor Author

@Saripalli-lavanya are there more changes needed ?

No @bpradipt, no more changes needed. Thank you.

@bpradipt
Copy link
Member

bpradipt commented Aug 2, 2024

@Saripalli-lavanya are there more changes needed ?

No @bpradipt, no more changes needed. Thank you.

Since you pushed new changes post review, would you mind mentioning the new changes just for awareness ..

@bpradipt bpradipt merged commit 1229a2b into confidential-containers:main Aug 3, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenhorsman stevenhorsman stevenhorsman left review comments

@liudalibj liudalibj liudalibj approved these changes

@huoqifeng huoqifeng huoqifeng approved these changes

@snir911 snir911 snir911 approved these changes

@bpradipt bpradipt bpradipt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Saripalli-lavanya @stevenhorsman @liudalibj @bpradipt @savitrilh @snir911 @huoqifeng