Project

General

Profile

action #45146

action #44843: [functional][u][epic] Cleanup the use of serial-/virtio-/ssh-consoles in our tests (was: use $self->select_serial_terminal instead of checking IPMI in every module)

[functional][u] Replace wrappers of wrappers for consoles with proper 'select_console' calls

Added by okurz over 1 year ago. Updated 3 months ago.

Status:
New
Priority:
Normal
Category:
Enhancement to existing tests
Target version:
SUSE QA tests - Milestone 31
Start date:
2018-12-13
Due date:
% Done:

0%

Estimated time:
Difficulty:
Duration:

Description

Motivation

See #44843#note-4 . We should use proper consoles from os-autoinst and not write wrappers of wrappers as consoles should provide already enough abstraction.

Acceptance criteria

  • AC1: Tasks 1 and 2 are done.

Tasks

  1. Handle all remote consoles in the activate_console method: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/susedistribution.pm#L681
  2. Refactor the code so that it properly uses select_console('root-console') without wrappers nor if-else conditions.

Related issues

Related to openQA Tests - action #54650: [functional][u] test fails in boot_to_desktopResolved2019-07-25

History

#1 Updated by mgriessmeier over 1 year ago

  • Target version changed from Milestone 23 to Milestone 25

moving to M25

#2 Updated by szarate about 1 year ago

  • Priority changed from Normal to High

To be re-refined :)

#3 Updated by SLindoMansilla about 1 year ago

  • Description updated (diff)
  • Status changed from New to Workable
  • Priority changed from High to Normal
  • Target version changed from Milestone 25 to Milestone 26

As spoken in the grooming meeting

#4 Updated by jorauch about 1 year ago

  • Assignee set to jorauch

#5 Updated by jorauch about 1 year ago

  • Status changed from Workable to In Progress

Task 1 should be done by replacing https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/susedistribution.pm#L681 with || is_remote_backend
So now I need to find all the wrappers

#6 Updated by szarate about 1 year ago

jorauch just to be 100% clear: You mean current line 685 right? :)

#7 Updated by jorauch about 1 year ago

Indeed, I mean the new Line 685

#8 Updated by jorauch about 1 year ago

  • Status changed from In Progress to Feedback

#9 Updated by SLindoMansilla about 1 year ago

  • Status changed from Feedback to Workable

Feedback provided in PR, removing from feedback queue

#10 Updated by jorauch about 1 year ago

  • Status changed from Workable to In Progress

#11 Updated by SLindoMansilla about 1 year ago

PR merged, waiting for verification on OSD

#12 Updated by jorauch about 1 year ago

  • Related to action #54650: [functional][u] test fails in boot_to_desktop added

#13 Updated by jorauch about 1 year ago

  • Status changed from In Progress to Feedback

After the emergency PR has been merged we should be done with this ticket?

#14 Updated by SLindoMansilla about 1 year ago

jorauch wrote:

After the emergency PR has been merged we should be done with this ticket?

After the verification on OSD, yes

#15 Updated by jorauch about 1 year ago

  • Status changed from Feedback to Resolved

Nothing broke the last 11 days, I guess that counts as verification

#16 Updated by pvorel almost 1 year ago

Could be next step of this to use select_serial_terminal() (instead of select_console 'root-console'), which is wrapper trying to handle everything. It's adjustable with variables (VIRTIO_CONSOLE=0 to disable virtio console; SERIAL_CONSOLE=0 to disable svirt serial console).

Nobody is forced to use virtio console / serial console, but on the other hand it's much faster and we hear complains about lack of resources (osd workers being under heavy load).

#17 Updated by okurz 12 months ago

pvorel wrote:

Could be next step of this to use select_serial_terminal() (instead of select_console 'root-console')

the idea was actually the opposite, to use select_console 'whatever-console-you-define' instead of the wrapper. Of course the console "whatever-console-you-define" can exactly be the serial terminal whenever configured. As we can freely create the consoles and activate them within the test code all this should be possible without needing to change any os-autoinst code.

#18 Updated by pvorel 12 months ago

But simple select_console 'root-console' just ignores the fact that some people want virtio console. Having wrapper which just select "proper console" makes tests more portable and avoid if/else code in tests.

