-
Notifications
You must be signed in to change notification settings - Fork 350
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
Topology support #141
Topology support #141
Conversation
pkg/controller/controller.go
Outdated
return rsp.GetCapabilities(), nil | ||
} | ||
|
||
func supportsPluginControllerService(capabilities []*csi.PluginCapability) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have one helper method that searches capabilities for the passed in service
pkg/controller/controller.go
Outdated
if err != nil { | ||
glog.Errorf("failed to get support info :%v", err) | ||
return "", err | ||
glog.Errorf("failed to get plugin capabilities: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled as a separate pr, but I want to get rid of this style of logging errors that we're also returning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address it in this PR. Improving the code base one line at a time :)
pkg/controller/controller.go
Outdated
@@ -90,6 +90,11 @@ type csiProvisioner struct { | |||
config *rest.Config | |||
} | |||
|
|||
type driverState struct { | |||
driverName string | |||
accessibilityConstraintCapability bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a more clear name could be "supportsAccessiblity"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you could potentially make this generic and have an enum of all the different capabilities
87991fd
to
8a083d0
Compare
All the core functionality and the vast majority of core testing is complete. Reviewers PTAL. I'll add StatefulSet spreading in a separate PR post k8s 1.12 release since it's an optimization. Working on the TODOs left in the PR but I'll hold off on pushing mode code for now until we get a critical mass of reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too large to review. Skimmed it. Largely LGTM.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saad-ali, verult The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Added backoff parameters to external attacher
Creating a PR for early review.
I have a local working prototype. The hope is each of these PRs introduce a feature whose implementation is as close to the final form as possible.
@jsafrane @msau42 @saad-ali @ddebroy
Features (some of these need more refinement but core logic is there):