Replace check_var ARCH ppc64le by get_var OFW #4477

michelmno · 2018-02-22T13:50:16Z

problem initially detected on ppc64 (BE) tests failures
with openSUSE snapshot 2018021 as reported by
https://progress.opensuse.org/issues/32155

Assumption: OFW key defined in used templates for PowerPC arches
(ppc64, ppc64le) which is the case for default opensuse templates
(bot not for sle one <= potential TODO)

$find . -name templates -exec grep -Hn backend -A8 {}
\; |grep -E 'backend|ppc64|OFW' |grep ppc64 -A100
products/opensuse/templates-2691- name => "ppc64le",
products/opensuse/templates-2694- { key => "OFW", value => 1 },
products/opensuse/templates-2695- { key => "QEMU", value => "ppc64" },
products/opensuse/templates:2704: backend => "qemu",
products/opensuse/templates-2705- name => "ppc64",
products/opensuse/templates-2708- { key => "OFW", value => 1 },
products/opensuse/templates-2709- { key => "QEMU", value => "ppc64" },
products/opensuse/templates:2718: backend => "qemu",
products/opensuse/templates-2719- name => "ppc64le-multipath",
products/opensuse/templates-2723- { key => "OFW", value => 1 },
products/opensuse/templates-2724- { key => "QEMU", value => "ppc64" },
products/opensuse/templates:2733: backend => "qemu",
products/opensuse/templates-2734- name => "ppc64-multipath",
products/opensuse/templates-2738- { key => "OFW", value => 1 },
products/opensuse/templates-2739- { key => "QEMU", value => "ppc64" },
products/opensuse/templates:2748: backend => "qemu",

Related ticket: https://progress.opensuse.org/issues/32155
Verification run: not yet

michelmno · 2018-02-22T13:50:45Z

WARNING: I did not tried to test it yet !

problem initially detected on ppc64 (BE) tests failures with openSUSE snapshot 2018021 as reported by https://progress.opensuse.org/issues/32155 Assumption: OFW key defined in used templates for PowerPC arches (ppc64, ppc64le) which is the case for default opensuse templates (bot not for sle one <= potential TODO) === $find . -name templates -exec grep -Hn backend -A8 {} \; |grep -E 'backend|ppc64|OFW' |grep ppc64 -A100 products/opensuse/templates-2691- name => "ppc64le", products/opensuse/templates-2694- { key => "OFW", value => 1 }, products/opensuse/templates-2695- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2704: backend => "qemu", products/opensuse/templates-2705- name => "ppc64", products/opensuse/templates-2708- { key => "OFW", value => 1 }, products/opensuse/templates-2709- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2718: backend => "qemu", products/opensuse/templates-2719- name => "ppc64le-multipath", products/opensuse/templates-2723- { key => "OFW", value => 1 }, products/opensuse/templates-2724- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2733: backend => "qemu", products/opensuse/templates-2734- name => "ppc64-multipath", products/opensuse/templates-2738- { key => "OFW", value => 1 }, products/opensuse/templates-2739- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2748: backend => "qemu", === Signed-off-by: Michel Normand <normand@linux.vnet.ibm.com>

okurz · 2018-02-22T15:59:16Z

looks like a good idea. Let's see if someone can help with testing.

@SergioAtSUSE @Zaoliang ?

michelmno · 2018-02-22T16:26:23Z

I am on the way to test it locally with same snapshot 20180221 ppc64 iso on my local openQA instance, at least first tests seems to work correctly.

SergioAtSUSE

Good idea. ;)
We have to look at this in more detail to detect where it is PPC related and OFW related.

SergioAtSUSE · 2018-02-22T16:29:30Z

lib/utils.pm

