-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Enhanced Feature Table state enable/disable for multi-asic platforms. #5358
Conversation
In Multi-asic for some features we can service per asic so we need to get list of all services. Also updated logic to return if any one of systemctl command return failure and make sure syslog of feature getting enable/disable only come when all commads are sucessful. Moved the service list get api from sonic-util to sonic-py-common Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
@abdosi, @jleveque : I am wondering if the change should be done in the |
I agree with this sentiment. I can't think of a reason we need to build init_cfg.json into the image. We could generate it on first boot. |
Thanks @tahmed-dev for the quick discussion. @jleveque generating init_cfg.json during boot time is not possible as it depends on some compile time variable. Also to make hostcfgd agnostic of multi-asic we will need to update Feature table to have service instance count for each feature for which we have to read again generated_service_conf file during first boot and can't be just template based. |
@abdosi: Good points. I guess it isn't as easy to move the init_cfg.json generation to first boot as I hoped. |
@jleveque and @tahmed-dev can you please help review this. |
@abdosi what about extending information about services in the init_cfg.json regarding the where the feature should run host/per namespace/both. The hostcfgd can then check the multi_npu flag and act accordingly. |
Made init_cfg.json.j2 knowledegable of Feature service is global scope or per asic scope Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
@tahmed-dev Updated accordingly. |
files/image_config/hostcfgd/hostcfgd
Outdated
# Create feature name suffix depending feature is running in host or namespace or in both | ||
feature_name_suffix_list = (([feature_name] if has_global_scope or not device_info.is_multi_npu() else []) + | ||
([(feature_name + '@' + str(asic_inst)) for asic_inst in range(device_info.get_num_npus()) | ||
if has_per_asic_scope and device_info.is_multi_npu()])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make is_multi_npu() a class member?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tahmed-dev Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. minor comment regarding the is_multi_npu being class member.
Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
…#5358) * Enhanced Feature Table state enable/disbale for multi-asic platforms. In Multi-asic for some features we can service per asic so we need to get list of all services. Also updated logic to return if any one of systemctl command return failure and make sure syslog of feature getting enable/disable only come when all commads are sucessful. Moved the service list get api from sonic-util to sonic-py-common Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Make sure to retun None for both service list in case of error. Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Return empty list as fail condition Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Address Review Comments. Made init_cfg.json.j2 knowledegable of Feature service is global scope or per asic scope Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Fix merge conflict * Address Review Comment. Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> Co-authored-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
for suffix in feature_suffixes: | ||
start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name_suffix, suffix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great work. I know it merged already, but wanted to follow up the story here if we have plan to support enabling/disabling services for particular asics in system?
More specifically, says there are 2 asics in system, we want to disable lldp on asic 1, but not asic 2.
I imagine that to do that, we need
(1) Create entry like FEATURE|lldp|1
with enabled = false
and FEATURE|lldp|2
with enabled = true
. hostcfgd listens and reacts to the change. It will turn off lldp@1.service
but keep lldp@2.service
on.
Or (2) Create hostcfgd@1
for asic 1 and hostcfgd@2
for asic 2. hostcfgd@1
listens database@1 CONFIG_DB
, and reacts to FEATURE|lldp
. Same for hostcfgd@2
which listens database@2 CONFIG_DB
. But this one may not work if we want to enable/disable database. If database doesn't exits, hostcfgd@x
will have nothing to listen and react.
…sonic-net#5358) * Enhanced Feature Table state enable/disbale for multi-asic platforms. In Multi-asic for some features we can service per asic so we need to get list of all services. Also updated logic to return if any one of systemctl command return failure and make sure syslog of feature getting enable/disable only come when all commads are sucessful. Moved the service list get api from sonic-util to sonic-py-common Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Make sure to retun None for both service list in case of error. Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Return empty list as fail condition Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Address Review Comments. Made init_cfg.json.j2 knowledegable of Feature service is global scope or per asic scope Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> * Fix merge conflict * Address Review Comment. Signed-off-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net> Co-authored-by: Abhishek Dosi <abdosi@abdosi-ubuntu-vm0.nwp1qucpfg5ejooejenqshkj3e.cx.internal.cloudapp.net>
What/Why I did:
Change 1:
Enhanced Feature Table state enable/disable for multi-asic platforms.
In Multi-asic for some features can have service per asic so enhanced
Feature Table Generation to have scope (global or per asic)
of each feature service.
In Single asic platforms all service will run in global/host scope
In Multi-asic services can run in global/host scope (eg snmp) , per_asic scope (eg swss, syncd, etc..) or both (eg lldp)
Without above change all feature/services were getting enable/disable in global scope also
for multi-asic platforms.
Change 2:
Also updated logic to return if any one of systemctl command return failure
and make sure syslog of feature getting enable/disable only come when
all commands are successful.
How I verify:
Verified feature enable/disable on both single and multi-asic platforms.