Skip to content

Skip removing vnet namespace in Linux CNI Multitenancy #1841

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

Merged
merged 8 commits into from
Mar 9, 2023
Merged

Skip removing vnet namespace in Linux CNI Multitenancy #1841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
54c93a8
skip removing vnet namespace
tamilmani1989 Mar 2, 2023
6d5f56e
Merge branch 'master' of https://github.com/Azure/azure-container-net…
tamilmani1989 Mar 7, 2023
faf0100
disable deleting namespace testcase
tamilmani1989 Mar 7, 2023
49a5553
lint fix
tamilmani1989 Mar 7, 2023
775f3e2
Merge branch 'master' into tamanoha/deletens_fix
tamilmani1989 Mar 8, 2023
dc35667
Merge branch 'master' into tamanoha/deletens_fix
tamilmani1989 Mar 8, 2023
3d7fd68
Merge branch 'master' into tamanoha/deletens_fix
tamilmani1989 Mar 9, 2023
b6cebe7
Merge branch 'master' into tamanoha/deletens_fix
tamilmani1989 Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions network/transparent_vlan_endpointclient_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,18 @@ func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, g

log.Printf("[transparent vlan] There are %d routes remaining after deletion", routesLeft)

if routesLeft <= numDefaultRoutes {
Copy link
Contributor

@vipul-21 vipul-21 Mar 7, 2023

Choose a reason for hiding this comment

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

Just curious, will commenting this out won't affect the unit test cases we run for it ? https://github.com/Azure/azure-container-networking/blob/master/network/transparent_vlan_endpointclient_linux_test.go#L467

If so, do we need to update the unit test cases as well ?

Copy link
Member Author

@tamilmani1989 tamilmani1989 Mar 7, 2023

Choose a reason for hiding this comment

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

correct updated it

Copy link
Contributor

@vipul-21 vipul-21 Mar 7, 2023

Choose a reason for hiding this comment

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

But this does limit the user's ability to go beyond 23k( approximately) even if the vlan is not at max i.e 4k
I understand that this is safe as we don't have the requirement or a user that has that many namespace, but why are we doing it? Isn't it is safer to delete and make sure it is created when needed, instead of letting it be there.

OR Does this help in reducing the latency since we don't need to create a namespace again ?
(Do correct me if I am understanding it wrong)

Copy link
Member Author

@tamilmani1989 tamilmani1989 Mar 8, 2023

Choose a reason for hiding this comment

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

so this will be a problem when we parallelize the execution of cni in future.. one doing add and other doing delete in same namespace based on number of default routes

// Deletes default arp, default routes, vlan veth; there are two default routes
// so when we have <= numDefaultRoutes routes left, no containers use this namespace
Copy link
Contributor

@vipul-21 vipul-21 Mar 7, 2023

Choose a reason for hiding this comment

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

We are skipping deletion of vnet namespace if there are no containers but when there are new containers for that namespace, we won't need to create that namespace again ? Is that correct ?

Wanted to know if there is a scenario in which the namespaces are getting accumulated since they were not deleted.

Copy link
Member Author

@tamilmani1989 tamilmani1989 Mar 7, 2023

Choose a reason for hiding this comment

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

good question. cni creates netns per vlanid and at max vlanid can range from 1-4096 and also we dont support more than 150 containers in multitenancy. so not deleting netns should not cause any issue

vmss000000:/# cat /proc/sys/user/max_net_namespaces
27653

log.Printf("[transparent vlan] Deleting namespace %s as no containers occupy it", client.vnetNSName)
delErr := client.netnsClient.DeleteNamed(client.vnetNSName)
if delErr != nil {
return errors.Wrap(delErr, "failed to delete namespace")
// TODO: revist if this require in future.
//nolint gocritic
/* if routesLeft <= numDefaultRoutes {
// Deletes default arp, default routes, vlan veth; there are two default routes
// so when we have <= numDefaultRoutes routes left, no containers use this namespace
log.Printf("[transparent vlan] Deleting namespace %s as no containers occupy it", client.vnetNSName)
delErr := client.netnsClient.DeleteNamed(client.vnetNSName)
if delErr != nil {
return errors.Wrap(delErr, "failed to delete namespace")
}
}
}
*/
return nil
}

Expand Down
51 changes: 26 additions & 25 deletions network/transparent_vlan_endpointclient_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,33 +432,34 @@ func TestTransparentVlanDeleteEndpoints(t *testing.T) {
},
wantErr: false,
},
{
name: "Delete endpoint fail to delete namespace",
client: &TransparentVlanEndpointClient{
primaryHostIfName: "eth0",
vlanIfName: "eth0.1",
vnetVethName: "A1veth0",
containerVethName: "B1veth0",
vnetNSName: "az_ns_1",
netnsClient: &mockNetns{
deleteNamed: func(name string) (err error) {
return newNetnsErrorMock("netns failure")
//nolint gocritic
/* {
name: "Delete endpoint fail to delete namespace",
client: &TransparentVlanEndpointClient{
primaryHostIfName: "eth0",
vlanIfName: "eth0.1",
vnetVethName: "A1veth0",
containerVethName: "B1veth0",
vnetNSName: "az_ns_1",
netnsClient: &mockNetns{
deleteNamed: func(name string) (err error) {
return newNetnsErrorMock("netns failure")
},
},
netlink: netlink.NewMockNetlink(false, ""),
plClient: platform.NewMockExecClient(false),
netUtilsClient: networkutils.NewNetworkUtils(nl, plc),
netioshim: netio.NewMockNetIO(false, 0),
},
netlink: netlink.NewMockNetlink(false, ""),
plClient: platform.NewMockExecClient(false),
netUtilsClient: networkutils.NewNetworkUtils(nl, plc),
netioshim: netio.NewMockNetIO(false, 0),
},
ep: &endpoint{
IPAddresses: IPAddresses,
},
routesLeft: func() (int, error) {
return numDefaultRoutes, nil
},
wantErr: true,
wantErrMsg: "failed to delete namespace: netns failure: " + errNetnsMock.Error(),
},
ep: &endpoint{
IPAddresses: IPAddresses,
},
routesLeft: func() (int, error) {
return numDefaultRoutes, nil
},
wantErr: true,
wantErrMsg: "failed to delete namespace: netns failure: " + errNetnsMock.Error(),
},*/
}

for _, tt := range tests {
Expand Down