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

add rdma device busId at scheduler #2250

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ferris-cx
Copy link

Ⅰ. Describe what this PR does

The RDMA device is mounted inside the container based on the scheduling assignment result.

Ⅱ. Does this pull request fix one issue?

The scheduling framework already supports joint allocation of Gpus and RDMA devices, but several semantics need to be tested in practice, including preference and samePCIE. In order to allow RDMA devices to access the container, we need to fix the lack of BDF addresses in the results assigned by the scheduling algorithm to RDMA devices

Ⅲ. Describe how to verify it

  1. In the k8s cluster, prepare one or more servers that support rdma network adapters as cluster nodes. Install the new version of the koordlet component, koord-manager, and the revamped multus-cni on each node, and check the node status. The number of resources is displayed as the actual number of RDMA nics on the node.

  2. Write a pod.YAML to apply for RDMA network card resources, kubectl apply-f pod. On pod note (device-allocated), check whether the rdma device bdf address (busID field) is included in the rdma allocation result, and check the accuracy.

  3. When the pod is in the running state, enter the container and run the ifconfig command to check the number of network cards. If the case is correct, multiple network card lists will be output, such as net1, net2, net3, and so on.

Ⅳ. Special notes for reviews

  1. Deploy components that support RDMA, including koordlet, koord-manager, koord-scheduler, and multus-cni.

  2. Due to the complete end-to-end passthrough, multi-CNI plug-in is also required, which complies with CNI specifications and supports multi-NIC allocation. Assigning the RDMA network card PF/VF to the Pod mentioned here requires the device ID to be injected into the component, otherwise it will not work. This change will be maintained separately in the multi-cni project or another PR

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

@@ -545,12 +546,43 @@ func (p *Plugin) preBindObject(ctx context.Context, cycleState *framework.CycleS
return nil
}

var deviceAllocs *apiext.DeviceAllocations
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 consider add move this logic to Reserve() phase?

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -89,6 +89,7 @@ type DeviceAllocation struct {
Minor int32 `json:"minor"`
Resources corev1.ResourceList `json:"resources"`
Extension *DeviceAllocationExtension `json:"extension,omitempty"`
BusID string `json:"busID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer define it as this:

type DeviceAllocation struct {
	Minor     int32                      `json:"minor"`
	Resources corev1.ResourceList        `json:"resources"`
	Extension *DeviceAllocationExtension `json:"extension,omitempty"`
	Topology  *DeviceTopology            `json:"topology,omitempty"`
}

type DeviceTopology struct {
	// BusID is the domain:bus:device.function formatted identifier of PCI/PCIE device
	BusID string `json:"busID,omitempty"`
}

Copy link
Author

Choose a reason for hiding this comment

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

OK!BusId is one of the attributes of the topology structure and can be encapsulated in the topology structure!

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 64.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.02%. Comparing base (8661de2) to head (f644394).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
pkg/scheduler/plugins/deviceshare/plugin.go 64.00% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2250      +/-   ##
==========================================
- Coverage   66.29%   66.02%   -0.28%     
==========================================
  Files         453      453              
  Lines       53311    53411     +100     
==========================================
- Hits        35344    35265      -79     
- Misses      15419    15604     +185     
+ Partials     2548     2542       -6     
Flag Coverage Δ
unittests 66.02% <64.00%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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