Next hop group with members.

[erohsik]

Introduce a new type of Next hop group that allows the application
to provide the list of next hops that are members of the Next
hop group along with the list of weights.

[itaibaz]

next hop group member objects are being created this way implicitly, without being returned to the application
we don't support such flow in any place AFAIK
If we go with the approach here, application should then query the next hop list of each group created, then query the next hop id, and thus learn the object ids. This seems like a must step, as application should then be able to manage and remove these next hop group members
So while the motivation for the PR is clear, it creates complexity

[erohsik]

next hop group member objects are being created this way implicitly, without being returned to the application we don't support such flow in any place AFAIK If we go with the approach here, application should then query the next hop list of each group created, then query the next hop id, and thus learn the object ids. This seems like a must step, as application should then be able to manage and remove these next hop group members So while the motivation for the PR is clear, it creates complexity

Thanks for taking a look. An example of the modification workflow is in the doc. With this next hop group, if the next hop group starts with {a,b,c,d} and then transitions to {a,b,e}, then the group is modified with the next hop list set to {a,b,e}. There is no explicit deletion of members c and d or addition of e.

[itaibaz]

should we mark then
SAI_NEXT_HOP_GROUP_ATTR_NEXT_HOP_MEMBER_LIST
as not valid for this type of group ?

and add clarification in the MD, that

1. no next hop group member objects are created here
2. to change next hops set a new full list
3. group can be removed without a need to remove group members first
   ?

[erohsik]

should we mark then SAI_NEXT_HOP_GROUP_ATTR_NEXT_HOP_MEMBER_LIST as not valid for this type of group ?

and add clarification in the MD, that

1. no next hop group member objects are created here

2. to change next hops set a new full list

3. group can be removed without a need to remove group members first
   ?

Thanks for the review. I have updated the PR.

[itaibaz]

perhaps change this to @validonly with all types except ecmp with members, so this is included in the metadata

thanks for all other changes

[rlhui]

@erohsik , is this addressed? good to get @itaibaz to approve as well.

[rck-innovium]

@erohsik  This is a useful enhancement. Thanks for that.

I have a comment on the way this is modelled in this proposal. The model is maintaining two different lists: one for the NHs and the other for the weights of the corresponding NH. We cannot atomically add a new NH to a created group- we need [step-1] call to add the NH to group, and [step-2] another call to set the weight. The weights to be programmed in the h/w are inconsistent between step-1 and step-2.

This is not elegant and tomorrow if we want to support this say for Ordered ECMP, we would need another list to specify the 'Sequence Id'.  We could extend this for every ECMP feature like Fine Grained etc.

A cleaner way would be to do one of the below:

Option-1: reuse  sai_next_hop_group_member:

Make SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID mandatory only if NH Group is NOT of type SAI_NEXT_HOP_GROUP_TYPE_ECMP_WITH_MEMBERS.

Make the object type of SAI_NEXT_HOP_GROUP_ATTR_NEXT_HOP_LIST as SAI_OBJECT_TYPE_NEXT_HOP_GROUP_MEMBER and remove SAI_NEXT_HOP_GROUP_ATTR_NEXT_HOP_MEMBER_WEIGHT_LIST

Option-2: create a new type of sai_next_hop_group_init_member

A challenge with option#1 would be to enforce it via SAI metacheckers and backward compatibility. If option#1 is not possible, then we can consider creating a new object type like below (definitely needs a better name).

typedef enum _sai_next_hop_group_init_member_attr_t
{

:
:
    /**
     * @brief Next hop id
     *
     * @type sai_object_id_t
     * @flags MANDATORY_ON_CREATE | CREATE_AND_SET
     * @objects SAI_OBJECT_TYPE_NEXT_HOP, SAI_OBJECT_TYPE_NEXT_HOP_GROUP
     */
    SAI_NEXT_HOP_GROUP_INIT_MEMBER_ATTR_NEXT_HOP_ID,
  
    /**
     * @brief Member weights
     *
     *
     * @type sai_uint32_t
     * @flags CREATE_AND_SET
     * @default 1
     */
    SAI_NEXT_HOP_GROUP_INIT_MEMBER_ATTR_WEIGHT,
        
 :
 :
 }

[rck-innovium]

Please consider using a object type instead of maintaining the weights as another list.

[erohsik]

@erohsik This is a useful enhancement. Thanks for that.

I have a comment on the way this is modelled in this proposal. The model is maintaining two different lists: one for the NHs and the other for the weights of the corresponding NH. We cannot atomically add a new NH to a created group- we need [step-1] call to add the NH to group, and [step-2] another call to set the weight. The weights to be programmed in the h/w are inconsistent between step-1 and step-2.

Thanks for the review.
The document describes the modify workflow. The application would bulk set the two attributes (new NH list, new weight list) in one step..

[helloanandhi]

Creating a generic solution for all next hop group types would be more future extensible.

[rck-innovium]

@erohsik This is a useful enhancement. Thanks for that.
I have a comment on the way this is modelled in this proposal. The model is maintaining two different lists: one for the NHs and the other for the weights of the corresponding NH. We cannot atomically add a new NH to a created group- we need [step-1] call to add the NH to group, and [step-2] another call to set the weight. The weights to be programmed in the h/w are inconsistent between step-1 and step-2.

Thanks for the review. The document describes the modify workflow. The application would bulk set the two attributes (new NH list, new weight list) in one step..

The current solution looks to be a point fix for a specific NHG type like WECPM and not for other NHG type. How would you handle the case of supporting counters per member Nexthop using the single step workflow?

[kcudnik]

you will need to rebase on origin/master and force push to pass metadata check

[rck-innovium]

Please update to:

sai_bulk_object_set_attribute_fn set_next_hop_groups_attribute()

[erohsik]

Thanks for the review. Updated the calls..