merge inject and rename chain elements #306
Conversation
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
pperiyasamy
force-pushed
the
merge-inject-rename
branch
from
5fd6eed to
42910ff
Compare
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
| if vfConfig != nil && vfConfig.VFInterfaceName != ifName { | ||
| err = moveInterfaceToAnotherNamespace(vfConfig.VFInterfaceName, curNetNS, curNetNS, targetNetNS) | ||
| if err == nil { | ||
| err = renameInterface(vfConfig.VFInterfaceName, ifName, curNetNS, targetNetNS) |
There was a problem hiding this comment.
Looks like we need to move interface back in such case.
There was a problem hiding this comment.
I am afraid that this operation is not idempotent, because after the rename we still have different vfconfig.VFInterfaceName and ifName, but interface is no more accessible by vfconfig.VFInterfaceName because it was renamed.
There was a problem hiding this comment.
yes, that's why there is a check at line 100-103 to see ifName is already present in contNetNS and if interface is already present, it's returned immediately without an error.
| return nil | ||
| } | ||
|
|
||
| func moveToContNetNS(vfConfig *vfconfig.VFConfig, ifName string, curNetNS, targetNetNS netns.NsHandle) (err error) { |
There was a problem hiding this comment.
Please rename here curNetNS to hostNetNS and targetNetNS to conNetNS if you make such renaming.
|
|
||
| func moveToHostNetNS(vfConfig *vfconfig.VFConfig, ifName string, curNetNS, targetNetNS netns.NsHandle) (err error) { | ||
| if vfConfig != nil && vfConfig.VFInterfaceName != ifName { | ||
| link, _ := kernellink.FindHostDevice("", vfConfig.VFInterfaceName, curNetNS) |
There was a problem hiding this comment.
If interface was renamed, we need to find it by ifName and so if we are moving it back, we probably need to find it in targetNetNS.
There was a problem hiding this comment.
This method gets executed only for sriov device move back scenario and renaming would happen only at line 123.
This check is done to make things to be idempotent and also to find out if sriov device is accidentally moved out to hostNetNS without renaming the interface (in case of container kill), its not appropriate to rely on ifName here as multiple NSCs use same interface name. so we must also search using kernellink.FindHostDevice using VFs pci address to restore interface original name. will revisit here once this PR is merged.
| err = move(logger, request.GetConnection(), true) | ||
| if err != nil { | ||
| logger.Warnf("server request failed, failed to move back the interface: %v", err) | ||
| moveRenameErr := move(ctx, request.GetConnection(), true) |
There was a problem hiding this comment.
Please use here isEstablished check as we discussed offline.
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
…k-kernel@main PR link: networkservicemesh/sdk-kernel#306 Commit: 5bdd8f7 Author: Ed Warnicke Date: 2021-08-17 09:17:32 -0500 Message: - Merge pull request #306 from Nordix/merge-inject-rename Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Fixes #102
Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech