Plumbing topology information through Provision call in external PVController library. #902
Conversation
|
@verult: GitHub didn't allow me to request PR reviews from the following users: lichuqiang. Note that only kubernetes-incubator members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
k8s-ci-robot
added
size/M
|
Seems you need go-fmt. |
lib/controller/controller.go
Outdated
| strerr := fmt.Sprintf("Failed to get target node: %v", err) | ||
| glog.Infof("unexpected error getting target node %q for claim %q: %v", nodeName, claimToClaimKey(claim), err) | ||
| ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", strerr) | ||
| return err |
There was a problem hiding this comment.
can we use the same error message for the Event + return?
There was a problem hiding this comment.
The behavior in this block matches the behavior in the internal PVController
There was a problem hiding this comment.
agreed let's make error message same for event + return + glog, we do the same weird thing here https://github.com/verult/external-storage/blob/5dee038e0ee3133a8e40fbe39d915cef0c722af2/lib/controller/controller.go#L1102
lib/controller/controller.go
Outdated
| selectedNode, err = ctrl.client.CoreV1().Nodes().Get(nodeName, metav1.GetOptions{}) // TODO (verult) cache Nodes | ||
| if err != nil { | ||
| strerr := fmt.Sprintf("Failed to get target node: %v", err) | ||
| glog.Infof("unexpected error getting target node %q for claim %q: %v", nodeName, claimToClaimKey(claim), err) |
There was a problem hiding this comment.
Do we need to log the error if it's being returned?
There was a problem hiding this comment.
Same as the other comment
There was a problem hiding this comment.
@wongma7 what do you think of potentially cleaning this up (as a separate PR)? I think the style of logging an error that you also return is unusual
There was a problem hiding this comment.
yes I will clean it up on next week, the caller of provisionClaimOperation should log the error.
upstream is weirder because the error is bubbled up through goroutinemap and such so I imagine there is a logical explanation for the weirdness but at least here we should make it simpler if we can
k8s-ci-robot
added
size/L
|
Cleaned up errors/logs/events as mentioned above, and added tests |
lib/controller/controller.go
Outdated
| // Get AllowedTopologies | ||
| allowedTopologies, err = ctrl.fetchAllowedTopologies(claimClass) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
should an event be logged here too?
lib/controller/controller.go
Outdated
| return class.AllowedTopologies, nil | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("Cannot convert object to StorageClass: %+v", classObj) |
There was a problem hiding this comment.
nit: we've been trying to follow the convention of error messages starting with lowercase
lib/controller/controller_test.go
Outdated
| expectedParams *provisionParams | ||
| }{ | ||
| { | ||
| name: "provision with AllowedTopologies", |
There was a problem hiding this comment.
also a test case to provision with neither
|
@msau42 addressed comments |
|
/lgtm |
This is a dependency for adding topology functionality to CSI.
In the future, we'd like to cache Node objects since almost all plugins will go through CSI. For now I hope to get the provisioner interface change in first to unblock CSI work, and open an issue to track the work once this PR is merged.
/cc @wongma7 @msau42 @lichuqiang