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

Fixing virtio console for users #16190

Merged
merged 5 commits into from
Jan 24, 2023
Merged

Fixing virtio console for users #16190

merged 5 commits into from
Jan 24, 2023

Conversation

pevik
Copy link
Contributor

@pevik pevik commented Jan 6, 2023 •
edited
Loading

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@pevik pevik marked this pull request as draft January 6, 2023 15:36
@pevik pevik force-pushed the virtio-console-user branch 2 times, most recently from bd17f33 to cf1e5f9 Compare January 9, 2023 11:42
@pevik pevik mentioned this pull request Jan 9, 2023
Merged
@pevik pevik force-pushed the virtio-console-user branch 6 times, most recently from ded31bd to 33e027a Compare January 10, 2023 14:03
pevik
pevik commented Jan 10, 2023
View reviewed changes
lib/serial_terminal.pm Outdated Show resolved Hide resolved
pevik
pevik commented Jan 10, 2023
View reviewed changes
lib/serial_terminal.pm
@@ -354,6 +374,7 @@ sub select_serial_terminal {
}

die "No support for backend '$backend', add it" if (!defined $console) || ($console eq '');
$testapi::distri->{serial_term_prompt} = $prompt;
Copy link
Contributor Author

@pevik pevik Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Together with this call. Or should be prompt be handled in select_console() (optionally, probably part of another effort)?
Calling this can bring problems somewhere. At least SAP tests (and others, which set $serial_term_prompt should be carefully tested.

@pevik pevik changed the title [WIP] Virtio console for user [WIP] Fixing virtio console for users Jan 11, 2023
@pevik pevik mentioned this pull request Jan 11, 2023
Merged
cfconrad
cfconrad reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

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

Without having tested it, it looks good to me.
Regarding the serial_term_prompt, maybe it should be stored somehow in each console. And get's read from there when activate such console. But this is only needed if you switch between the consoles and using custom prompts, not sure if it is worth the effort.

lib/serial_terminal.pm Outdated Show resolved Hide resolved
asmorodskyi
asmorodskyi reviewed Jan 11, 2023
View reviewed changes
lib/serial_terminal.pm Outdated Show resolved Hide resolved
pevik
pevik commented Jan 11, 2023
View reviewed changes
lib/susedistribution.pm Outdated Show resolved Hide resolved
@pevik pevik force-pushed the virtio-console-user branch 2 times, most recently from 48542aa to e4efa3a Compare January 18, 2023 13:00
@pevik pevik changed the title [WIP] Fixing virtio console for users Fixing virtio console for users Jan 18, 2023
@pevik pevik marked this pull request as ready for review January 18, 2023 21:47
@pevik pevik force-pushed the virtio-console-user branch 2 times, most recently from 4b4af5f to cc2efda Compare January 19, 2023 09:51
@pevik
Copy link
Contributor Author

pevik commented Jan 19, 2023

@b10n1k FYI Pushed changes, which fixes HPC tests when only this repo is merged: http://quasar.suse.cz/tests/2121 http://quasar.suse.cz/tests/2122 http://quasar.suse.cz/tests/2119 http://quasar.suse.cz/tests/2120

@pevik
Copy link
Contributor Author

pevik commented Jan 23, 2023

Only updated perldoc.

b10n1k
b10n1k approved these changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@b10n1k b10n1k left a comment

Choose a reason for hiding this comment

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

lets do that :)

Martchus
Martchus requested changes Jan 23, 2023
View reviewed changes
lib/serial_terminal.pm Outdated Show resolved Hide resolved
@pevik pevik force-pushed the virtio-console-user branch 3 times, most recently from 6204604 to b8e99a2 Compare January 23, 2023 12:45
@pevik
Copy link
Contributor Author

pevik commented Jan 23, 2023 

Name "OpenQA::Isotovideo::Interface::version" used only once: possible typo at lib/susedistribution.pm line 433.

How to fix this? Do I need to use get_version()?

@pevik
susedistribution: Run user virtio console on it's own socket
4a89bfb 
This allows switching between root and non-root user.

NOTE: passing socked_path is possible only to updated os-autoinst
backend/qemu.pm (must have this path in virtio_console_names())
thus guarded with $OpenQA::Isotovideo::Interface::version ge 35
detection (changes in 20d0119b ("backend/qemu.pm: Add
virtio_console_user fifo name")).

Console names should be handled in library (why tests should care about
the internals?), but for now at least properly document that.

This also requires to add new getty (serial console) to
system_prepare.pm (will be working with newly generated images) and
shift numbers in wickedbase.pm. This will work also on older
backend/qemu.pm, getty just won't be used.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
select_serial_terminal(): Update prompt
eea4f0f 
This fixes switching from user and serial console.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
virtio_console: Test also non-root user
d988768 
Print id and running serial-getty services (debugging).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
hpc/mpi_master: Add -p to mkdir
55bd9e9 
This way is directory always created, but avoids error:

mkdir: cannot create directory /home/bernhard/bin: File exists

Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
hpc/mpi_master: Adjust serial prompt
58d9d69 
This needs to be done to get user serial terminal working again,
likely due: 2fcbb2b ("select_serial_terminal(): Update prompt")

Guarded by $OpenQA::Isotovideo::Interface::version ge 35 (changes in
20d0119b ("backend/qemu.pm: Add virtio_console_user fifo name")).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
Copy link
Contributor Author

pevik commented Jan 23, 2023

  1. Use isotovideo::get_version() instead of $OpenQA::Isotovideo::Interface::version to fix error on older perl 5.26: Name "OpenQA::Isotovideo::Interface::version" used only once: possible typo at lib/susedistribution.pm line 433..
  2. Use >= for numeric comparison (ge is primarily for strings).

PR should be ready now (rerun all tests with and without os-autoinst/os-autoinst#2240). Waiting if anybody has any more comments. @cfconrad @asmorodskyi @dzedro @jlausuch any comment? Or could you please approve?

cfconrad
cfconrad approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

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

LGTM

@pevik pevik merged commit fc0da61 into os-autoinst:master Jan 24, 2023
@pdostal
Copy link
Member

pdostal commented Jan 24, 2023

This is amazing! Thank you Petr

@ggardet
Copy link
Collaborator

ggardet commented Jan 26, 2023

This breaks on RPi3/4 tests (generalhw backend): https://openqa.opensuse.org/tests/3065106#step/system_prepare/8

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

@b10n1k b10n1k b10n1k approved these changes

@dzedro dzedro dzedro approved these changes

@Martchus Martchus Martchus approved these changes

@cfconrad cfconrad cfconrad approved these changes

@jlausuch jlausuch Awaiting requested review from jlausuch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@pevik @pdostal @ggardet @b10n1k @Vogtinator @dzedro @Martchus @asmorodskyi @mloviska @cfconrad