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

add L1VH IB support on CNI #2762

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
Draft

add L1VH IB support on CNI #2762

wants to merge 17 commits into from

Conversation

paulyufan2
Copy link
Contributor

@paulyufan2 paulyufan2 commented Jun 3, 2024 •
edited

Reason for Change:

This PR is to add L1VH IB support on CNI

The logic is:
CNI receives multiple NIC info from CNS
CNI will get the info and disable/dismount VF device
CNI creates the network and stdout cni result for containerd to create multiple pods with IB NICs or multiple different NICs

Issue Fixed:

Requirements:

Notes:

@paulyufan2 paulyufan2 added the cni Related to CNI. label Jun 3, 2024
QxBytes
QxBytes reviewed Jun 6, 2024
View reviewed changes
Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

some preliminary considerations (feel free to resolve if I've misunderstood etc.-- these are just what immediately came to mind)

cni/network/network.go Outdated
@@ -422,37 +422,38 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {

// Add Interfaces to result.
// previously just logged the default (infra) interface so this is equivalent behavior
cniResult := &cniTypesCurr.Result{}
cniResults := []cniTypesCurr.Result{}
Copy link
Contributor

Choose a reason for hiding this comment

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

does this change anything if we get multiple interface infos in existing cases-- previously we only had 1 cni result no matter how many interface infos we had (i.e. you've considered: singularity windows delegated nic case, dual nic case, and swift linux v2 case which all can have multiple interface infos, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes if CNS gives us multiple interfaceInfos, containerd will get multiple stdout from us to create pods

Copy link
Contributor

Choose a reason for hiding this comment

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

But currently I don't think that is the case in the above scenarios (the non-backend nic scenarios). We only return the infra in the current code (or the last interface info if no infra is found), but we claim to support multiple interface infos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make sure there is no regression here after I verify my new changes on Linux swiftv2

network/manager.go
@@ -807,6 +810,25 @@ func (nm *networkManager) GetEndpointInfosFromContainerID(containerID string) []
return ret
}

// Get PnP Device ID
func (nm *networkManager) GetPnPDeviceID(instanceID string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to confirm that the instanceID isn't controlled by customer or anything right? like it's not possible to execute arbitrary powershell commands with this

Copy link
Contributor

Choose a reason for hiding this comment

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

is this and the other powershell command functions unit-testable?

Copy link
Contributor Author

@paulyufan2 paulyufan2 Jun 7, 2024 •
edited

Choose a reason for hiding this comment

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

instanceID is from CNS as PnPID/PciID and we should have UTs to cover this

network/manager.go
Copy link
Contributor

Choose a reason for hiding this comment

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

does backend nic require deletion of the network like delegated nic does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we need

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is the case could you update the network deletion code to also include the backend nic case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we discussed that we should return nil when we createEndpoint and deleteEndpoint to avoid creating endpoint with IB NIC because HNS will not program the endpoint if it's IB NIC;
And we should delete network with backend NIC same as delegatedNIC

network/endpoint.go Outdated
@@ -109,8 +109,8 @@ type EndpointInfo struct {
Options map[string]interface{}
DisableHairpinOnHostInterface bool
IsIPv6Enabled bool

HostSubnetPrefix string // can be used later to add an external interface
PnPID string
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a brief comment about the pnp here or in interface info-- the pnpid isn't needed later during deletion right? and does backend nic only exist in windows stateless cni?

cni/network/network.go Outdated
@@ -422,37 +422,38 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {

// Add Interfaces to result.
// previously just logged the default (infra) interface so this is equivalent behavior
cniResult := &cniTypesCurr.Result{}
cniResults := []cniTypesCurr.Result{}
for key := range ipamAddResult.interfaceInfo {
logger.Info("Exiting add, interface info retrieved", zap.Any("ifInfo", ipamAddResult.interfaceInfo[key]))
// previously we had a default interface info to select which interface info was the one to be returned from cni add
Copy link
Contributor

Choose a reason for hiding this comment

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

if the cniresult logic is being modified can you update this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, before we only stdout one cniResult and containerd creates one pod, now we should provide multiple cniResults for containerd to create multiple pods

cni/network/invoker_cns.go Outdated
@@ -166,7 +167,7 @@ func (invoker *CNSIPAMInvoker) Add(addConfig IPAMAddConfig) (IPAMAddResult, erro
// Do we want to leverage this lint skip in other places of our code?
key := invoker.getInterfaceInfoKey(info.nicType, info.macAddress)
switch info.nicType {
case cns.DelegatedVMNIC:
case cns.DelegatedVMNIC, cns.BackendNIC:
// only handling single v4 PodIPInfo for DelegatedVMNICs at the moment, will have to update once v6 gets added
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment still accurate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QxBytes QxBytes QxBytes left review comments

@tamilmani1989 tamilmani1989 Awaiting requested review from tamilmani1989

@behzad-mir behzad-mir Awaiting requested review from behzad-mir

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cni Related to CNI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@paulyufan2 @QxBytes