When I created select_serial_terminal() I wanted to have smart way to select console. I also thought that speed of openQA is important thus virtio console should be used as much as it can (i.e. when needles can be avoided, which should IMHO be in consoletests, but tests/console/consoletest_setup.pm does needle matching due testing fonts, it's a question if this must be done in every test).
But that was wrong assumption, as there are probably more tests which needs needles (maybe there could be workaround in needles functions to switch to root-console, do the matching and then switch back).

But now I understand it, that select_console 'root-console' is for all tests matching needles and select_serial_terminal() for kernel and any console tests (which don't work with needles).
Assuming that there is now no need to have any wrapper for needles based testing regardless used backend? (will select_console 'root-console' always work on any backend without any other helper function).
Than the state should be documented, so it's clear.

#19 Updated by okurz 12 months ago

  • Status changed from Resolved to Feedback

I hope you all agree that it makes sense to keep this ticket open when the discussion is not yet resolved? Setting to "Feedback".

pvorel Sure, I agree with all your points regarding a smart way to select consoles based on needs and preferring the faster serial terminal where possible. I think we are not too far from our goal. Maybe it can be as simple as reworking the function select_serial_terminal() into an according console selection? Maybe this needs to be moved to os-autoinst and also makes sense to have there for the benefit of multiple test distributions. So that e.g. select_console 'root-serial-terminal' would do what select_serial_terminal() does so far? Same for select_console 'user-serial-terminal' replacing select_user_serial_terminal?

#20 Updated by pvorel 12 months ago

okurz wrote:

I hope you all agree that it makes sense to keep this ticket open when the discussion is not yet resolved? Setting to "Feedback".

+1

pvorel Sure, I agree with all your points regarding a smart way to select consoles based on needs and preferring the faster serial terminal where possible. I think we are not too far from our goal. Maybe it can be as simple as reworking the function select_serial_terminal() into an according console selection?

I don't understand how you want to achieve it.
The goal for select_serial_terminal() was not to have parameters, but set preferences via variables (thus code is simple).

I'd be for keeping select_console() and select_serial_terminal() as it is + add select_needles_console() (need better way), which would be equivalent of select_console 'user-serial-terminal' (e.g. both ways use wrapper, not just console based testing).
But that might be strange/radical idea.

Maybe this needs to be moved to os-autoinst and also makes sense to have there for the benefit of multiple test distributions.

So that e.g. select_console 'root-serial-terminal' would do what select_serial_terminal() does so far? Same for select_console 'user-serial-terminal' replacing select_user_serial_terminal?

I originally wanted to have it in the library. But it depends on variables which are set only in our testing repository, that's why it's there. IMHO generally there are some parts in our test repo which should be in the library, but are in test repo due the difficulty to make changes in the library. To put it into library, the decision would have to be some general mechanism or at least on variables which aren't specific on our testing repository.

#21 Updated by okurz 12 months ago

hm, this sounds like we have a conflicting proposal to the os-autoinst design. I suggest you wait for coolo's return and ask for his opinion on the architectural design.

#22 Updated by SLindoMansilla 12 months ago

  • Assignee changed from jorauch to okurz

Assigning to okurz until discussion is resolved.
Feel free to reassign.

#23 Updated by okurz 12 months ago

  • Assignee changed from okurz to pvorel

I feel like pvorel is way more active in the implementation so over to him

#24 Updated by mgriessmeier 12 months ago

  • Target version changed from Milestone 26 to Milestone 27

#25 Updated by mgriessmeier 11 months ago

  • Target version changed from Milestone 27 to Milestone 28

#26 Updated by mgriessmeier 7 months ago

  • Target version changed from Milestone 28 to Milestone 31

any update?

#27 Updated by pvorel 7 months ago

  • Assignee deleted (pvorel)

Removing myself. I've expressed my opinion in comment 18 and 20. I'm all for further simplification, moving code to functions instead of adding if/else clauses into tests, I just don't see the point how to move code of select_serial_terminal() (wrapper) into select_console().
If anybody implements something I'll be glad to review && test.

#28 Updated by SLindoMansilla 3 months ago

  • Status changed from Feedback to New
  • Assignee set to SLindoMansilla

Also available in: Atom PDF