feat: sync list gpu cards with new sdk usage#593
Conversation
Signed-off-by: steven-chiu-bigstack <steven.chiu@bigstack.co>
steven-chiu-bigstack
requested review from
raven-pan
and removed request for
traviswu-bigstack
Signed-off-by: steven-chiu-bigstack <steven.chiu@bigstack.co>
| continue | ||
| } | ||
|
|
||
| profileId := profileIdMap[*instance.ProfileAlias] |
There was a problem hiding this comment.
Maybe we should use ok to check is profile exist:
profileId, ok := profileIdMap[*instance.ProfileAlias]
if ok {
remainingMap[profileId] = max(remainingMap[profileId]-1, 0)
}There was a problem hiding this comment.
Yeah, you're right. Otherwise profileId will default to 0 thanks to GO's built-in behavior.
| } | ||
|
|
||
| if !IsHexSuccessful(err) { | ||
| log.Errorf("nodes: output error when listing vgpu profiles for gpu %s via hex_sdk: %v", gpuId, err) |
There was a problem hiding this comment.
The err is already guaranteed nil in the previous if err != nil {.
So following code will never be executed.
log.Errorf("nodes: output error when listing vgpu profiles for gpu %s via hex_sdk: %v", gpuId, err)
return collectionThere was a problem hiding this comment.
You're definitely right!
I copied this pattern from other files; not sure why there's a bunch of redundant check in the codebase. 🤔
I'll proceed to remove this if !IsHexSuccessful(err) { ... } block because it's unnecessary.
Code_Max8AJPqwY.mp4
| AliasName string `json:"aliasName"` | ||
| Count int `json:"count"` | ||
| Remaining int `json:"remaining"` | ||
| Id uint32 `json:"id"` |
There was a problem hiding this comment.
These field change from string to number.
Maybe we should update the openapi doc also?
There was a problem hiding this comment.
Yes, and if I'm not mistaken, it should've been updated already (for list GPU cards API, see docs):
As for the openapi docs for updating GPU API, perhaps we can handle that during your API implementation?
| for _, instance := range attachedInstances { | ||
| profile := profileMapByAlias[*instance.ProfileAlias] | ||
| profileInstanceCountMap[profile.Id]++ | ||
| for _, profile := range *hexProfileCollection.Sriov { |
There was a problem hiding this comment.
Maybe we should add a nil guard before for _, profile := range *hexProfileCollection.Sriov {?
There was a problem hiding this comment.
Sure, good catch!
| profile.Remaining = profile.Count - instanceCount | ||
| migProfileRemainingMap := createMigProfileRemainingMap(hexProfileCollection.MigBacked, attachedInstances) | ||
|
|
||
| for _, profile := range *hexProfileCollection.MigBacked { |
There was a problem hiding this comment.
Maybe we should add a nil guard before or _, profile := range *hexProfileCollection.MigBacked {?
There was a problem hiding this comment.
Sure, good catch!
| } | ||
|
|
||
| vgpuProfiles, hexProfilesMap := listVgpuProfiles(device, hexGpu) | ||
| hexProfilesMap, hexProfileCollection := cubecos.GetNodeVgpuProfilesMap(hexGpu.PciAddress) |
There was a problem hiding this comment.
Discussion:
We can call isVgpu function before cubecos.GetNodeVgpuProfilesMap(hexGpu.PciAddress).
If type is unset or pgpu, we can just skip the cubecos.GetNodeVgpuProfilesMap(hexGpu.PciAddress) call.
There was a problem hiding this comment.
No problem. 👌
| if vgpuProfiles != nil && attachedInstances != nil { | ||
| updateVgpuProfilesRemaining(*vgpuProfiles, *attachedInstances) | ||
| } | ||
| profileCollection := toProfileCollection(hexProfileCollection, attachedInstances) |
There was a problem hiding this comment.
Discussion:
We can call isVgpu function before toProfileCollection.
If type is unset or pgpu, we can just skip the toProfileCollection call and assign an empty array to profileCollection.
There was a problem hiding this comment.
I prefer keeping the current pattern since it's pure data mapping (raw data to view model), which should happen regardless of the GPU type.
Signed-off-by: steven-chiu-bigstack <steven.chiu@bigstack.co>
2 participants
What type of PR is this?
Feat
Which issue(s) this PR fixes?
Related to bigstack-oss/cubecos#859
What this PR does?
Sync list GPU cards API implementation with the new SDK usage.
Test results (optional)
1). make sure the api docs have been updated
2). make sure the api works properly