Remove condition which create wrong scheduling on JeOS #13668

b10n1k · 2021-11-11T18:38:45Z

The main issue was the condition is products/opensuse/main.pm in L261
which was evaluated to false and then later another condition linked to load_container_tests.

The solution was to remove this condition, so letting load_jeos_tests run first and then
find load_container_tests which in turn will load_jeos_containers as it was before.

Signed-off-by: Ioannis Bonatakis ybonatakis@suse.com

Related ticket: https://progress.opensuse.org/issues/102272
Verification run:

https://openqa.opensuse.org/tests/2029918
https://openqa.opensuse.org/tests/2029917
https://openqa.opensuse.org/tests/2030191

https://openqa.suse.de/tests/7650121
https://openqa.suse.de/tests/7650120

VR for 366eb59:
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2031116
Github oauth token provided, performing authenticated requests
Created job #2036430: opensuse-15.3-JeOS-for-kvm-and-xen-ppc64le-Build9.279-containers_docker_on_15.2_gm@ppc64le -> https://openqa.opensuse.org/t2036430
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2031115
Github oauth token provided, performing authenticated requests
Created job #2036431: opensuse-15.3-JeOS-for-kvm-and-xen-ppc64le-Build9.279-containers_podman_on_15.2_gm@ppc64le -> https://openqa.opensuse.org/t2036431
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2034865
Github oauth token provided, performing authenticated requests
Created job #2036432: opensuse-Tumbleweed-JeOS-for-kvm-and-xen-x86_64-Build20211113-jeos-containers@64bit_virtio -> https://openqa.opensuse.org/t2036432
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2034867
Github oauth token provided, performing authenticated requests
Created job #2036433: opensuse-Tumbleweed-JeOS-for-kvm-and-xen-x86_64-Build20211113-jeos@64bit_virtio -> https://openqa.opensuse.org/t2036433
❯
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2031132
Github oauth token provided, performing authenticated requests
Created job #2036434: opensuse-15.3-JeOS-for-AArch64-aarch64-Build9.279-jeos-containers@USBboot_aarch64 -> https://openqa.opensuse.org/t2036434
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2031116
Github oauth token provided, performing authenticated requests
Created job #2036436: opensuse-15.3-JeOS-for-kvm-and-xen-ppc64le-Build9.279-containers_docker_on_15.2_gm@ppc64le -> https://openqa.opensuse.org/t2036436
❯ openqa-clone-custom-git-refspec #13668 https://openqa.opensuse.org/tests/2031118
Github oauth token provided, performing authenticated requests
Created job #2036437: opensuse-15.3-JeOS-for-kvm-and-xen-x86_64-Build9.279-containers_on_centos_host_x86_64@64bit-2G -> https://openqa.opensuse.org/t2036437
❯
❯ openqa-clone-custom-git-refspec #13668 https://openqa.suse.de/tests/7675887
Github oauth token provided, performing authenticated requests
Created job #7678825: sle-15-SP3-JeOS-for-kvm-and-xen-Updates-x86_64-Build20211115-1-jeos-filesystem@uefi-virtio-vga -> https://openqa.suse.de/t7678825
❯ openqa-clone-custom-git-refspec #13668 https://openqa.suse.de/tests/7677181
Github oauth token provided, performing authenticated requests
Created job #7678827: sle-15-SP3-Server-DVD-Updates-x86_64-Build20211115-1-docker_tests@64bit -> https://openqa.suse.de/t7678827

jlausuch · 2021-11-11T18:51:57Z

lib/main_containers.pm

@@ -84,23 +88,44 @@ sub load_host_tests_docker {
    loadtest "containers/container_diff" if (is_opensuse());
 }

+sub load_jeos_containers {


This is wrong. We already have a function loader for podman and docker tests. You are duplicating a lot of code here which is unnecessary.

As an urgent task i tried to keep the scheduling clean and isolated. in other case i would have many moving parts. Like many "if else". ofc i would like to avoid and improve this part, which appears twice

if (is_opensuse_jeos()) { load_jeos_containers(); } else

but it was the faster approach i came up initially. but it could happen later.
I also think that the load_jeos_containers is a good idea for now to not interfere, as jeos is kinda special case, and doing this so, we decouple the scheduling from sle.

mloviska · 2021-11-12T08:03:23Z

products/opensuse/main.pm

@@ -258,7 +258,7 @@ sub load_default_tests {
 }

 # load the tests in the right order
-if (is_jeos && !is_container_test) {
+if (is_jeos) {


This would break the containers_* tests that are actually not JeOS but have flavor JeOS-for-kvm-and-xen

my proposal is actually only having this change :)

@mloviska if this is one of the test cases that you had in mind, I run a VR. i cant see how the implementation affects it.

Please compare https://openqa.opensuse.org/tests/2031118# vs https://openqa.opensuse.org/tests/2031324#

i did an effort to fix this with a dirty approach. I dont know. maybe @jlausuch suggestion can clean this mess.

this change is ready for review Still i can remove the duplication of the new block but feel free to go on(because the urgency) if you want and we will fix in another PR if it is needed.

jlausuch · 2021-11-12T09:03:09Z

This is my proposal I had in mind before you came up with this PR. Sorry if I didn't present it before, I hope it's fine.
#13672
If you agree, feel free to update this PR and I will close mine.

jlausuch · 2021-11-15T12:57:05Z

I would really like to fix the issue from the root, which is the openqa job setup, not having a hacky code to handle any weird setups (e.g. booting centos with JeOS flavor).
So, even though it's urgent, let's fix this in the right way, please. I proposed to have a new flavor for these cases where we boot "other" host images.

b10n1k · 2021-11-15T13:03:26Z

I would really like to fix the issue from the root, which is the openqa job setup, not having a hacky code to handle any weird setups (e.g. booting centos with JeOS flavor). So, even though it's urgent, let's fix this in the right way, please. I proposed to have a new flavor for these cases where we boot "other" host images.

i will unassign myself from this ticket then. i have no idea what is the right way. But i know that this would be a much bigger change, so it is up to you.

grisu48

For a fix this is too complex and we are applying a fix plus some additional cleanup at the same time. If it is an urgent fix, then please only fix the issue and do the cleanup afterwards.

In particular: Why don't we apply Joses fix #13672 so that the issue is resolved and then continue with this PR to make it nice without time pressure?

grisu48 · 2021-11-15T12:57:51Z

lib/main_common.pm

@@ -554,6 +554,7 @@ sub load_system_role_tests {
    }
 }
 sub load_jeos_tests {
+    return if (get_var('CONTAINER_RUNTIME', 0) && (is_leap && is_ppc64le) || check_var('CONTAINERS_NO_SUSE_OS', 1));


Please add a comment, why this is necessary

#13668 (comment)

Yianni, please add # a comment 😄

grisu48 · 2021-11-15T12:58:27Z

lib/main_containers.pm

+}
+
+sub is_container_jeos {
+    return (get_var('CONTAINER_RUNTIME', 0) && ((is_leap && is_ppc64le) || check_var('CONTAINERS_NO_SUSE_OS', 1)));


I don't understand the purpose of this check. Please add a comment explaining what we get in doing those checks.

grisu48 · 2021-11-15T13:02:57Z

lib/main_containers.pm


 sub load_container_tests {
    my $runtime = get_required_var('CONTAINER_RUNTIME');
    if (get_var('BOOT_HDD_IMAGE')) {
        loadtest 'installation/bootloader_zkvm' if is_s390x;
-        loadtest 'boot/boot_to_desktop';
+        loadtest 'boot/boot_to_desktop' if (is_container_jeos || !is_jeos);


By default we always run boot/boot_to_desktop, so this logic is flawed to fail in the future.
The default should always be performed unless the exception and not the other way around. So instead of asking "right now, boot_to_desktop needs to be executed if ...." one should approach it as "boot_to_desktop should always be executed, except for the special case, that ....

TL'DR: Please change it to something like

Suggested change

loadtest 'boot/boot_to_desktop' if (is_container_jeos || !is_jeos);

loadtest 'boot/boot_to_desktop' unless ... ;

but remains the default. i tried with unless but there were tests failing. i would like to leave it as it is, because this is getting complex enough and i think it is going to be a better approach soon after.

It does not remain the default, because it is only executed if a certain condition is fulfilled.
The opposite is that it is executed by default, unless a condition is fulfilled.

The difference is opt-in by default or opt-out by default.

Why is this important? Because if we add another test e.g. for SLE Micro, that one would not execute boot/boot_to_desktop unless we add that flavor to the if condition.
If we have a exclusion logic (unless) then every new test runs, however weird they might be, will schedule boot_to_desktop by default.

The main issue was the condition is `products/opensuse/main.pm` in L261 which was evaluated to *false* and then later another condition linked to *load_container_tests*. The solution was to remove this condition, so letting *load_jeos_tests* run first and then find *load_container_tests* which in turn will *load_jeos_containers* as it was before. Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com> test containers_on Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com>

b10n1k · 2021-12-15T08:42:53Z

deprecated from #13746

jlausuch reviewed Nov 11, 2021

View reviewed changes

mloviska reviewed Nov 12, 2021

View reviewed changes

jlausuch mentioned this pull request Nov 12, 2021

Containers: fix schedule for JeOS #13672

Merged

b10n1k force-pushed the jeos_102272 branch 3 times, most recently from 91634b0 to 0f55413 Compare November 15, 2021 12:48

grisu48 reviewed Nov 15, 2021

View reviewed changes

b10n1k force-pushed the jeos_102272 branch from 0f55413 to bde92a0 Compare November 16, 2021 10:18

b10n1k closed this Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove condition which create wrong scheduling on JeOS #13668

Remove condition which create wrong scheduling on JeOS #13668

b10n1k commented Nov 11, 2021 •

edited

Loading

jlausuch Nov 11, 2021

b10n1k Nov 11, 2021

mloviska Nov 12, 2021

jlausuch Nov 12, 2021

b10n1k Nov 12, 2021

mloviska Nov 12, 2021

b10n1k Nov 15, 2021

b10n1k Nov 15, 2021

jlausuch commented Nov 12, 2021 •

edited

Loading

jlausuch commented Nov 15, 2021

b10n1k commented Nov 15, 2021

grisu48 left a comment •

edited

Loading

grisu48 Nov 15, 2021

b10n1k Nov 15, 2021

pdostal Nov 16, 2021

b10n1k Nov 16, 2021

grisu48 Nov 15, 2021

grisu48 Nov 15, 2021

b10n1k Nov 15, 2021

grisu48 Nov 16, 2021 •

edited

Loading

b10n1k commented Dec 15, 2021

	loadtest 'boot/boot_to_desktop' if (is_container_jeos \|\| !is_jeos);
	loadtest 'boot/boot_to_desktop' unless ... ;

Remove condition which create wrong scheduling on JeOS #13668

Remove condition which create wrong scheduling on JeOS #13668

Conversation

b10n1k commented Nov 11, 2021 • edited Loading

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

jlausuch commented Nov 12, 2021 • edited Loading

jlausuch commented Nov 15, 2021

b10n1k commented Nov 15, 2021

grisu48 left a comment • edited Loading

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

grisu48 Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

b10n1k commented Dec 15, 2021

b10n1k commented Nov 11, 2021 •

edited

Loading

jlausuch commented Nov 12, 2021 •

edited

Loading

grisu48 left a comment •

edited

Loading

grisu48 Nov 16, 2021 •

edited

Loading