Add libguestfs workaround for ppc64le by ravanelli · Pull Request #2714 · coreos/coreos-assembler

ravanelli · 2022-02-17T21:30:58Z

Add a workaround for the issue:
openshift/os#720

The kola tests failing are using guestfish, for some
reason the libguestfs is not able to create the wrapped to
add the vmst value, required for the POWER8 cluster.
Until we can debug it better, let's unlock the power builds

Signed-off-by: Renata Ravanelli rravanel@redhat.com

cgwalters · 2022-02-17T21:37:45Z

+	exec qemu-system-ppc64 "$@" -machine pseries,accel=kvm:tcg,vsmt=8,cap-fwnmi=off
+	EOF
+    chmod +x "${qemu_wrapper}"
+    export LIBGUESTFS_HV="${qemu_wrapper}"


I think we need to set this variable at runtime too, not just build time. (I mean, this file is the build of coreos-assembler itself, but we need to set it when coreos-assembler is performing a build of FCOS/RHCOS)

So if we do the above of installing the file literally, then I think what we want to do is set this environment variable like:
export LIBGUESTFS_HV=/usr/lib/coreos-assembler/libguestfs-ppc64-wrapper.sh
inside cmdlib.sh or so?

Does that make sense?

Though note kola doesn't use cmdlib.sh. So we'll want to set the environment like this does:

coreos-assembler/mantle/platform/qemu.go

Line 659 in 189d179

cmd.Env = append(os.Environ(), "LIBGUESTFS_BACKEND=direct")

Ahh right. Though, now I'm confused why cosa build doesn't also fail...oh, but I bet it would if we went to build another artifact; we only run libguestfs when we go to change ignition.platform.id.

Hmm. It's kind of tempting to by default produce an "unstamped" artifact and force libguestfs into the default build path too.

(And deduplicate the libguestfs environment variables between build and kola)

I totally forgot the export won't persist in the pod creation. Thanks @cgwalters and @jlebon

cgwalters · 2022-02-17T21:37:53Z

    prepare_git_artifacts "${srcdir}" /cosa/coreos-assembler-git.tar.gz /cosa/coreos-assembler-git.json
 }

+libguestfish_wrapper(){


This is totally fine as is...but we could probably just stick this file literally into the source code as e.g. src/libguestfs-ppc64-wrapper.sh, have it get installed by the build system as /usr/lib/coreos-assembler/libguestfs-ppc64-wrapper.sh.

jlebon · 2022-02-17T22:00:38Z

The funny thing is kola actually has a re-implementation of those args already here:

coreos-assembler/mantle/platform/qemu.go

Lines 1090 to 1091 in 189d179

    
           "qemu-system-ppc64", 
        
           "-machine", "pseries,kvm-type=HV,vsmt=8,cap-fwnmi=off," + accel,

As a follow-up, would be good to dedupe this somehow with what's in libguestfish.sh. Maybe a new kola get-qemu-base-args or something that the shell can call into?

cgwalters

This LGTM as is, but...I suspect we will need to fix both kola and the build side.

For example does e.g. cosa buildextend-openstack work?

If it doesn't, I think we probably want a patch to cmdlib.sh that does this same export.

cgwalters

But this is my pre-approval for that, let's get this merged once we have both the build side and kola working!

ravanelli · 2022-02-17T22:33:39Z

Running cosa buildextend-openstack test here to validade it.

ravanelli · 2022-02-17T22:48:17Z

@cgwalters cosa buildextend-openstack finished just fine.

jlebon · 2022-02-17T23:12:54Z

 	guestfishArgs = append(guestfishArgs, "-a", diskImagePath)
 	cmd := exec.Command("guestfish", guestfishArgs...)
 	cmd.Env = append(os.Environ(), "LIBGUESTFS_BACKEND=direct")
+	cmd.Env = append(os.Environ(), "LIBGUESTFS_HV=/usr/lib/coreos-assembler/libguestfs-ppc64le-wrapper.sh")


This needs to be conditional on the arch.

jlebon · 2022-02-17T23:16:35Z

I think the patch for the shell side would just be

diff --git a/src/libguestfish.sh b/src/libguestfish.sh
index fef7285ca..82cfcf86e 100755
--- a/src/libguestfish.sh
+++ b/src/libguestfish.sh
@@ -12,14 +12,7 @@ export LIBGUESTFS_BACKEND=direct
 arch=$(uname -m)

 if [ "$arch" = "ppc64le" ] ; then
-    tmp_qemu_wrapper=$(mktemp -tdp /tmp gf-vsmt.XXXXXX)
-    qemu_wrapper=${tmp_qemu_wrapper}/qemu-wrapper.sh
-       cat <<-'EOF' > "${qemu_wrapper}"
-       #!/bin/bash -
-       exec qemu-system-ppc64 "$@" -machine pseries,accel=kvm:tcg,vsmt=8,cap-fwnmi=off
-       EOF
-    chmod +x "${qemu_wrapper}"
-    export LIBGUESTFS_HV="${qemu_wrapper}"
+    export LIBGUESTFS_HV="/usr/lib/coreos-assembler/libguestfs-ppc64le-wrapper.sh"
 fi

 # http://libguestfs.org/guestfish.1.html#using-remote-control-robustly-from-shell-scripts
@@ -37,9 +30,6 @@ coreos_gf_launch() {

 _coreos_gf_cleanup () {
     guestfish --remote -- exit >/dev/null 2>&1 ||:
-    if [ -n "${tmp_qemu_wrapper:-}" ] ; then
-        rm -rf "${tmp_qemu_wrapper}";
-    fi
 }
 trap _coreos_gf_cleanup EXIT

?

But cool to do in a follow-up too!

Add a workaround for the issue: openshift/os#720 The kola tests failing are using guestfish, for some reason the libguestfs is not able to create the wrapped to add the vmst value, required for the POWER8 cluster. Until we can debug it better, let's unlock the power builds Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

ravanelli · 2022-02-21T13:35:36Z

/retest-required

ravanelli · 2022-02-21T14:10:46Z

@jlebon +1. Let me debug it a little bit more, to see why this part of the code is not being execute at all. I will do a follow-up once I got it.

miabbott

I believe ci/prow/rhcos is blocked on openshift/os#718 which is blocked on openshift/release#26334

If this passes locally, we shouldn't block on that CI job

miabbott · 2022-02-21T20:16:51Z

/override ci/prow/rhcos

openshift-ci · 2022-02-21T20:16:52Z

@miabbott: Overrode contexts on behalf of miabbott: ci/prow/rhcos

Details

In response to this:

/override ci/prow/rhcos

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.

ravanelli · 2022-02-22T13:19:56Z

@jlebon I just figure out the issue. Looks Power never really used the newGuestfish. With the new tests now using setupPreboot that was recently added, the issue happened because there is no call for the Power wrapper in the newGuestfish. This patch fixed it indeed. But I'm opening a new one to clean up the code in libguestfish.sh as you suggested before.

- It is a follow up for issue coreos#720 and for coreos#2714 - POWER never really used the newGuestfish function in mantle/platform/qemu.go, with the new tests now using setupPreboot function that was recently added, the issue happened because there is no call for the POWER wrapper in the newGuestfish. Nonetheless, it is not a libguestfish issue since POWER8 (cluster) needs the vsmt set to 8. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

- It is a follow up for issue coreos#2714 and for coreos#2714 - POWER never really used the newGuestfish function in mantle/platform/qemu.go, with the new tests now using setupPreboot function that was recently added, the issue happened because there is no call for the POWER wrapper in the newGuestfish. Nonetheless, it is not a libguestfish issue since POWER8 (cluster) needs the vsmt set to 8. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

- It is a follow up for the issue openshift/os#720 and for coreos#2714 - POWER never really used the newGuestfish function in mantle/platform/qemu.go, with the new tests now using setupPreboot function that was recently added, the issue happened because there is no call for the POWER wrapper in the newGuestfish. Nonetheless, it is not a libguestfish issue since POWER8 (cluster) needs the vsmt set to 8. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

- It is a follow up for the issue openshift/os#720 and for #2714 - POWER never really used the newGuestfish function in mantle/platform/qemu.go, with the new tests now using setupPreboot function that was recently added, the issue happened because there is no call for the POWER wrapper in the newGuestfish. Nonetheless, it is not a libguestfish issue since POWER8 (cluster) needs the vsmt set to 8. Signed-off-by: Renata Ravanelli <rravanel@redhat.com>

cgwalters reviewed Feb 17, 2022

View reviewed changes

ravanelli force-pushed the power_libguest branch 2 times, most recently from 21cd487 to 69d952d Compare February 17, 2022 22:29

cgwalters reviewed Feb 17, 2022

View reviewed changes

cgwalters previously approved these changes Feb 17, 2022

View reviewed changes

jlebon reviewed Feb 17, 2022

View reviewed changes

ravanelli dismissed cgwalters’s stale review via 23bd2fe February 17, 2022 23:21

ravanelli force-pushed the power_libguest branch from 69d952d to 23bd2fe Compare February 17, 2022 23:21

jlebon approved these changes Feb 17, 2022

View reviewed changes

jlebon enabled auto-merge (rebase) February 17, 2022 23:50

miabbott approved these changes Feb 21, 2022

View reviewed changes

ravanelli disabled auto-merge February 21, 2022 19:58

ravanelli merged commit a6d7bbb into coreos:main Feb 21, 2022

ravanelli mentioned this pull request Feb 22, 2022

RHCOS 4.11 ppc64le test failures due to libguestfs errors? openshift/os#720

Closed

ravanelli mentioned this pull request Feb 22, 2022

Clean up POWER wrapper in libguestfish #2718

Merged

ravanelli deleted the power_libguest branch December 5, 2022 13:43

Conversation

ravanelli commented Feb 17, 2022

Uh oh!

cgwalters Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

cgwalters Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

jlebon Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

cgwalters Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

ravanelli Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

cgwalters Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

jlebon commented Feb 17, 2022

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

ravanelli commented Feb 17, 2022

Uh oh!

ravanelli commented Feb 17, 2022

Uh oh!

jlebon Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

jlebon commented Feb 17, 2022

Uh oh!

ravanelli commented Feb 21, 2022

Uh oh!

ravanelli commented Feb 21, 2022

Uh oh!

miabbott left a comment

Choose a reason for hiding this comment

Uh oh!

miabbott commented Feb 21, 2022

Uh oh!

openshift-ci Bot commented Feb 21, 2022

Uh oh!

ravanelli commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants