search

sonic-net / DASH

Disaggregated APIs for SONiC Hosts
Apache License 2.0
78 stars 88 forks source link

[dash-pipeline] Populate attributes with default values #547

Closed kcudnik closed 2 months ago

kcudnik commented 3 months ago

When calling create function, populate attributes that have default value present to overcome BMv2 limitations.

kcudnik commented 3 months ago

@Pterosaur can you check whether this failure could be related ? fail is in one of python test

kcudnik commented 3 months ago

we still need to wait for @Pterosaur to test this change

Pterosaur commented 3 months ago

we still need to wait for @Pterosaur to test this change

Yes, I'm working on that.

Pterosaur commented 3 months ago

Hi, @r12f @marian-pritsak could you please help to check a BMv2-P4-SAI error about ENI dscp?

When I tested this PR, I got an error like: 

Apr 12 13:20:07.297741 vlab-01 ERR syncd#syncd: e:- mutateTableEntry: GRPC ERROR[2]: , #010#002#032`#012#037type.googleapis.com/p4.v1.Error#022=#010#003#022&Unexpected number of action parameters#032#021ALL-sswitch-p4org
Apr 12 13:20:07.297741 vlab-01 ERR syncd#syncd: e:- mutateTableEntry: GRPC call Write::INSERT ERROR: table_id: 45859274 match { field_id: 1 exact { value: "\000\000" } } action { action { action_id: 27167550 params { param_id: 8 value: "\000\000" } params { param_id: 2 value: "\000\000\'\020" } params { param_id: 1 value: "\000\000\003\350" } params { param_id: 3 value: "\000\000\000\n" } params { param_id: 4 value: "\000" } params { param_id: 6 value: "\n\000\001\002" } params { param_id: 7 value: "\000\020\341" } params { param_id: 5 value: "\000\000" } params { param_id: 9 value: "\000\000\000\000" } params { param_id: 10 value: "\000\000\000\000" } params { param_id: 11 value: "\000\000\000\000" } params { param_id: 12 value: "\000\000" } params { param_id: 13 value: "\000\000" } params { param_id: 14 value: "\000\000" } params { param_id: 16 value: "\000\000" } params { param_id: 17 value: "\000\000" } params { param_id: 18 value: "\000\000" } params { param_id: 19 value: "\000\000" } params { param_id: 20 value: "\000\000" } params { param_id: 21 value: "\000\000" } params { param_id: 22 value: "\000\000" } params { param_id: 23 value: "\000\000" } params { param_id: 24 value: "\000\000" } params { param_id: 25 value: "\000\000" } params { param_id: 26 value: "\000\000" } params { param_id: 27 value: "\000\000" } params { param_id: 28 value: "\000\000" } params { param_id: 29 value: "\000\000" } params { param_id: 30 value: "\000\000" } params { param_id: 31 value: "\000\000" } params { param_id: 32 value: "\000\000" } params { param_id: 33 value: "\000\000" } params { param_id: 34 value: "\000\000" } params { param_id: 35 value: "\000\000" } params { param_id: 36 value: "\000" } } }

According to the above log, it indicates the param_id 15 ,dscp, is missing. image

image

The flag validonly told SAI, the parameter, dscp, will only be provided if SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE == SAI_DASH_TUNNEL_DSCP_MODE_PIPE_MODEL. In the SAI's definition, if the validonly is false, related parameters shouldn't be provided.

But according to the BMv2 requirement, every parameter of an action should be provided as function parameters (No default value is supported).

Here is a conflict between BMv2, SAI and our P4 description.

Could anyone help to fix this?

kcudnik commented 3 months ago

im a litle bit confused ? we spoke that when validonly is provided then its required to pass, but only when condition is true and have default value, and my change is doing that, but here bmv2 requires that even if condition is false ?

can you also attach syslog ? to see which attributes were passed with validonly ?

Pterosaur commented 3 months ago

im a litle bit confused ? we spoke that when validonly is provided then its required to pass, but only when condition is true and have default value, and my change is doing that, but here bmv2 requires that even if condition is false ?

can you also attach syslog ? to see which attributes were passed with validonly ?

Yes, In this PR, if the condition of validonly flag is false, you will not provide the default value. I believe it's just what we want.

But in the P4 code that I mentioned above, even the condition is false, it expects a value that will not used.

The key syslog is:

Apr 15 16:35:13.937046 vlab-01 NOTICE swss#orchagent: :- addApplianceEntry: Created vip and direction lookup entries for 123
Apr 15 16:35:13.937413 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_HA_SCOPE_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_PL_SIP with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_PL_SIP_MASK with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_PL_UNDERLAY_SIP with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_V4_METER_POLICY_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_V6_METER_POLICY_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V4_STAGE1_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V4_STAGE2_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V4_STAGE3_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V4_STAGE4_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V4_STAGE5_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V6_STAGE1_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V6_STAGE2_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V6_STAGE3_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V6_STAGE4_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_INBOUND_V6_STAGE5_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V4_STAGE1_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937612 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V4_STAGE2_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937651 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V4_STAGE3_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937651 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V4_STAGE4_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937651 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V4_STAGE5_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937651 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V6_STAGE1_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937651 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V6_STAGE2_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937832 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V6_STAGE3_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937832 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V6_STAGE4_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937832 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_OUTBOUND_V6_STAGE5_DASH_ACL_GROUP_ID with default value
Apr 15 16:35:13.937832 vlab-01 NOTICE syncd#syncd: n:- populateDefaultAttributes: adding SAI_ENI_ATTR_DISABLE_FAST_PATH_ICMP_FLOW_REDIRECTION with default value

The condition of flag is the default value FALSE and you didn't provide any value for that. This behavior is totally correct in the most of SAI calls except this one.

222     SAI_ENI_ATTR_V6_METER_POLICY_ID,
223
224     /**
225      * @brief Action set_eni_attrs parameter DASH_TUNNEL_DSCP_MODE
226      *
227      * @type sai_dash_tunnel_dscp_mode_t
228      * @flags CREATE_AND_SET
229      * @default SAI_DASH_TUNNEL_DSCP_MODE_PRESERVE_MODEL
230      */
231     SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE,
232
233     /**
234      * @brief Action set_eni_attrs parameter DSCP
235      *
236      * @type sai_uint8_t
237      * @flags CREATE_AND_SET
238      * @default 0
239      * @validonly SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE == SAI_DASH_TUNNEL_DSCP_MODE_PIPE_MODEL
240      */
241     SAI_ENI_ATTR_DSCP,
Pterosaur commented 3 months ago

@kcudnik I have a question, is there any requirement that the condition variables of validonly should be placed in front of other variables in the info->attrmetadata? If no this requirement, maybe we should do a first loop for validonly variable.

kcudnik commented 3 months ago

i dont understand the question, what do you mean in front ?

Pterosaur commented 3 months ago

i dont understand the question, what do you mean in front ?

@kcudnik Just ignore my question, I checked the implementation of sai_metadata_is_validonly_met. This PR is good.

kcudnik commented 3 months ago

So is this good to go ? Not sure why it's causing the test to fail

r12f commented 3 months ago

since this pattern will be very helpful for us in the future, for example, multiple modes. I wonder if we can ignore the validonly tag in default value generation, it the attributes is not *_ATTR_ACTION? this should be able to solve this issue for good.

Pterosaur commented 3 months ago

since this pattern will be very helpful for us in the future, for example, multiple modes. I wonder if we can ignore the validonly tag in default value generation, it the attributes is not *_ATTR_ACTION? this should be able to solve this issue for good.

Hi @r12f , I believe your reply is for this question.

Hi, @r12f @marian-pritsak could you please help to check a BMv2-P4-SAI error about ENI dscp?

When I tested this PR, I got an error like:

