Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added query Enum value capability for vxlan EVPN feature #920

Merged
merged 7 commits into from
Sep 15, 2021

Conversation

dgsudharsan
Copy link
Collaborator

Added query enum value capability for some attributes which are required for EVPN VxLAN feature.

kcudnik
kcudnik previously approved these changes Sep 4, 2021
@kcudnik
Copy link
Collaborator

kcudnik commented Sep 4, 2021

check build errors

kcudnik
kcudnik previously approved these changes Sep 5, 2021
@dgsudharsan
Copy link
Collaborator Author

/azpw run

@dgsudharsan
Copy link
Collaborator Author

check build errors

@kcudnik @prsunny Can you please start pipeline?

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 13, 2021

this test failure is not related to the PR

@dgsudharsan
Copy link
Collaborator Author

this test failure is not related to the PR

@kcudnik Can we merge this PR? What is the approach?

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 13, 2021

can you add unittests for this feature ? in sairedis/unittests/vslib/


if (object_type == SAI_OBJECT_TYPE_TUNNEL && attr_id == SAI_TUNNEL_ATTR_PEER_MODE)
{
return queryVxlanTunnelPeerModeCapability(enum_values_capability);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why there is Vxlan in in name here? there is no vxlan object type nor attribute in name, this is just for tunnel

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@dgsudharsan
Copy link
Collaborator Author

sairedis/unittests/vslib/

Sure. Can you please share the steps on how to run these unit tests?

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 13, 2021

yea, just type "make check" in test directory

@dgsudharsan
Copy link
Collaborator Author

yea, just type "make check" in test directory

I ran the unit tests

[----------] 5 tests from SwitchMLNX2700
[ RUN ] SwitchMLNX2700.ctr
[ OK ] SwitchMLNX2700.ctr (14 ms)
[ RUN ] SwitchMLNX2700.refresh_bridge_port_list
[ OK ] SwitchMLNX2700.refresh_bridge_port_list (10 ms)
[ RUN ] SwitchMLNX2700.warm_update_queues
[ OK ] SwitchMLNX2700.warm_update_queues (17 ms)
[ RUN ] SwitchMLNX2700.test_tunnel_term_capability
[ OK ] SwitchMLNX2700.test_tunnel_term_capability (4 ms)
[ RUN ] SwitchMLNX2700.test_vlan_flood_capability
[ OK ] SwitchMLNX2700.test_vlan_flood_capability (4 ms)
[----------] 5 tests from SwitchMLNX2700 (49 ms total)

[----------] 5 tests from SwitchBCM56850
[ RUN ] SwitchBCM56850.ctr
[ OK ] SwitchBCM56850.ctr (16 ms)
[ RUN ] SwitchBCM56850.refresh_bridge_port_list
[ OK ] SwitchBCM56850.refresh_bridge_port_list (11 ms)
[ RUN ] SwitchBCM56850.warm_update_queues
[ OK ] SwitchBCM56850.warm_update_queues (20 ms)
[ RUN ] SwitchBCM56850.test_tunnel_term_capability
[ OK ] SwitchBCM56850.test_tunnel_term_capability (5 ms)
[ RUN ] SwitchBCM56850.test_vlan_flood_capability
[ OK ] SwitchBCM56850.test_vlan_flood_capability (4 ms)
[----------] 5 tests from SwitchBCM56850 (56 ms total)

@dgsudharsan
Copy link
Collaborator Author

@kcudnik I have added tests to validate new APIs
However, the infrastructure itself 'works' outside sai library. If fake_platform isset it will choose appropriate saixml file. The infrastructure is tested in swss tests which make use of the fake_platform concept. sonic-net/sonic-swss#1858

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 14, 2021

fake_platfrom sholud not be used anywhere its a terrible idea to use that variable anywhere

@dgsudharsan
Copy link
Collaborator Author

fake_platfrom sholud not be used anywhere its a terrible idea to use that variable anywhere

The infrastructure cleanup it's a big work item that will be done in some later point of time. For now fake_platform isn't used anywhere inside sai-redis
Please let me know if the current changes are good. If so can you please approve this? We have a feature pending on this.

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 14, 2021

sonic project is a classic book example for "will be done in some later point" translates to never

int32_t list[2];
enum_val_cap.count = 2;
enum_val_cap.list = list;
EXPECT_EQ(sw.queryAttrEnumValuesCapability(0x2100000000, SAI_OBJECT_TYPE_TUNNEL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use 1 parameter per line if you want to break function params to multiple lines, also do this across this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dgsudharsan
Copy link
Collaborator Author

sonic project is a classic book example for "will be done in some later point" translates to never

Taking up the entire cleanup work is not within the timelines of the current feature. It's a new work item by itself. We have prioritized this work item as it is also required for us to specify platform and I agree fake_platform isn't a good approach.

@dgsudharsan
Copy link
Collaborator Author

The test failures are not related to changes in the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants