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

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

Merged
merged 3 commits into from
Aug 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39cb45c
Move hostname handling to hostcfgd
fastiuk Mar 24, 2022
26866a2
Move mgmt handling to hostcfgd
fastiuk Mar 30, 2022
64bca6e
Add test for hostname setting
fastiuk Jun 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 7 additions & 61 deletions config/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from sonic_py_common import device_info, multi_asic
from sonic_py_common.interface import get_interface_table_name, get_port_table_name, get_intf_longname
from utilities_common import util_base
from swsscommon import swsscommon
from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector
from utilities_common.db import Db
from utilities_common.intf_filter import parse_interface_in_filter
Expand Down Expand Up @@ -1889,19 +1890,11 @@ def hostname(new_hostname):

config_db = ConfigDBConnector()
config_db.connect()
config_db.mod_entry('DEVICE_METADATA' , 'localhost', {"hostname" : new_hostname})
try:
command = "service hostname-config restart"
clicommon.run_command(command, display_cmd=True)
except SystemExit as e:
click.echo("Restarting hostname-config service failed with error {}".format(e))
raise
config_db.mod_entry(swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, 'localhost',
{'hostname': new_hostname})

# Reload Monit configuration to pick up new hostname in case it changed
click.echo("Reloading Monit configuration ...")
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 •
edited
Loading

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.

' preserve setting, run `config save`.')

#
# 'synchronous_mode' command ('config synchronous_mode ...')
Expand Down Expand Up @@ -2827,22 +2820,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).

"""Restart interfaces-config service and NTP service when mvrf is changed"""
"""
When mvrf is enabled, eth0 should be moved to mvrf; when it is disabled,
move it back to default vrf. Restarting the "interfaces-config" service
will recreate the /etc/network/interfaces file and restart the
"networking" service that takes care of the eth0 movement.
NTP service should also be restarted to rerun the NTP service with or
without "cgexec" accordingly.
"""
cmd="service ntp stop"
os.system (cmd)
cmd="systemctl restart interfaces-config"
os.system (cmd)
cmd="service ntp start"
os.system (cmd)

def vrf_add_management_vrf(config_db):
"""Enable management vrf in config DB"""
Expand All @@ -2852,22 +2829,7 @@ def vrf_add_management_vrf(config_db):
click.echo("ManagementVRF is already Enabled.")
return None
config_db.mod_entry('MGMT_VRF_CONFIG', "vrf_global", {"mgmtVrfEnabled": "true"})
mvrf_restart_services()
"""
The regular expression for grep in below cmd is to match eth0 line in /proc/net/route, sample file:
$ cat /proc/net/route
Iface Destination Gateway Flags RefCnt Use Metric Mask MTU Window IRTT
eth0 00000000 01803B0A 0003 0 0 202 00000000 0 0 0
"""
cmd = r"cat /proc/net/route | grep -E \"eth0\s+00000000\s+[0-9A-Z]+\s+[0-9]+\s+[0-9]+\s+[0-9]+\s+202\" | wc -l"
proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)
output = proc.communicate()
if int(output[0]) >= 1:
cmd="ip -4 route del default dev eth0 metric 202"
proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)
proc.communicate()
if proc.returncode != 0:
click.echo("Could not delete eth0 route")


def vrf_delete_management_vrf(config_db):
"""Disable management vrf in config DB"""
Expand All @@ -2877,7 +2839,7 @@ def vrf_delete_management_vrf(config_db):
click.echo("ManagementVRF is already Disabled.")
return None
config_db.mod_entry('MGMT_VRF_CONFIG', "vrf_global", {"mgmtVrfEnabled": "false"})
mvrf_restart_services()


@config.group(cls=clicommon.AbbreviationGroup)
@click.pass_context
Expand Down Expand Up @@ -4113,20 +4075,6 @@ def _get_all_mgmtinterface_keys():
config_db.connect()
return list(config_db.get_table('MGMT_INTERFACE').keys())

def mgmt_ip_restart_services():
"""Restart the required services when mgmt inteface IP address is changed"""
"""
Whenever the eth0 IP address is changed, restart the "interfaces-config"
service which regenerates the /etc/network/interfaces file and restarts
the networking service to make the new/null IP address effective for eth0.
"ntp-config" service should also be restarted based on the new
eth0 IP address since the ntp.conf (generated from ntp.conf.j2) is
made to listen on that particular eth0 IP address or reset it back.
"""
cmd="systemctl restart interfaces-config"
os.system (cmd)
cmd="systemctl restart ntp-config"
os.system (cmd)

#
# 'mtu' subcommand
Expand Down Expand Up @@ -4274,7 +4222,6 @@ def add(ctx, interface_name, ip_addr, gw):
config_db.set_entry("MGMT_INTERFACE", (interface_name, str(ip_address)), {"NULL": "NULL"})
else:
config_db.set_entry("MGMT_INTERFACE", (interface_name, str(ip_address)), {"gwaddr": gw})
mgmt_ip_restart_services()

return

Expand Down Expand Up @@ -4314,7 +4261,6 @@ def remove(ctx, interface_name, ip_addr):

if interface_name == 'eth0':
config_db.set_entry("MGMT_INTERFACE", (interface_name, str(ip_address)), None)
mgmt_ip_restart_services()
return

table_name = get_interface_table_name(interface_name)
Expand Down
34 changes: 34 additions & 0 deletions tests/config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1585,3 +1585,37 @@ def test_config_rate(self, get_cmd_module, setup_single_broadcom_asic):
def teardown_class(cls):
print("TEARDOWN")
os.environ['UTILITIES_UNIT_TESTING'] = "0"


class TestConfigHostname(object):
@classmethod
def setup_class(cls):
print("SETUP")
import config.main
importlib.reload(config.main)

@mock.patch('config.main.ConfigDBConnector')
def test_hostname_add(self, db_conn_patch, get_cmd_module):
db_conn_patch().mod_entry = mock.Mock()
(config, show) = get_cmd_module

runner = CliRunner()
result = runner.invoke(config.config.commands["hostname"],
["new_hostname"])

# Verify success
assert result.exit_code == 0

# Check was called
args_list = db_conn_patch().mod_entry.call_args_list
assert len(args_list) > 0

args, _ = args_list[0]
assert len(args) > 0

# Check new hostname was part of args
assert {'hostname': 'new_hostname'} in args

@classmethod
def teardown_class(cls):
print("TEARDOWN")