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.

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

[CLI] Move hostname, mgmt interface/vrf config to hostcfgd #2173

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

fastiuk
Copy link
Contributor

@fastiuk fastiuk commented May 18, 2022

Why I did it

To be able to configure the management interface and hostname standalone by changing database config at runtime.
From the CLI perspective fo view, the following behavior is the same. But now you have two ways of configuring it: CLI, directly through the database.

How I did it

Moved configuration part of the interface and hostname to "hostcfgd".

How to verify it

  • Built an image
  • Flash it to the switch
  • Run CLI commands
# Set IP address: verify address is set on the iface
sudo config interface ip add eth0 10.210.25.127/22 10.210.24.1
ip address show eth0
# 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
#     link/ether 98:03:9b:a2:be:80 brd ff:ff:ff:ff:ff:ff
#     inet 10.210.25.127/22 brd 10.210.27.255 scope global eth0
#        valid_lft forever preferred_lft forever
#     inet6 fe80::9a03:9bff:fea2:be80/64 scope link
#       valid_lft forever preferred_lft forever

# Remove IP address: verify you received address form DHCP
sudo config interface ip remove eth0 10.210.25.127/22
ip address show eth0
# 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
#     link/ether 98:03:9b:a2:be:80 brd ff:ff:ff:ff:ff:ff
#     inet 10.210.25.127/22 brd 10.210.27.255 scope global eth0
#        valid_lft forever preferred_lft forever
#     inet6 fe80::9a03:9bff:fea2:be80/64 scope link
#       valid_lft forever preferred_lft forever

# Enable/disable mgmt VRF
ip address show mgmt
# Device "mgmt" does not exist.

sudo config vrf add mgmt
ip address show mgmt
# 72: mgmt: <NOARP,MASTER,UP,LOWER_UP> mtu 65575 qdisc noqueue state UP group default qlen 1000
#     link/ether fa:9b:ad:7b:1e:83 brd ff:ff:ff:ff:ff:ff

sudo config vrf del mgmt
ip address show mgmt
# Device "mgmt" does not exist.

# Setting the hostname
admin@r-anaconda-27:~$ sudo config hostname bla
# Login / Logout
admin@bla:~$

Description for the changelog

  • Moved management interface configuration to hostcfgd.
  • Moved management VRF configuration to hostcfgd.
  • Moved hostname configuration to hostcfgd.

@ghost
Copy link

ghost commented May 18, 2022

CLA assistant check
All CLA requirements met.

@fastiuk fastiuk changed the title Move cli to hostcfgd [CLI] Move hostname, mgmt interface/vrf config to hostcfgd May 19, 2022
Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

Please add UT for the hostname add. Else the coverage will fail.

@liat-grozovik
Copy link
Collaborator

@fastiuk when do you think UT will be avaialble so we can review and signoff?

@fastiuk
Copy link
Contributor Author

fastiuk commented Jun 20, 2022

@fastiuk when do you think UT will be avaialble so we can review and signoff?

Done

dgsudharsan
dgsudharsan previously approved these changes Jun 21, 2022
@fastiuk
Copy link
Contributor Author

fastiuk commented Jun 21, 2022

@dgsudharsan Please take a look at this one as well. It is part of the same changes: sonic-net/sonic-buildimage#10868

clicommon.run_command("sudo monit reload")

click.echo("Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.")
click.echo('Please note loaded setting will be lost after system reboot. To'
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 17, 2022

Choose a reason for hiding this comment

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

(

Could you use click.wrap_text() to wrap long line instead of manually wrap? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am wrapping just the code string here. The log line in runtime will be the same as it was before this change.

@qiluo-msft qiluo-msft requested a review from prsunny July 17, 2022 22:57
@@ -2671,22 +2664,6 @@ def warm_restart_bgp_eoiu(ctx, enable):
db = ctx.obj['db']
db.mod_entry('WARM_RESTART', 'bgp', {'bgp_eoiu': enable})

def mvrf_restart_services():
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the code to restart services are moved to hostcfgd. What is the benefit that we are getting by this approach? What is missing in the current approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The benefit here is that you can configure your switch without the cli, by just writing configuration data at runtime to the Redis DB.
With the current approach, the only way to load a new MGMT iface config was using sonic cli.
The new approach will allow you to configure it without calling sonic cli. You just write config to DB and voila, everything will be configured.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that is expected to be supported. We use CLI to configure the DB and there are multiple validations done at the CLI level (For e.g, validate IP address corrrectness etc). So why should not we use the CLI here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree.
Example: SONiC has the ability to configure syslog servers. It has a dedicated table for it but doesn't have cli to do such configuration. That means that the only way to configure it is through the DB.
Another example: What if you don't need CLI at all? You have SNMP server and it configures your switch. It won't be possible if commands which are required to make configuration reside here in cli.

Copy link
Contributor

Choose a reason for hiding this comment

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

afaik, there is CLI for syslog, you can also refer this. #2191. The point is, if there is a need for configuring after the initial config, then it should ideally have a CLI. We don't need CLI for those configs that can be applied at startup (via config_db.json or equivalent..) and not to be changed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is, if there is a need for configuring after the initial config, then it should ideally have a CLI.

Agree. But we can do it without CLI as well, right? It is not prohibited, SONiC is an open Linux system, so why a user can't just use it the way it wants?

We don't need CLI for those configs that can be applied at startup (via config_db.json or equivalent..)

Agree, so that is why I proposing those changes. It allows users to have yet another way to configure its switch. The CLI is still here, so you can still configure your switch via CLI (And most users will do so).

fastiuk added 3 commits July 20, 2022 17:25
Signed-off-by: Yevhen Fastiuk <[email protected]>
Signed-off-by: Yevhen Fastiuk <[email protected]>
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Discussed offline on the use-case. Signing-off from my side.

@liat-grozovik
Copy link
Collaborator

/easycla

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@fastiuk
Copy link
Contributor Author

fastiuk commented Aug 8, 2022

/easycla

@liat-grozovik liat-grozovik merged commit 14c483f into sonic-net:master Aug 10, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 11, 2022
Update sonic-utilities submodule pointer to include the following:
* Convert IPv6 addresses to lowercase in apply-patch ([sonic-net#2299](sonic-net/sonic-utilities#2299))
* [CLI] Move hostname, mgmt interface/vrf config to hostcfgd ([sonic-net#2173](sonic-net/sonic-utilities#2173))
* [config][muxcable] add support to enable/disable ycable telemetry ([sonic-net#2297](sonic-net/sonic-utilities#2297))

Signed-off-by: dprital <[email protected]>
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 11, 2022
Update sonic-utilities submodule pointer to include the following:
* Convert IPv6 addresses to lowercase in apply-patch ([#2299](sonic-net/sonic-utilities#2299))
* [CLI] Move hostname, mgmt interface/vrf config to hostcfgd ([#2173](sonic-net/sonic-utilities#2173))
* [config][muxcable] add support to enable/disable ycable telemetry ([#2297](sonic-net/sonic-utilities#2297))

Signed-off-by: dprital <[email protected]>
qiluo-msft pushed a commit to sonic-net/sonic-host-services that referenced this pull request Aug 21, 2022
This PR depends on sonic-net/sonic-utilities#2173

#### Why I did it
To be able to configure the management interface and hostname standalone by changing database config at runtime.
From the CLI perspective fo view, the following behavior is the same. But now you have two ways of configuring it: CLI, directly through the database.

#### How I did it
Moved configuration part of the interface and hostname to "hostcfgd".

#### How to verify it
* Built an image
* Flash it to the switch
* Run CLI commands
```
# Set IP address: verify address is set on the iface
sudo config interface ip add eth0 10.210.25.127/22 10.210.24.1
ip address show eth0
# 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
#     link/ether 98:03:9b:a2:be:80 brd ff:ff:ff:ff:ff:ff
#     inet 10.210.25.127/22 brd 10.210.27.255 scope global eth0
#        valid_lft forever preferred_lft forever
#     inet6 fe80::9a03:9bff:fea2:be80/64 scope link
#       valid_lft forever preferred_lft forever

# Remove IP address: verify you received address form DHCP
sudo config interface ip remove eth0 10.210.25.127/22
ip address show eth0
# 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
#     link/ether 98:03:9b:a2:be:80 brd ff:ff:ff:ff:ff:ff
#     inet 10.210.25.127/22 brd 10.210.27.255 scope global eth0
#        valid_lft forever preferred_lft forever
#     inet6 fe80::9a03:9bff:fea2:be80/64 scope link
#       valid_lft forever preferred_lft forever

# Enable/disable mgmt VRF
ip address show mgmt
# Device "mgmt" does not exist.

sudo config vrf add mgmt
ip address show mgmt
# 72: mgmt: <NOARP,MASTER,UP,LOWER_UP> mtu 65575 qdisc noqueue state UP group default qlen 1000
#     link/ether fa:9b:ad:7b:1e:83 brd ff:ff:ff:ff:ff:ff

sudo config vrf del mgmt
ip address show mgmt
# Device "mgmt" does not exist.

# Setting the hostname
admin@r-anaconda-27:~$ sudo config hostname bla
# Login / Logout
admin@bla:~$
```
#### Description for the changelog
* Moved management interface configuration to hostcfgd.
* Moved management VRF configuration to hostcfgd.
* Moved hostname configuration to hostcfgd.

#### Submodules PR's :

| Repo  | PR title | State |
| ----------------- | ----------------- | ----------------- |
| sonic-utilities | [[CLI] Move hostname, mgmt interface/vrf config to hostcfgd](sonic-net/sonic-utilities#2173) | ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/Azure/sonic-utilities/2173) |
@fastiuk fastiuk deleted the dev-move-cli-to-hostcfgd branch August 30, 2022 07:13
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
…#2173)

- Why I did it
To be able to configure the management interface and hostname standalone by changing database config at runtime.
From the CLI perspective fo view, the following behavior is the same. But now you have two ways of configuring it: CLI, directly through the database.

- How I did it
Moved configuration part of the interface and hostname to "hostcfgd".

- How to verify it
Built an image
Flash it to the switch
Run CLI commands

Signed-off-by: Yevhen Fastiuk <[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.

5 participants