[abdosi]

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.

[tahmed-dev]

@abdosi, @jleveque : I am wondering if the change should be done in the hostcfgd. Another approach is to template the change into init_cfg.json.j2 and move the template rendering out of sonic_debian_extension.j2 ro rc.local under FIRST_BOOT_FILE flag there. the reason is to keep hostcfg as simple and multi-asic agnostic.

[jleveque]

the reason is to keep hostcfg as simple and multi-asic agnostic.

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.

[abdosi]

the reason is to keep hostcfg as simple and multi-asic agnostic.

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.

[jleveque]

@abdosi: Good points. I guess it isn't as easy to move the init_cfg.json generation to first boot as I hoped.

[abdosi]

@jleveque and @tahmed-dev can you please help review this.

[tahmed-dev]

the reason is to keep hostcfg as simple and multi-asic agnostic.

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 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.

[abdosi]

the reason is to keep hostcfg as simple and multi-asic agnostic.

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 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.

@tahmed-dev Updated accordingly.

[tahmed-dev]

can we make is_multi_npu() a class member?

[abdosi]

@tahmed-dev Done

[tahmed-dev]

lgtm. minor comment regarding the is_multi_npu being class member.

[ngoc-do]

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.