Apr 12 13:20:07.297741 vlab-01 ERR syncd#syncd: e:- mutateTableEntry: GRPC ERROR[2]: , #010#002#032`#012#037type.googleapis.com/p4.v1.Error#022=#010#003#022&Unexpected number of action parameters#032#021ALL-sswitch-p4org
Apr 12 13:20:07.297741 vlab-01 ERR syncd#syncd: e:- mutateTableEntry: GRPC call Write::INSERT ERROR: table_id: 45859274 match { field_id: 1 exact { value: "\000\000" } } action { action { action_id: 27167550 params { param_id: 8 value: "\000\000" } params { param_id: 2 value: "\000\000\'\020" } params { param_id: 1 value: "\000\000\003\350" } params { param_id: 3 value: "\000\000\000\n" } params { param_id: 4 value: "\000" } params { param_id: 6 value: "\n\000\001\002" } params { param_id: 7 value: "\000\020\341" } params { param_id: 5 value: "\000\000" } params { param_id: 9 value: "\000\000\000\000" } params { param_id: 10 value: "\000\000\000\000" } params { param_id: 11 value: "\000\000\000\000" } params { param_id: 12 value: "\000\000" } params { param_id: 13 value: "\000\000" } params { param_id: 14 value: "\000\000" } params { param_id: 16 value: "\000\000" } params { param_id: 17 value: "\000\000" } params { param_id: 18 value: "\000\000" } params { param_id: 19 value: "\000\000" } params { param_id: 20 value: "\000\000" } params { param_id: 21 value: "\000\000" } params { param_id: 22 value: "\000\000" } params { param_id: 23 value: "\000\000" } params { param_id: 24 value: "\000\000" } params { param_id: 25 value: "\000\000" } params { param_id: 26 value: "\000\000" } params { param_id: 27 value: "\000\000" } params { param_id: 28 value: "\000\000" } params { param_id: 29 value: "\000\000" } params { param_id: 30 value: "\000\000" } params { param_id: 31 value: "\000\000" } params { param_id: 32 value: "\000\000" } params { param_id: 33 value: "\000\000" } params { param_id: 34 value: "\000\000" } params { param_id: 35 value: "\000\000" } params { param_id: 36 value: "\000" } } }

According to the above log, it indicates the param_id 15 ,dscp, is missing. image

image

The flag validonly told SAI, the parameter, dscp, will only be provided if SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE == SAI_DASH_TUNNEL_DSCP_MODE_PIPE_MODEL. In the SAI's definition, if the validonly is false, related parameters shouldn't be provided.

But according to the BMv2 requirement, every parameter of an action should be provided as function parameters (No default value is supported).

Here is a conflict between BMv2, SAI and our P4 description.

Could anyone help to fix this?

If my understanding is correct, it means when we populate the default value, we will only check the validonly flag to variables with postfix _ATTR_ACTION

To my question, because the condition variable is SAI_ENI_ATTR_DASH_TUNNEL_DSCP_MODE that doesn't have the postfix _ATTR_ACTION, so we should populate a default value for SAI_ENI_ATTR_DSCP. @r12f Please correct me if anything I misunderstood. Or @kcudnik Do you have any questions about Riff's suggestion?

kcudnik commented 3 months ago

since this pattern will be very helpful for us in the future, for example, multiple modes. I wonder if we can ignore the validonly tag in default value generation, it the attributes is not *_ATTR_ACTION? this should be able to solve this issue for good.

which specific attributes are you talking about ? which object type ?

Pterosaur commented 3 months ago

since this pattern will be very helpful for us in the future, for example, multiple modes. I wonder if we can ignore the validonly tag in default value generation, it the attributes is not *_ATTR_ACTION? this should be able to solve this issue for good.

which specific attributes are you talking about ? which object type ?

https://github.com/opencomputeproject/SAI/blob/2587c3b89241022bb0eef3ef82ab764eda7a183c/experimental/saiexperimentaldasheni.h#L215-L232

r12f commented 3 months ago

since this pattern will be very helpful for us in the future, for example, multiple modes. I wonder if we can ignore the validonly tag in default value generation, it the attributes is not *_ATTR_ACTION? this should be able to solve this issue for good.

which specific attributes are you talking about ? which object type ?

Hi @kcudnik , for every DASH object that supports multiple types of arguments, it will have a *_ATTR_ACTION attribute, for example CA-PA mapping, it will have a SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION attribute:

image

this is the only attribute that determines how many number of arguments will be expected, so I wonder if we can ignore other @validonly conditions in the checks. This will solve this problem for good.

kcudnik commented 3 months ago

do you mean that this action attribute if it has default value it should be always passed ?

r12f commented 3 months ago

i see. let me share some more concrete examples:

When CA-PA mapping is programmed, depends on the value of action attribute, we can populate other default values based on the validonly tag, that is related to SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION attribute. For example:

image

However, for ENI, it doesn't have any SAI_ENI_ATTR_ACTION attribute. And for SAI_ENI_ATTR_DSCP, the validonly tag doesn't contain any SAI_ENI_ATTR_ACTION condition, hence we still set the default value for it, if it is missing.

image

Hope this explains.

r12f commented 3 months ago

@marian-pritsak as FYI. this is the default value generation problem I was referring to.

kcudnik commented 3 months ago

i see. let me share some more concrete examples:

When CA-PA mapping is programmed, depends on the value of action attribute, we can populate other default values based on the validonly tag, that is related to SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION attribute. For example:

image

However, for ENI, it doesn't have any SAI_ENI_ATTR_ACTION attribute. And for SAI_ENI_ATTR_DSCP, the validonly tag doesn't contain any SAI_ENI_ATTR_ACTION condition, hence we still set the default value for it, if it is missing.

image

Hope this explains.

this is confusing, could you explain this more ? it seems like default value whether should passed or not, it depends on condition itselft whether it contains ACTION attribute ?

let imagine this situation:

we have object A (from DASH let say) it have default value defined and user didn't passed this attribute explicitly, there are some for that:

  1. there is no valid only condition,
  2. there is validonly condition which contains the same object ACTION attribute, a) condition is met b) condition is not met
  3. there is validonly condition which don't contain same object ACTION attribute a) condition is met b) condition is not met

also, notice that condition can be expressed as OR condition with multiple equals, and one of them could be ACTION attribute, what then if ACTION is not met, but other condition is met ? and vice versa ?

which those scenarios my code should pass default attribute as extra ?

Pterosaur commented 2 months ago

i see. let me share some more concrete examples: When CA-PA mapping is programmed, depends on the value of action attribute, we can populate other default values based on the validonly tag, that is related to SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION attribute. For example: image However, for ENI, it doesn't have any SAI_ENI_ATTR_ACTION attribute. And for SAI_ENI_ATTR_DSCP, the validonly tag doesn't contain any SAI_ENI_ATTR_ACTION condition, hence we still set the default value for it, if it is missing. image Hope this explains.

this is confusing, could you explain this more ? it seems like default value whether should passed or not, it depends on condition itselft whether it contains ACTION attribute ?

let imagine this situation:

we have object A (from DASH let say) it have default value defined and user didn't passed this attribute explicitly, there are some for that:

  1. there is no valid only condition,
  2. there is validonly condition which contains the same object ACTION attribute, a) condition is met b) condition is not met
  3. there is validonly condition which don't contain same object ACTION attribute a) condition is met b) condition is not met

also, notice that condition can be expressed as OR condition with multiple equals, and one of them could be ACTION attribute, what then if ACTION is not met, but other condition is met ? and vice versa ?

which those scenarios my code should pass default attribute as extra ?

Hi @kcudnik ,

The default value whether should passed or not, it totally depends on condition itselft whether it contains ACTION attribute.

  1. there is no valid only condition, Always, Set the default value Because if no valid, it means this object only has one default ACTION.
  2. there is validonly condition which contains the same object ACTION attribute, a) condition is met Set the default value b) condition is not met Don't set the default value
  3. there is validonly condition which don't contain same object ACTION attribute a) condition is met b) condition is not met Same as the first scenario, always set the default value

I see there are some combination conditions, like OR condition with multiple equals. I feel we just to ignore these non-ACTION checker.

@r12f Please correct me if anything I'm misleading.

kcudnik commented 2 months ago

Hi @kcudnik ,

The default value whether should passed or not, it totally depends on condition itselft whether it contains ACTION attribute.

  1. there is no valid only condition, Always, Set the default value Because if no valid, it means this object only has one default ACTION.
  2. there is validonly condition which contains the same object ACTION attribute, a) condition is met Set the default value b) condition is not met Don't set the default value
  3. there is validonly condition which don't contain same object ACTION attribute a) condition is met b) condition is not met Same as the first scenario, always set the default value

I see there are some combination conditions, like OR condition with multiple equals. I feel we just to ignore these non-ACTION checker.

@r12f Please correct me if anything I'm misleading.

ok this clears up, 3rd option needs to be implemented to set default value even if condition is NOT met, but contains ACTION attribute, i will update PR

kcudnik commented 2 months ago

wow and it's passing now with this case :D