action #33388

openQA Project - action #14626: [epic] backend and console capabilities interface to increase extensibility and code reuse

action #38819: [tools][functional][u][epic] Refactor use of backends

[functional][u][easy][pvm] Implement proper split from other backends

Added by nicksinger about 2 years ago. Updated 9 months ago.

Status:ResolvedStart date:17/02/2019
Priority:NormalDue date:
Assignee:mgriessmeier% Done:

0%

Category:Enhancement to existing tests
Target version:SUSE QA tests - Milestone 25
Difficulty:easy
Duration:

Description

Motivation

The current implementation of the spvm backend makes use of functions ( e.g. use_ssh_serial_console located in lib/ipmi_backend_utils.pm) from other "backends".
This is good enough for a first proof of concept but for the sake of clarity and further maintainability this should reside in a separate file which is shared/included from the backends using it.

Acceptance criteria

  • AC1: Have clearly seperated backends with shared common functions
  • AC2: Do not include any functions from other backends (e.g. from ipmi into spvm)

Suggestions

  • Extract method "use_ssh_serial_console" into a common lib, e.g. "ssh_remote_console_backend_something_foobar"
  • Call git grep 'BACKEND.*pvm' and only work on these occurences, that excludes e.g. boot_from_pxe.pm
  • Find occurences like in https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/susedistribution.pm#L554 where we look for "s390x || ipmi || spvm" and replace this with an explicit function returning a boolean value for what it really is, e.g. "no_local_tty" or "remote_backend" or "relies_on_ssh_terminal"
  • Create followup sibling ticket for all necessary work for other backends

Further details

Focus on spvm, leave the rest for the parent epic. "Having a single verification run is enough" says okurz

Example for a change:

- if (check_var('BACKEND', 'foo') || check_var('BACKEND', 'bar')) {
+ sub backend_is_remote { return check_var('BACKEND', 'foo') || check_var('BACKEND', 'bar') }
+ if (backend_is_remote) {

Related issues

Related to openQA Project - action #38486: [functional][u] add capability flags to os-autoinst backe... Rejected 17/07/2018
Related to openQA Tests - action #38423: [sle][functional][u][hard] Refactor first_boot to unify d... Rejected 16/07/2018
Blocked by openQA Project - action #21838: [functional][u][saga] PowerVM backend Resolved 08/08/2017 31/07/2018
Blocks openQA Tests - action #41546: [functional][y] Change architecture checks with functiona... Workable 25/09/2018
Precedes openQA Tests - action #45008: [functional][u] Further improvements on splitting backend... Resolved 16/04/2019

History

#1 Updated by nicksinger about 2 years ago

  • Copied from action #33340: [tools][functional][u][medium][pvm] Enable graphical installation for the powerVM backend added

#2 Updated by nicksinger about 2 years ago

  • Blocked by action #21838: [functional][u][saga] PowerVM backend added

#3 Updated by nicksinger about 2 years ago

  • Copied from deleted (action #33340: [tools][functional][u][medium][pvm] Enable graphical installation for the powerVM backend)

#4 Updated by nicksinger about 2 years ago

  • Copied to action #33697: [functional][u][hard][pvm] Enable the powerVM backend to conduct multimachine tests added

#5 Updated by nicksinger about 2 years ago

  • Copied to deleted (action #33697: [functional][u][hard][pvm] Enable the powerVM backend to conduct multimachine tests)

#6 Updated by okurz almost 2 years ago

  • Subject changed from [tools][functional][easy][pvm] Implement proper split from other backends to [tools][functional][u][easy][pvm] Implement proper split from other backends
  • Target version set to Milestone 17

#7 Updated by okurz almost 2 years ago

  • Due date set to 14/08/2018
  • Target version changed from Milestone 17 to Milestone 18

#8 Updated by okurz almost 2 years ago

  • Target version changed from Milestone 18 to Milestone 18

#9 Updated by okurz over 1 year ago

  • Related to action #38486: [functional][u] add capability flags to os-autoinst backends (or tests) added

#10 Updated by okurz over 1 year ago

  • Related to action #38423: [sle][functional][u][hard] Refactor first_boot to unify duplicated behavior for remote backend added

#11 Updated by okurz over 1 year ago

  • Copied to action #38819: [tools][functional][u][epic] Refactor use of backends added

#12 Updated by okurz over 1 year ago

  • Copied to deleted (action #38819: [tools][functional][u][epic] Refactor use of backends)

#13 Updated by okurz over 1 year ago

  • Description updated (diff)
  • Status changed from New to Workable
  • Parent task set to #38819

#14 Updated by okurz over 1 year ago

  • Description updated (diff)
  • Estimated time set to 3.00
  • Difficulty set to easy

#15 Updated by SLindoMansilla over 1 year ago

  • Status changed from Workable to In Progress
  • Assignee set to SLindoMansilla

#16 Updated by SLindoMansilla over 1 year ago

  • Status changed from In Progress to Feedback

#17 Updated by okurz over 1 year ago

  • Due date changed from 14/08/2018 to 28/08/2018

bulk move to next sprint as could not be discussed in SR

#18 Updated by SLindoMansilla over 1 year ago

  • Status changed from Feedback to In Progress

PR is WIP until I clarify with Alice the details of the affected test suite: https://openqa.suse.de/tests/1495366

#19 Updated by mgriessmeier over 1 year ago

  • Due date changed from 28/08/2018 to 11/09/2018

PR is still WIP
move due to vacation

#21 Updated by SLindoMansilla over 1 year ago

  • Status changed from In Progress to Workable

I have problems executing IPMI jobs on my webui.
And general problems with my openQA instance.

#22 Updated by mgriessmeier over 1 year ago

  • Due date changed from 11/09/2018 to 25/09/2018

#23 Updated by SLindoMansilla over 1 year ago

  • Status changed from Workable to In Progress

#24 Updated by SLindoMansilla over 1 year ago

  • Due date changed from 25/09/2018 to 09/10/2018

Moving to sprint 27. Not able to finish on sprint 26.

#25 Updated by riafarov over 1 year ago

  • Start date set to 01/01/5000

due to changes in a related task

#26 Updated by riafarov over 1 year ago

  • Blocks action #41546: [functional][y] Change architecture checks with functionality of the #33388 to detect VNC installation added

#27 Updated by okurz over 1 year ago

Wow, not sure if we want to wait that long ;)

#28 Updated by okurz over 1 year ago

  • Start date set to 01/01/5000

due to changes in a related task

#29 Updated by okurz over 1 year ago

  • Start date set to 01/01/5000

due to changes in a related task

#30 Updated by okurz over 1 year ago

  • Due date set to 23/10/2018
  • Target version changed from Milestone 18 to Milestone 20
  • Start date deleted (01/01/5000)

#31 Updated by okurz over 1 year ago

  • Status changed from In Progress to Workable
  • Assignee deleted (SLindoMansilla)

as discussed in planning, not really working on that right now.

#32 Updated by szarate over 1 year ago

  • Status changed from Workable to In Progress
  • Assignee set to szarate

I'll take a look to it, with #38423 in mind

#33 Updated by okurz over 1 year ago

  • Target version changed from Milestone 20 to Milestone 21

#35 Updated by okurz over 1 year ago

@szarate, this ticket was estimated as "easy" but is above our mean cycle time. Is the status "In Progress" correct or would you consider this task "Blocked" waiting for another ticket first or "Feedback" waiting on feedback in the PR? What I saw in the PR makes sense to me but needs a bit more love to pull the changes through in the test modules I guess :)

#36 Updated by szarate over 1 year ago

@okurz, please take a look at the last changes and my two questions (I actually updated the PR yesterday). There are some verification runs that could be ran, can you help with that?

What I saw in the PR makes sense to me but needs a bit more love to pull the changes through in the test modules I guess :)

Can you elaborate? I don't understand too much the sentence.

#37 Updated by szarate over 1 year ago

  • Project changed from openQA Project to openQA Tests
  • Category changed from 132 to Enhancement to existing tests

Also, this belongs to test code, not backend.

#38 Updated by okurz over 1 year ago

szarate wrote:

@okurz, please take a look at the last changes and my two questions (I actually updated the PR yesterday). There are some verification runs that could be ran, can you help with that?

I don't yet see why it would be necessary to run verification runs as I do not see any bigger, risky changes yet.

What I saw in the PR makes sense to me but needs a bit more love to pull the changes through in the test modules I guess :)


Can you elaborate? I don't understand too much the sentence.

Easy, follow all suggestions and ensure the ACs are covered

szarate wrote:

Also, this belongs to test code, not backend.

Agreed – for now – as we still have #38486 for a more backend-centric proposal which we can base on refactoring the test code first.

#39 Updated by szarate over 1 year ago

I don't yet see why it would be necessary to run verification runs as I do not see any bigger, risky changes yet.

@okurz I planned initially on doing more changes, but I guess there's no reason to keep losig time on trying to get verification runs.

Easy, follow all suggestions and ensure the ACs are covered

I think they're covered by now. for suggestion #2, there's already is_remote_backend, which I think is sufficient as a first step (it was there already, I just moved it to a different place).

Agreed – for now – as we still have #38486 for a more backend-centric proposal which we can base on refactoring the test code first.

#40 Updated by szarate over 1 year ago

  • Status changed from In Progress to Feedback

Setting to feedback, waiting for either merge or more feedback on the PR.

#41 Updated by szarate over 1 year ago

  • Start date set to 01/01/5000

due to changes in a related task

#42 Updated by szarate over 1 year ago

  • Precedes action #45008: [functional][u] Further improvements on splitting backend code added

#43 Updated by okurz about 1 year ago

https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/6316 is merged, thanks for that. What I see as missing according to the suggestions in the description is the still many "if backend spvm":

$ git grep 'BACKEND.*pvm'
lib/Utils/Backends.pm:      check_var('BACKEND', 'spvm');
lib/bootloader_setup.pm:    type_string_very_slow "console=tty " unless (check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm'));
lib/bootloader_setup.pm:    if (check_var("BACKEND", "spvm")) {
lib/main_common.pm:    elsif (check_var('BACKEND', 'spvm')) {
lib/main_common.pm:        if (!check_var('BACKEND', 'ipmi') && !is_hyperv_in_gui && !get_var("LIVECD") && !check_var('BACKEND', 'spvm')) {
lib/main_common.pm:            and !check_var('BACKEND', 'spvm')
lib/power_action_utils.pm:    console('root-ssh')->kill_ssh if check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm');
lib/susedistribution.pm:    if (check_var('BACKEND', 'ikvm') || check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm')) {
lib/susedistribution.pm:    if (check_var('BACKEND', 'ipmi') || check_var('BACKEND', 's390x') || get_var('S390_ZKVM') || check_var('BACKEND', 'spvm')) {
lib/susedistribution.pm:        $hostname = get_required_var('SUT_IP') if check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm');
lib/susedistribution.pm:        elsif (check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm')) {
lib/susedistribution.pm:    elsif ($console =~ m/root-console$/ && check_var('BACKEND', 'spvm')) {
lib/susedistribution.pm:        if (check_var('BACKEND', 's390x') || get_var('S390_ZKVM') || check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm')) {
lib/utils.pm:    return if (check_var('BACKEND', 'spvm')) || (check_var('BACKEND', 'ipmi'));
tests/installation/installation_overview.pm:    $need_ssh = 1 if check_var('BACKEND', 'spvm');    # we better be able to login
tests/installation/logs_from_installation_system.pm:    if (check_var('BACKEND', 'ipmi') || check_var('BACKEND', 'spvm')) {
tests/installation/reboot_after_installation.pm:    if (check_var('BACKEND', 'spvm')) {

the first occurence within lib/Utils/Backends.pm is fine but we should try to use more is_remote_backend or similar in the test modules.

#44 Updated by okurz about 1 year ago

  • Priority changed from Normal to High
  • Target version changed from Milestone 21 to Milestone 22

#45 Updated by mgriessmeier about 1 year ago

how to proceed here to not let the ticket rot in Feedback?
If there is anything left, I suggest to either create a new ticket or put it back to workable with suggestions of "next steps"

#46 Updated by szarate about 1 year ago

  • Status changed from Feedback to Resolved

I call it resolved, as okurz addresed his own comments in the child ticket: https://progress.opensuse.org/issues/45008#note-5

#47 Updated by okurz about 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from szarate to okurz

Well, so far we have not fulfilled what was written in the suggestions or ACs but let's wait for the PR https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/6600 as referenced in #45008#note-5

#48 Updated by okurz about 1 year ago

  • Status changed from Feedback to New
  • Assignee deleted (okurz)
  • Priority changed from High to Normal

PR merged, no message received about broken tests. Next steps as mentioned in the suggestion can be followed on with.

#49 Updated by okurz about 1 year ago

  • Status changed from New to Workable

#50 Updated by szarate about 1 year ago

  • Subject changed from [tools][functional][u][easy][pvm] Implement proper split from other backends to [functional][u][easy][pvm] Implement proper split from other backends
  • Status changed from Workable to Blocked
  • Assignee set to okurz

@okurz, can you please set expectations for this ticket? also, there's the child ticket, can you also give it a look? so that we can decide to pick it up or not? I'd close it tbh.

Setting to blocked until PO clarifies.

#51 Updated by okurz about 1 year ago

  • Status changed from Blocked to Workable
  • Assignee deleted (okurz)
  • Target version changed from Milestone 22 to Milestone 23
  • Start date changed from 01/01/5000 to 17/02/2019

As stated in #33388#note-48 I expect to follow "next steps as mentioned in the suggestion". E.g. we can now use the method "Utils::Backends::has_ttys" in more places, e.g. when checking if we should execute the console font test.

#52 Updated by mgriessmeier 12 months ago

  • Target version changed from Milestone 23 to Milestone 24

moving to M24

#53 Updated by mgriessmeier 11 months ago

  • Target version changed from Milestone 24 to Milestone 25

#54 Updated by zluo 10 months ago

  • Status changed from Workable to In Progress
  • Assignee set to zluo

take over and check this. maybe directly working on ticket https://progress.opensuse.org/issues/51236 make sense now.

#55 Updated by zluo 10 months ago

Compilation looks good for Backends.pm, bootloader_setup.pm

http://f40.suse.de/tests/3746
http://f40.suse.de/tests/3751

add more changes...

#56 Updated by zluo 10 months ago

tests/installation/bootloader.pm
lib/main_common.pm

modified now.

#57 Updated by zluo 10 months ago

WIP PR:

https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/7503

Adding documentation into Utils::Backends is required.

#58 Updated by SLindoMansilla 10 months ago

  • Blocked by action #51725: [s390x][kvm] test fails in bootloader_zkvm added

#59 Updated by zluo 10 months ago

check:
https://openqa.suse.de/tests/2915205 (s390x) okay with issue for hostname_inst which should not be there
https://openqa.suse.de/tests/2915204 (ppc64le) okay with issue for hostname_inst which should not be there
https://openqa.suse.de/tests/2912003 (aarch64) okay
https://openqa.suse.de/tests/2915206 (spvm) okay with issue for hostname_inst which should not be there

#60 Updated by zluo 10 months ago

check:

https://openqa.suse.de/tests/2915201 (xen) okay with issue for hostname_inst which should not be there
https://openqa.suse.de/tests/2912213 (svirt-xen-pv) okay
https://openqa.suse.de/tests/2915202 (s390x-kvm) okay
https://openqa.suse.de/tests/2915203 (svirt-hyperv) okay

#61 Updated by zluo 10 months ago

mark: need to file a ticket for https://openqa.suse.de/tests/2912470#settings

#62 Updated by zluo 10 months ago

check issue with reboot after installation:
https://openqa.suse.de/tests/2915198

done, looks good.

#64 Updated by zluo 10 months ago

  • Status changed from In Progress to Feedback

#66 Updated by zluo 10 months ago

issues with hostname_inst from ppc64le-spvm and system_role failure from s390x-kvm-sle12 are not related.

#67 Updated by zluo 10 months ago

https://openqa.suse.de/tests/2933890 system_role works fine.

#68 Updated by zluo 10 months ago

  • Status changed from Feedback to In Progress

#69 Updated by zluo 10 months ago

  • Status changed from In Progress to Feedback

cannot set as resolved atm.

#70 Updated by zluo 10 months ago

  • Blocked by deleted (action #51725: [s390x][kvm] test fails in bootloader_zkvm)

#71 Updated by zluo 10 months ago

  • Status changed from Feedback to Resolved

#72 Updated by osukup 10 months ago

  • Status changed from Resolved to Feedback

#73 Updated by szarate 10 months ago

I vote to close this ticket and work better on defining #38819 rather than comming up with more and more tasks that are simply created out of nothing.

Assigning to PO to later discuss this.

#74 Updated by szarate 10 months ago

  • Assignee changed from zluo to mgriessmeier

#75 Updated by mgriessmeier 9 months ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF