open-switch / opx-nas-l2

https://openswitch.net
3 stars 11 forks source link

Why does nas_hash_cps.cpp only use dell-base-hash instead of using dell-base-switch-element? #34

Closed grantcurell closed 4 years ago

grantcurell commented 4 years ago

I'm looking into https://github.com/open-switch/opx-tools/issues/27 and want to implement a fix.

I'm still learning the code base, but after investigating opx-config-global-switch in opx-tools it looks like it reads from the CPS key base-switch/switching-entities/switching-entity which is defined in dell-base-switch-element.yang. However, the CPS server application responsible for the hash-fields attribute, the NAS daemon (with code defined in opx-nas-l2/src/switch/nas_hash_cps.cpp), uses dell-base-hash.yang instead. dell-base-hash.yang defines the exact same values as dell-base-switch-element.yang as far as I can tell.

This means when opx-config-global-switch runs:

parser = argparse.ArgumentParser(description='Set interface configuration')
for x in attrs + db_attrs:
    if x.cps_attr not in target_attrs:
        continue
    extra = {}
    if hasattr(x, 'metavar'):
        extra['metavar'] = x.metavar
    if hasattr(x, 'map'):
        extra['choices'] = x.map.keys()
    if hasattr(x, 'action'):
        extra['action'] = x.action
    parser.add_argument(x.option, help=x.help, type=getattr(x, 'type', str), **extra)
args = parser.parse_args()

base-switch/switching-entities/switching-entity/lag-hash-fields will never be present in target_attrs which leads to the problem described in https://github.com/open-switch/opx-tools/issues/27. What drove the use of dell-base-hash.yang instead of dell-base-switch-element.yang?

grantcurell commented 4 years ago

After spending a bunch of time rolling through the code and getting a much better idea of how this works - is the reason that you don't know ahead of time what hash capabilities a switch will provide? So on the Dell switch you have a separate YANG file which defines the specific hash capabilities on Dell hardware, but because opx-config-global-switch was written for the general case it doesn't reference those?

grantcurell commented 4 years ago

I haven't submitted yet, but I wrote a patch to fix the behavior: https://github.com/grantcurell/opx-tools/tree/opx-config-global-switch-add-hash-fields

grantcurell commented 4 years ago

Fixed in: https://github.com/open-switch/opx-tools/pull/29