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

Fix device share plugins npe #3351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

coldzerofear
Copy link

There is an NPE issue with the device share plugin, #3241 Not really solving the problem

unit testing

func TestDevices(t *testing.T) {
	others := make(map[string]interface{})
	nodeDevices := vgpu.DecodeNodeDevices("k8s01", "GPU-c496852d-f5df-316c-e2d5-86f0b322ec4c,20,30720,100,NVIDIA-NVIDIA GeForce RTX 3080 Ti,0,false:")
	others["gpu1"] = nodeDevices
	var dev2 *vgpu.GPUDevices = nil
	others["gpu2"] = dev2
	var dev3 Devices = nil
	others["gpu3"] = dev3
	var dev4 Devices = dev2
	others["gpu4"] = dev4
	for key, val := range others {
		if dev, ok := val.(Devices); ok {
			if dev == nil {
				fmt.Println(key, "is nil")
				continue
			}
			fmt.Println(key, "is not nil")
		} else {
			fmt.Println(key, "is not devices")
		}
	}
}

Output results

=== RUN   TestDevices
gpu1 is not nil
gpu2 is not nil
gpu3 is not devices
gpu4 is not nil
--- PASS: TestDevices (0.00s)

This does not meet expectations

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 15, 2024
@coldzerofear
Copy link
Author

/assign @k82cn

@volcano-sh-bot volcano-sh-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 15, 2024
@@ -18,11 +18,12 @@ package deviceshare

import (
"fmt"
"reflect"
"volcano.sh/volcano/pkg/scheduler/api/devices"
Copy link
Member

Choose a reason for hiding this comment

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

please sort the imports.

@@ -100,7 +100,7 @@ func NewGPUDevices(name string, node *v1.Node) *GPUDevices {
klog.Infof("node %v device %s leave", node.Name, handshake)

tmppat := make(map[string]string)
tmppat[handshake] = "Deleted_" + time.Now().Format("2006.01.02 15:04:05")
tmppat[VolcanoVGPUHandshake] = "Deleted_" + time.Now().Format("2006.01.02 15:04:05")
Copy link
Member

Choose a reason for hiding this comment

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

@archlitchi plz check this changes. I am not familiar with this vgpu feaure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, i didn't noticed that, thanks for fixing:)

@lowang-bh
Copy link
Member

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 31, 2024
@lowang-bh
Copy link
Member

please squash commits to one commit.

@coldzerofear coldzerofear deleted the fix-npe branch April 1, 2024 06:42
@coldzerofear coldzerofear restored the fix-npe branch April 1, 2024 06:42
2、Fix vgpu device handshake patch error
@Monokaix
Copy link
Member

Monokaix commented May 8, 2024

Hi, please sign off your commit with git commit -s

@volcano-sh-bot volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2024
@volcano-sh-bot
Copy link
Contributor

@coldzerofear: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants