Skip to content

[config/console][consutil] Support enable/disable console switch #1275

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.

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

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

Blueve
Copy link
Contributor

@Blueve Blueve commented Nov 27, 2020

- What I did
This is the implementation of HLD: https://github.com/Azure/SONiC/blob/master/doc/console/SONiC-Console-Switch-High-Level-Design.md#33136-enabledisable-console-switch-feature

  • This PR use below "CONSOLE_SWITCH:console_mgmt" instead the HLD mentioned key "CONSOLE_SWITCH" for better future change. I will update the HLD later.

This PR will allow user to enable/disable console switch management functions on a SONiC device.

- How I did it

  • Add config console {enable/disable} command
  • Exit directly if console swtich mgmt function not enabled when call console command
  • Add unit tests

- How to verify it

  1. Added unit test cases and all pass
  2. Build py-wheel package and tested on a physical SONiC DUT
admin@sonic:~$ consutil show
Console switch feature is disabled

admin@sonic:~$ consutil connect 1
Console switch feature is disabled

admin@sonic:~$ sudo consutil clear 1
Console switch feature is disabled

admin@sonic:~$ show line
Console switch feature is disabled

admin@sonic:~$ sudo sonic-clear line 1
Console switch feature is disabled

admin@sonic:~$ connect line 1
Console switch feature is disabled

admin@sonic:~$ sudo config console disable

admin@sonic:~$ show line
Console switch feature is disabled

admin@sonic:~$ sudo config console enable
admin@sonic:~$ show line
  Line    Baud    PID    Start Time    Device
------  ------  -----  ------------  --------
     0    9600      -             -
     1    9600      -             -

- Previous command output (if the output of a command-line utility has changed)
N/A

- New command output (if the output of a command-line utility has changed)
N/A

@Blueve Blueve requested a review from yxieca November 27, 2020 05:36
@yxieca yxieca requested review from jleveque and lguohan November 29, 2020 19:26
@yxieca
Copy link
Contributor

yxieca commented Nov 29, 2020

@jleveque, @lguohan want some input from you here:

  1. Console switch feature is not a containerized feature. Do you want to see the configuration be included in feature table or Jing's approach is fine? Add the control to feature table will cause some change in feature table dependent code.
  2. I think the current naming convention is use 'enabled' and 'disabled' to mark a state. (instead of using 'enable', disable').

@Blueve
Copy link
Contributor Author

Blueve commented Nov 30, 2020

@jleveque, @lguohan want some input from you here:

  1. Console switch feature is not a containerized feature. Do you want to see the configuration be included in feature table or Jing's approach is fine? Add the control to feature table will cause some change in feature table dependent code.
  2. I think the current naming convention is use 'enabled' and 'disabled' to mark a state. (instead of using 'enable', disable').

Great ask, I saw we have a FEATURE table already a few days ago and I am entangled too on if use it or follow the HLD. Seems like the FEATURE table have some options for service/container only and most features are not able to configure by minigraph.

For the naming convention, we really need that. I try search our doc: https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md and we have both {enable/disable} {enabled/disabled} documented, it would be never late to align them earlier.

@jleveque
Copy link
Contributor

@jleveque, @lguohan want some input from you here:

  1. Console switch feature is not a containerized feature. Do you want to see the configuration be included in feature table or Jing's approach is fine? Add the control to feature table will cause some change in feature table dependent code.

Currently, all entries in the FEATURE table are services which manage a container. I'm not sure if want to extend that to include this "feature". @lguohan to comment.

  1. I think the current naming convention is use 'enabled' and 'disabled' to mark a state. (instead of using 'enable', disable').

With regards to the CLI as it currently stands, config console enable/disable is acceptable. It might not align exactly with the "config feature ..." commands (e.g., config feature state snmp enabled), however, so we might want to standardize this convention across the entire CLI.

yxieca
yxieca previously approved these changes Nov 30, 2020
@Blueve
Copy link
Contributor Author

Blueve commented Dec 1, 2020

@jleveque, @lguohan want some input from you here:

  1. Console switch feature is not a containerized feature. Do you want to see the configuration be included in feature table or Jing's approach is fine? Add the control to feature table will cause some change in feature table dependent code.

Currently, all entries in the FEATURE table are services which manage a container. I'm not sure if want to extend that to include this "feature". @lguohan to comment.

Yes, actually this is a simply binary switch for console CLI, its quite different with a service since it can't 'restart' or 'auto-start'. But the FEATURE table is appropriate from the semantic aspect. Looking forward to your comments @lguohan

yxieca
yxieca previously approved these changes Dec 8, 2020
@Blueve Blueve merged commit e22980f into sonic-net:master Dec 9, 2020
@Blueve Blueve deleted the dev/jika/console_switch branch December 9, 2020 01:29
anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
…ic-net#1275)

* [config/console][consutil] Support enable/disable console switch
* Changed the key to aligned with feature table style

Signed-off-by: Jing Kan [email protected]
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.

3 participants