Skip to content

Ensure correct value from/to SERIALCONSOLE#24927

Open
waynechen55 wants to merge 1 commit intoos-autoinst:masterfrom
waynechen55:wayne/update_serial_console
Open

Ensure correct value from/to SERIALCONSOLE#24927
waynechen55 wants to merge 1 commit intoos-autoinst:masterfrom
waynechen55:wayne/update_serial_console

Conversation

@waynechen55
Copy link
Contributor

@waynechen55 waynechen55 commented Mar 12, 2026

@waynechen55 waynechen55 marked this pull request as draft March 12, 2026 04:48
@waynechen55 waynechen55 force-pushed the wayne/update_serial_console branch 9 times, most recently from 4b90217 to c4e6187 Compare March 12, 2026 09:40
@waynechen55 waynechen55 marked this pull request as ready for review March 12, 2026 09:51
@waynechen55
Copy link
Contributor Author

Welcome your review @alice-suse @guoxuguang @Julie-CAO @RoyCai7 @nanzhg @tbaev @martinsmarcelo Please help have a look if you are interested and leave a message.

@alice-suse
Copy link
Contributor

If it has been ensured for my 2 comments, and all validation is ok, I think it will be good enough.

@waynechen55
Copy link
Contributor Author

waynechen55 commented Mar 18, 2026

If it has been ensured for my 2 comments, and all validation is ok, I think it will be good enough.

I want to further clarify that this pull request is just to ensure serial console gets reset to default value ttyS1 instead of something totally different from SERIALDEV if it has no value. This pull request does not change any other things and does not intend to complicate things further either. All validation is OK. @alice-suse

@alice-suse
Copy link
Contributor

If it has been ensured for my 2 comments, and all validation is ok, I think it will be good enough.

I want to further clarify that this pull request is just to ensure serial console gets reset to default value ttyS1 instead of something totally different from SERIALDEV if it has no value. This pull request does not change any other things and does not intend to complicate things further either. All validation is OK. @alice-suse

Thanks for the explanation. Now I understand your motivation better.

To me, the code logic has changed a little bit and updates the (best-practice or necessary) requirements when setting ipmi workers. Maybe others are not that deep into the code as you about the relevant parts. Would you point out explicitly what's the change/new requirement now to configure ipmi workers? Will it additionally require CORRECT SETTINGs for SERIALDEV and SERIALCONSOLE for any ipmi worker? Let's also involve other ipmi worker users for awareness, @czerw @rfan1 @Dawei-Pang @JoyceNa .

@waynechen55
Copy link
Contributor Author

waynechen55 commented Mar 18, 2026

If it has been ensured for my 2 comments, and all validation is ok, I think it will be good enough.

I want to further clarify that this pull request is just to ensure serial console gets reset to default value ttyS1 instead of something totally different from SERIALDEV if it has no value. This pull request does not change any other things and does not intend to complicate things further either. All validation is OK. @alice-suse

Thanks for the explanation. Now I understand your motivation better.

To me, the code logic has changed a little bit and updates the (best-practice or necessary) requirements when setting ipmi workers. Maybe others are not that deep into the code as you about the relevant parts. Would you point out explicitly what's the change/new requirement now to configure ipmi workers? Will it additionally require CORRECT SETTINGs for SERIALDEV and SERIALCONSOLE for any ipmi worker? Let's also involve other ipmi worker users for awareness, @czerw @rfan1 @Dawei-Pang @JoyceNa .

Basically no requirements from others.

I think they can just bear in mind that SERIALCONSOLE is the most safe setting to be used with serial console, so they can start using SERIALCONSOLE instead of SERIALDEV in the future.

But this does not exclude use of SERIALDEV if someone would like to stick to this setting because this pull request does not change it.

I want to reiterate that this pull request is just to reset default value of SERIALCONSOLE instead of changing underlying logic. There is no logic change. And there is no change to SERIALDEV either.

@waynechen55 waynechen55 force-pushed the wayne/update_serial_console branch from c4e6187 to 5c2654d Compare March 18, 2026 03:03
@alice-suse
Copy link
Contributor

LGTM

@czerw czerw requested review from czerw and mdoucha March 18, 2026 07:25
@czerw
Copy link
Contributor

czerw commented Mar 18, 2026

Commit message is missing important information, why we actually want such a change. What is broken?
Please add more details to commit message and also failing job examples to description.

@JoyceNa
Copy link
Contributor

JoyceNa commented Mar 18, 2026

LGTM since no change to SERIALDEV

@waynechen55 waynechen55 force-pushed the wayne/update_serial_console branch 2 times, most recently from ca691bd to efdaaab Compare March 18, 2026 08:05
@waynechen55
Copy link
Contributor Author

Commit message is missing important information, why we actually want such a change. What is broken? Please add more details to commit message and also failing job examples to description.

Already updated. @czerw

@waynechen55 waynechen55 force-pushed the wayne/update_serial_console branch from efdaaab to fd57e09 Compare March 18, 2026 08:13
1.For OSD workers, some machines use ttyS0 and some others use ttyS1 or even ttyS2
which is assigned to setting SERIALDEV. But SERIALDEV is not safe enough to be used
everywhere, because it becomes sshserial during test run, here is an example:
https://openqa.suse.de/tests/21287146/file/vars.json.
2.Merge request https://gitlab.suse.de/openqa/salt-pillars-openqa/-/merge_requests/1225
introduces setting SERIALCONSOLE which is already being widely used in automation code.
This can avoid incorrect SERIALCONSOLE leading to test run failure, for exmaple:
https://openqa.suse.de/tests/21287262#step/prepare_transactional_server/74.
3.Current subroutine save_serial_console resets SERIALCONSOLE, if it has no value, to
default value obtained from SERIALDEV which may become sshserial. So it is necessary
to check and assign more meaningful default value ttyS1 to SERIALCONSOLE. And ttyS1 is
also the most widely used serial console.
@waynechen55 waynechen55 force-pushed the wayne/update_serial_console branch from fd57e09 to 55c6d15 Compare March 19, 2026 07:03
@waynechen55
Copy link
Contributor Author

record_info already added. @czerw

@waynechen55
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants