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.

Sign up for GitHub

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

Jump to bottom

[vlan] Add support of VLAN host interface #1645

Merged
merged 4 commits into from
Apr 8, 2021
Merged

[vlan] Add support of VLAN host interface #1645

merged 4 commits into from
Apr 8, 2021

Conversation

volodymyrsamotiy
Copy link
Collaborator

  • Infrastructure needed for the VNET ping tool

Signed-off-by: Volodymyr Samotiy volodymyrs@nvidia.com

What I did

Add support of VLAN host interface

Why I did it

It is infrastructure needed for the VNET ping tool

How I verified it
Configured new VLAN with "hostif_name" attribute and verified that it created in HW

Details if related
N/A

@volodymyrsamotiy
[vlan] Add support of VLAN host interface
125e407 
* Infrastructure needed for the VNET ping tool

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
@lguohan lguohan requested a review from prsunny February 18, 2021 04:07
@lguohan
Copy link
Contributor

lguohan commented Feb 18, 2021

missing pytest

prsunny
prsunny reviewed Mar 4, 2021
View reviewed changes
orchagent/portsorch.cpp Outdated
{
if (!createVlanHostIntf(vl, hostif_name))
{
throw runtime_error("Cannot create VLAN host interface");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets handle the failure gracefully as this is for monitoring Vlan. We can erase it and continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

cfgmgr/vlanmgr.cpp
@@ -362,6 +363,10 @@ void VlanMgr::doVlanTask(Consumer &consumer)
mac = fvValue(i);
setHostVlanMac(vlan_id, mac);
}
else if (fvField(i) == "hostif_name")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean, for every Vlan thats in VNET, a corresponding host if shall be created?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, since it is per VLAN attribute and created for specific VLAN.

@volodymyrsamotiy
[vlan] Add support of VLAN host interface
3619498 
* Add VS test

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
@volodymyrsamotiy volodymyrsamotiy mentioned this pull request Mar 4, 2021
Merged
@volodymyrsamotiy
Copy link
Collaborator Author

In order for VS test to pass first need to merge the following PR for "vslib": sonic-net/sonic-sairedis#804

@liat-grozovik
Copy link
Collaborator

@prsunny can you please review?

@liat-grozovik
Copy link
Collaborator

In order for VS test to pass first need to merge the following PR for "vslib": Azure/sonic-sairedis#804

@volodymyrsamotiy it was merged. please check that submodule is updated and then retest.

@volodymyrsamotiy
[vlan] Add support of VLAN host interface
c4cdd71 
* Handle gracefully faiure for creating monitoring VLAN hostif

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
@prsunny
Copy link
Collaborator

prsunny commented Apr 6, 2021

retest this please

@prsunny
Copy link
Collaborator

prsunny commented Apr 6, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

prsunny
prsunny reviewed Apr 6, 2021
View reviewed changes
orchagent/portsorch.cpp Outdated
@@ -3689,6 +3753,12 @@ bool PortsOrch::removeVlan(Port vlan)
return false;
}

if (!removeVlanHostIntf(vlan))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This must be called only if host_intf_id is present, else skip

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

orchagent/portsorch.cpp Outdated
if (!createVlanHostIntf(vl, hostif_name))
{
// No need to fail in case of error as this is for monitoring VLAN.
// Error message is printed by "createVlanHostIntf" so just handle faiure gracefully.
Copy link
Collaborator

Choose a reason for hiding this comment

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

failure - typo

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

@prsunny
Copy link
Collaborator

prsunny commented Apr 7, 2021

@volodymyrsamotiy , can you please take a look at the test failures?

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@volodymyrsamotiy
[vlan] Add support of VLAN host interface
a2fa21e 
* Fix review comments

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
@volodymyrsamotiy
Copy link
Collaborator Author

Fixed last review comments and test failure

@prsunny prsunny merged commit ca8ba6d into sonic-net:master Apr 8, 2021
@abdosi
Copy link
Contributor

abdosi commented Apr 8, 2021

Need pr for 201911. cherry-pick has conflict.

@prsunny @volodymyrsamotiy

yxieca pushed a commit that referenced this pull request Apr 8, 2021
@volodymyrsamotiy @yxieca
[vlan] Add support of VLAN host interface (#1645)
72ae85d 
* [vlan] Add support of VLAN host interface
* Infrastructure needed for the VNET ping tool

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
abdosi pushed a commit that referenced this pull request Apr 8, 2021
@volodymyrsamotiy @abdosi
[vlan] Add support of VLAN host interface (#1645)
5ff3d8c 
* [vlan] Add support of VLAN host interface
* Infrastructure needed for the VNET ping tool

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
@abdosi
Copy link
Contributor

abdosi commented Apr 8, 2021

Included without dvs test. Please create new pr for dvs test in 201911.

@abdosi
Copy link
Contributor

abdosi commented Apr 10, 2021 •
edited
Loading

@volodymyrsamotiy we should optimize the PR to use addHostIntfs() as this can support VLAN based Host router interface. There should not be need of new set of create/remove API's().

We can just add this check

    if (port.m_type == Port::VLAN)
    {
        attr.value.oid = port.m_vlan_info.vlan_oid;
    }
    else
    {
        attr.value.oid = port.m_port_id;
    }

@prsunny

This was referenced Apr 30, 2021
Closed
Closed
Merged
Merged
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 4, 2021
@volodymyrsamotiy
[Mellanox] [202012] Update SAI submodule pointer (#7499)
4152c3e 
- Why I did it
To include below changes:
Set monitoring VLAN hostif up dy default (for VNET ping tool)

- How I did it
Updated SAI submodule pointer

- How to verify it
Create VLAN hostif according to changes in PR: sonic-net/sonic-swss#1645
Verify it is admin up by default

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
@volodymyrsamotiy @raphaelt-nvidia
[vlan] Add support of VLAN host interface (sonic-net#1645)
3faff04 
* [vlan] Add support of VLAN host interface
* Infrastructure needed for the VNET ping tool

Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prsunny prsunny prsunny approved these changes

Assignees
No one assigned
Labels
Included in 201911 Branch Included in 202012 Branch request_for_201911_branch Request for 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@volodymyrsamotiy @lguohan @liat-grozovik @prsunny @abdosi @yxieca