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

Fix docker test on SLE12 validation tests #4805

Merged
merged 3 commits into from
Apr 11, 2018
Merged

Fix docker test on SLE12 validation tests #4805

merged 3 commits into from
Apr 11, 2018

Conversation

okurz
Copy link
Member

@okurz okurz commented Apr 9, 2018

docker is part of the containers-module. Our SLE12 test scenario
extra_tests_in_textmode runs based on an unregistered SLES installation so the
module is not immediately available. Adding the update repo which includes the
module solves the problem same as is already done in other places. As the
validation tests focus on the base system and not the modules I guess we can
go with this approach.

Alternatives would have been:

  • O1: Just register the whole system and add the module for just one test
    module -> deregister again after the test module
  • O2: Publish the hdd image from a test suite that registers the system and
    add a new test scenario like "extra_tests_registered" where we would
    schedule the docker module -> Con: Quite different test flow for SLE12 vs.
    SLE15 as well as openSUSE

Verification run: http://lord.arch/tests/755

Related progress issue: https://progress.opensuse.org/issues/33184

asmorodskyi
asmorodskyi reviewed Apr 10, 2018
View reviewed changes
lib/main_common.pm Outdated
@@ -1402,7 +1402,7 @@ sub load_extra_tests_textmode {
# Currently for our SLE12 validation tests we are not using a
# registered SLE installation so we should not schedule the test
# modules.
load_docker_tests if (check_var('ARCH', 'x86_64') && ((is_sle('12-SP2+') && (is_sle('<12-SP4') || is_sle('15+')) || !is_sle)));
load_docker_tests if (check_var('ARCH', 'x86_64') && (is_sle('12-SP2+') || is_sle('15+') || !is_sle));
Copy link
Member

Choose a reason for hiding this comment

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

'12-SP+' should include '15+' ,right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks we were working on similar thing #4810 my goal is to disable docker tests on SLE12-SP2.

Copy link
Member

Choose a reason for hiding this comment

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

Then why not simply ?

load_docker_tests if (check_var('ARCH', 'x86_64') && (is_sle('12-SP3+') || !is_sle));

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Should i cancel my PR and wait for this one?

Copy link
Member

Choose a reason for hiding this comment

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

@czerw, if you fix your PR quickest than @okurz, I will merge yours first, so you don't have to wait for #4805.

Copy link
Member

Choose a reason for hiding this comment

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

@okurz, please be aware that I merged @czerw's PR #4810

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. I assume this broke SLE12SP4 tests because without this PR the docker test module will fail. So I would like to rebase and then merge. And we can discuss which repo to take or to register later.

asmorodskyi
asmorodskyi reviewed Apr 10, 2018
View reviewed changes
lib/registration.pm
@@ -624,7 +624,12 @@ sub install_docker_when_needed {
die 'Docker is not pre-installed.' if zypper_call('se -x --provides -i docker | grep docker', allow_exit_codes => [0, 1]);
}
else {
add_suseconnect_product('sle-module-containers') if is_sle('15+');
if (is_sle('<15')) {
assert_script_run('zypper se docker || zypper -n ar -f http://download.suse.de/ibs/SUSE:/SLE-12:/Update/standard/SUSE:SLE-12:Update.repo');
Copy link
Member

Choose a reason for hiding this comment

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

zypper ref , also don't you need to accept key from new repo ?

Copy link
Member Author

Choose a reason for hiding this comment

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

apparently not -> http://lord.arch/tests/755#step/docker/10

Copy link
Contributor

@czerw czerw Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't do it in this way. QAM tests have already container module added during installation.
See https://openqa.suse.de/tests/1604714 MRU_ADDONS=sdk,tcm,wsm,we,contm
So basically there is no need to add special repo for SLE12-SP3(not sure about SP4).

Copy link
Member

Choose a reason for hiding this comment

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

@czerw, please be aware that the repo will only be added if package docker is not found. When container module is already added, the repo will not be added. So it may work as expected.

But, I am still skeptical of this change. Now the reason why we need to add that repo is shadowed (to trick that we are using an image with unregistered SUT. An scenario where the customer also could not install docker). This tricky added repo may affect the following test modules on this test suite and people may forget about this and will probably understand/change this wrong. I would still prefer to use an image with registered SUT, so we avoid false negatives and confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@czerw @SergioAtSUSE I share your concerns but after your PR was already enabling docker on SLE12SP4 I will merge my PR in the hope to fix the test for now for SLE12SP4 as well … or actually to prevent failure. But we can keep https://progress.opensuse.org/issues/33184 open for now and discuss there.

@SergioAtSUSE
Copy link
Member

Related: #4810

@okurz
Exclude console/sle2docker for SLE12SP4 where non-existant anymore
5018c0f 
See http://lord.arch/tests/755#step/sle2docker/6
@okurz
Fix docker test on SLE12 validation tests
64d88b5 
docker is part of the containers-module. Our SLE12 test scenario
extra_tests_in_textmode runs based on an unregistered SLES installation so the
module is not immediately available. Adding the update repo which includes the
module solves the problem same as is already done in other places. As the
validation tests focus on the base system and not the modules I guess we can
go with this approach.

Alternatives would have been:
* O1: Just register the whole system and add the module for just one test
  module -> deregister again after the test module
* O2: Publish the hdd image from a test suite that registers the system and
  add a new test scenario like "extra_tests_registered" where we would
  schedule the docker module -> Con: Quite different test flow for SLE12 vs.
  SLE15 as well as openSUSE

Verification run: http://lord.arch/tests/755

Related progress issue: https://progress.opensuse.org/issues/33184
@okurz
Delete obsolete 'validate_repos' export from lib/utils.pm
bea11e7 
Fixes a perl warning about duplicate definition.
@okurz okurz merged commit 323a463 into os-autoinst:master Apr 11, 2018
@okurz okurz deleted the fix/docker2 branch April 11, 2018 17:54
@okurz okurz mentioned this pull request Apr 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorodskyi asmorodskyi asmorodskyi left review comments

@SergioAtSUSE SergioAtSUSE SergioAtSUSE left review comments

@czerw czerw czerw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@okurz @SergioAtSUSE @asmorodskyi @czerw