@@ -333,7 +333,7 @@ sub workaround_type_encrypted_passphrase {
    return if get_var('UNENCRYPTED_BOOT');
    return if !get_var('ENCRYPT') && !get_var('FULL_LVM_ENCRYPT');
    # ppc64le on pre-storage-ng boot was part of encrypted LVM
-    return if !get_var('FULL_LVM_ENCRYPT') && !is_storage_ng && !check_var('ARCH', 'ppc64le');
+    return if !get_var('FULL_LVM_ENCRYPT') && !is_storage_ng && !get_var('OFW');


Here the check is about PPC having a proposed separated /boot partition.
AFAIK OFW and PPC are not always the same. So here would make more sense to have

return if !get_var('FULL_LVM_ENCRYPT') && !is_storage_ng && get_var('ARCH) !~ /^ppc/;

As per products/opensuse/templates OFW is defined for ppc64 and ppc64le ARCH, so I consider the same for all the tests of this commit.
Why do you want to keep a difference between the two ?

It is a coincidence that only our ppc machines have OFW. We could have another machines non-ppc which use OFW.

SergioAtSUSE · 2018-02-22T16:30:13Z

tests/console/kdump_and_crash.pm

@@ -44,7 +44,7 @@ sub run {
    select_console 'root-console';

    # all but PPC64LE arch's vmlinux images are gzipped
-    my $suffix = check_var('ARCH', 'ppc64le') ? '' : '.gz';
+    my $suffix = get_var('OFW') ? '' : '.gz';


Here also seems to be unrelated to OFW, but about PPC specific

SergioAtSUSE · 2018-02-22T16:30:54Z

tests/installation/bootloader.pm

@@ -39,7 +39,7 @@ sub run {
    }
    # on ppc64le boot have to be confirmed with ctrl-x or F10
    # and it doesn't have nice graphical menu with video and language options
-    if (!check_var('ARCH', 'ppc64le')) {
+    if (!get_var('OFW')) {


Please adapt also the comment above to clarify that the "textmode" comes from OFW.

SergioAtSUSE · 2018-02-22T16:31:12Z

tests/installation/partitioning_full_lvm.pm

@@ -22,10 +22,10 @@ use version_utils 'is_storage_ng';

 sub run {
    create_new_partition_table;
-    if (check_var('ARCH', 'ppc64le')) {    # ppc64le always needs PReP boot
+    if (get_var('OFW')) {    # ppc64le always needs PReP boot


That's also PPC related, not OFW

SergioAtSUSE · 2018-02-22T16:31:39Z

tests/installation/partitioning_warnings.pm

@@ -24,7 +24,7 @@ sub run {
    if (check_var('ARCH', 's390x')) {    # s390x need /boot/zipl on ext partition
        addpart(role => 'OS', size => 500, format => 'ext2', mount => '/boot');
    }
-    elsif (check_var('ARCH', 'ppc64le')) {    # ppc64le need PReP /boot
+    elsif (get_var('OFW')) {             # ppc64le need PReP /boot


PPC related, not OFW

SergioAtSUSE · 2018-02-22T16:36:01Z

tests/installation/welcome.pm

@@ -101,7 +101,7 @@ sub run {
                sles4sap => 'i',
                hpc      => 'x'
            );
-            $hotkey{sles4sap} = 'u' if check_var('ARCH', 'ppc64le');
+            $hotkey{sles4sap} = 'u' if get_var('OFW');


PPC, not OFW

Because a different list of products are offered for PPC

coolo · 2018-02-22T16:43:46Z

well, the whole idea of this patch is that OFW is always there when ARCH starts with ppc - so your reviews are bit strange

SergioAtSUSE · 2018-02-22T16:53:02Z

@coolo, for semantic. It would be like changing check_var('ARCH', 'aarch64') to check_var('UEFI', 1)

coolo · 2018-02-22T19:25:27Z

Yeah, once we test sparc servers we might be in trouble - until then I prefer check_var('OFW' ) over regexp checks

michelmno · 2018-02-23T08:38:22Z

@coolo, @SergioAtSUSE so what should I do with this PR ?

keep the PR with use of get_var('OFW') and dismiss the review to ignore the change request. (my preference of course :)
add a new "POWERPC" key to be set in template for ppc64 and ppc64le, and use it in all places identified in the review.
something else ?

Dismissing review. People is already aware that in the future we would need to review this checks, but accepting argument against regexp.

SergioAtSUSE · 2018-02-23T12:07:09Z

@michelmno, from my part I only need some verification runs, like RAID0 test.

michelmno · 2018-02-23T12:57:34Z

my local tests with this patch allow all Raid* tests to pass for ppc64 on snapshot 20180221.

SergioAtSUSE · 2018-02-23T16:54:26Z

Waiting for verification run: http://copland.arch.suse.de/tests/911

coolo · 2018-02-24T08:30:33Z

worked

problem initially detected on ppc64 (BE) tests failures with openSUSE snapshot 2018021 as reported by https://progress.opensuse.org/issues/32155 Assumption: OFW key defined in used templates for PowerPC arches (ppc64, ppc64le) which is the case for default opensuse templates (bot not for sle one <= potential TODO) === $find . -name templates -exec grep -Hn backend -A8 {} \; |grep -E 'backend|ppc64|OFW' |grep ppc64 -A100 products/opensuse/templates-2691- name => "ppc64le", products/opensuse/templates-2694- { key => "OFW", value => 1 }, products/opensuse/templates-2695- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2704: backend => "qemu", products/opensuse/templates-2705- name => "ppc64", products/opensuse/templates-2708- { key => "OFW", value => 1 }, products/opensuse/templates-2709- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2718: backend => "qemu", products/opensuse/templates-2719- name => "ppc64le-multipath", products/opensuse/templates-2723- { key => "OFW", value => 1 }, products/opensuse/templates-2724- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2733: backend => "qemu", products/opensuse/templates-2734- name => "ppc64-multipath", products/opensuse/templates-2738- { key => "OFW", value => 1 }, products/opensuse/templates-2739- { key => "QEMU", value => "ppc64" }, products/opensuse/templates:2748: backend => "qemu", === Signed-off-by: Michel Normand <normand@linux.vnet.ibm.com>

pevik · 2018-12-05T13:21:23Z

IMHO this was a wrong direction: detecting ppc* with variable for OFW is really cryptic. And one day we might want to use QEMU Open Firmware on other platforms.

It'd be great, if there was another variable for ARCH (not sure for name), which would have more general name than MACHINE or ARCH has. I.e. having content which QEMU variable has (i.e. ppc64 for both ppc64 and ppc64le). Problem with QEMU is, that is not defined on x86_64 ARCH (this variable would have to be mandatory for all tests).

okurz · 2019-01-25T12:16:48Z

@pevik I agree with you that we should not "detect ppc* with OFW" but I do not think that we do. I think we are doing a good job to really handle OFW-specific behaviour based on that variable and we still look at ARCH (or maybe MACHINE) in other places. We can always use get_var('ARCH', '') =~ /ppc/ or check_var('ARCH', 'ppc') || check_var('ARCH', 'ppc64le')

SergioAtSUSE previously requested changes Feb 22, 2018

View reviewed changes

coolo merged commit 8e30d7c into os-autoinst:master Feb 24, 2018

michelmno deleted the ofw_not_ppc64le branch February 26, 2018 07:35

alvarocarvajald mentioned this pull request Mar 6, 2018

Add developer supplied tests to tests/sles4sap/sapconf #4564

Merged

alvarocarvajald mentioned this pull request Oct 9, 2018

HA: fix upgrade tests on ppc64le #5905

Merged

ldevulder mentioned this pull request Dec 5, 2018

Fix virtio console on ppc64le #6352

Merged

czerw mentioned this pull request Dec 11, 2019

Clarify OFW variable purpose os-autoinst/os-autoinst#1321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace check_var ARCH ppc64le by get_var OFW #4477

Replace check_var ARCH ppc64le by get_var OFW #4477

michelmno commented Feb 22, 2018

michelmno commented Feb 22, 2018

okurz commented Feb 22, 2018

michelmno commented Feb 22, 2018

SergioAtSUSE left a comment

SergioAtSUSE Feb 22, 2018

michelmno Feb 22, 2018

SergioAtSUSE Feb 22, 2018

SergioAtSUSE Feb 22, 2018

SergioAtSUSE Feb 22, 2018

SergioAtSUSE Feb 22, 2018

SergioAtSUSE Feb 22, 2018

SergioAtSUSE Feb 22, 2018

coolo commented Feb 22, 2018

SergioAtSUSE commented Feb 22, 2018

coolo commented Feb 22, 2018

michelmno commented Feb 23, 2018

SergioAtSUSE commented Feb 23, 2018

michelmno commented Feb 23, 2018

SergioAtSUSE commented Feb 23, 2018 •

edited

Loading

coolo commented Feb 24, 2018

pevik commented Dec 5, 2018

okurz commented Jan 25, 2019

Replace check_var ARCH ppc64le by get_var OFW #4477

Replace check_var ARCH ppc64le by get_var OFW #4477

Conversation

michelmno commented Feb 22, 2018

michelmno commented Feb 22, 2018

okurz commented Feb 22, 2018

michelmno commented Feb 22, 2018

SergioAtSUSE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolo commented Feb 22, 2018

SergioAtSUSE commented Feb 22, 2018

coolo commented Feb 22, 2018

michelmno commented Feb 23, 2018

SergioAtSUSE commented Feb 23, 2018

michelmno commented Feb 23, 2018

SergioAtSUSE commented Feb 23, 2018 • edited Loading

coolo commented Feb 24, 2018

pevik commented Dec 5, 2018

okurz commented Jan 25, 2019

SergioAtSUSE commented Feb 23, 2018 •

edited

Loading