Fix GPU attach/detach handling and CM response processing#44
Open
NekoHK wants to merge 11 commits into
Open
Conversation
NekoHK
requested review from
hase1128,
mgazz,
shogom2 and
tmkymst
as code owners
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
Signed-off-by: Ko Kai <ko.kai@jp.fujitsu.com>
NekoHK
force-pushed
the
github-sync-ado-dev
branch
from
ba02cdc to
2ec315c
Compare
shogom2
approved these changes
Jun 15, 2026
hase1128
approved these changes
Jun 15, 2026
mgazz
reviewed
Jun 15, 2026
mgazz
left a comment
mgazz
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for the submission. The PR looks good, just a couple of suggestions to improve readibility.
| var composableResourceLog = ctrl.Log.WithName("composable_resource_controller") | ||
|
|
||
| func nvidiaGpuOperatorNamespace() string { | ||
| if ns := os.Getenv("NVIDIA_GPU_OPERATOR_NAMESPACE"); ns != "" { |
Collaborator
There was a problem hiding this comment.
I would update the documentation explaining that, if the gpu operator was installed in a custom namespace, this variable needs to be set. The default value is gpu-operator
Collaborator
There was a problem hiding this comment.
I would also log the default value when the variable is not set. Something like
log.Info("GPU_OPERATOR_NAMESPACE not set, using default", "namespace", ns)| if device.Detail.ResourceOPStatus[:1] == "0" { | ||
| resourceOPStatus := string(device.Detail.ResourceOPStatus) | ||
| if len(resourceOPStatus) == 0 { | ||
| return fmt.Errorf("the target gpu '%s' has empty status in CM", instance.Status.DeviceID) |
Collaborator
There was a problem hiding this comment.
I would expand this message with a reference to the machine (machineID) making it easier to inspect the resources involved in the attachment
|
|
||
| err = fmt.Errorf("failed to process CM scaleup request. http returned status: '%d', cm return code: '%s', error message: '%s'", errBody.Status, errBody.Detail.Code, errBody.Detail.Message) | ||
| clientLog.Error(err, "failed to process CM scaleup request", "ComposableResource", instance.Name) | ||
| if response.StatusCode < 200 || response.StatusCode >= 300 { |
Collaborator
There was a problem hiding this comment.
For readability I would isolate this check in a separated function.
func isHTTPSuccess(statusCode int) bool {
return statusCode >= 200 && statusCode < 300
}The function can be used as following:
if !isHTTPSuccess(response.StatusCode) {
....
}
Comment on lines
+293
to
299
| if resourceOPStatus[:1] == "0" { | ||
| // The target device exists and has no error, return OK. | ||
| return nil | ||
| } else if device.Detail.ResourceOPStatus[:1] == "1" { | ||
| } else if resourceOPStatus[:1] == "1" { | ||
| return fmt.Errorf("the target gpu '%s' is showing a Warning status in CM", instance.Status.DeviceID) | ||
| } else if device.Detail.ResourceOPStatus[:1] == "2" { | ||
| } else if resourceOPStatus[:1] == "2" { | ||
| return fmt.Errorf("the target gpu '%s' is showing a Critical status in CM", instance.Status.DeviceID) |
Collaborator
There was a problem hiding this comment.
I would use constants to explain the various status.
const (
ResourceStatusOK = "0"
ResourceStatusWarning = "1"
ResourceStatusCritical = "2"
)if resourceOPStatus[:1] == ResourceStatusOK {
return nil
} else if resourceOPStatus[:1] == ResourceStatusWarning { ....
NekoHK
force-pushed
the
github-sync-ado-dev
branch
2 times, most recently
from
e6fd6a8 to
2ec315c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
GPU attach/detach processing needed improvements around driver detection, GPU load checks, kubelet plugin restart behavior, and empty device responses.
CM response handling also needed to correctly process machine lookup results and removal responses.
Changes