Flush all FDB entries if no attributes specified

[andriy-kokhan]

As per enum sai_fdb_flush_attr_t description:

 * For example:
 *
 * 1) Flush all entries in FDB table - Do not specify any attribute
   . . . . .

Also, it's logically to flush all types of FDB entries in case either no attributes specified or just BV_ID or BRIDGE_PORT_ID.

[kcudnik]

This is breaking change and I this was done on purpose, since probably you don't want to flush entries you created statically, let say your network hosts, but flush dynamic by default of all learned entries + @lguohan, let's discuss this on SAI community meeting

[andriy-kokhan]

This is breaking change and I this was done on purpose, since probably you don't want to flush entries you created statically, let say your network hosts, but flush dynamic by default of all learned entries + @lguohan, let's discuss this on SAI community meeting

Yeh, you are right. But anyway we should either fix a default value or enum description. At this point, they are contradictory. I'm fine with both ways.

[kcudnik]

This is breaking change and I this was done on purpose, since probably you don't want to flush entries you created statically, let say your network hosts, but flush dynamic by default of all learned entries + @lguohan, let's discuss this on SAI community meeting

Yeh, you are right. But anyway we should either fix a default value or enum description. At this point, they are contradictory. I'm fine with both ways.

ahh i see your point now, ok, in the syncd we just forward call to vendor: https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L598 and then we process it https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L639 based on attributes https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/RedisClient.cpp#L877 and default type is obtained here:
https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L609 which is encoded, and set as SAI says, but this should be obtained from sai metadata to be more correct, based on that , the description should be changed, and in flush default value should be obtained if not attribute is specified, which in this case should be DYNAMIC
and description here should be fixed

[andriy-kokhan]

This is breaking change and I this was done on purpose, since probably you don't want to flush entries you created statically, let say your network hosts, but flush dynamic by default of all learned entries + @lguohan, let's discuss this on SAI community meeting

Yeh, you are right. But anyway we should either fix a default value or enum description. At this point, they are contradictory. I'm fine with both ways.

ahh i see your point now, ok, in the syncd we just forward call to vendor: https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L598 and then we process it https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L639 based on attributes https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/RedisClient.cpp#L877 and default type is obtained here: https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/Syncd.cpp#L609 which is encoded, and set as SAI says, but this should be obtained from sai metadata to be more correct, based on that , the description should be changed, and in flush default value should be obtained if not attribute is specified, which in this case should be DYNAMIC and description here should be fixed

I'd expect that in case no attributes specified then SAI library implementation should take the default values based on SAI headers (or meta). I mean it's not mandatory for the application (syncd in our case) to obtain the default values and provide them explicitly to SAI library.

Also, I've just check Linux bridge command description (skipping not relevant parameters):

bridge fdb flush dev DEV [ brport DEV ] [ vlan VID ] [ [no]permanent | [no]static | [no]dynamic ]

Looks like bridge fdb flush dev DEV should flush all FDB entries.
So, I still tending to change the default to _ALL for SAI as well.

@lguohan , your input will be greatly appreciated. Thanks

[kcudnik]

added fix in syncd sonic-net/sonic-sairedis#1317 to get that value form SAI metadata

[kcudnik]

bridge fdb flush dev DEV [ brport DEV ] [ vlan VID ] [ [no]permanent | [no]static | [no]dynamic ]

Looks like `bridge fdb flush dev DEV` should flush all FDB entries. So, I still tending to change the default to `_ALL` for SAI as well.

thats a point to discuss whether we should behave like linux bridge command

[kcudnik]

Instead of chainging default value can you update description?

[andriy-kokhan]

Instead of chainging default value can you update description?

done

[rck-innovium]

Looks good.