-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Dynamic provisioning for flocker volume plugin #31005
Dynamic provisioning for flocker volume plugin #31005
Conversation
2b5a589
to
ae5699c
Compare
Automatic merge from submit-queue Adds myself to the flocker volume plugin owners I am happy to look after the flocker volume plugin and support @agonzalezro. Currently refactoring the volume plugin and adding dynamic provisioning features in kubernetes#31005
|
||
var _ volume.Provisioner = &flockerVolumeProvisioner{} | ||
|
||
func (c *flockerVolumeProvisioner) Provision() (*api.PersistentVolume, error) { |
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.
If I am reading it right, you do not support any configuration in StorageClass.Parameters and in PVC.Spec.Selector. In that case, you should check that c.options.parameters
and c.options.selector
are empty and throw an error if not.
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.
Ok I have added the tests and checks accordingly, with the latest changes
I reviewed the provisioner and it looks pretty solid at this point. |
ae5699c
to
e32c910
Compare
e32c910
to
fbdbb1b
Compare
@jsafrane Thanks for the review. I've just modified it based on your comments. I think there's no point to hurry, as it won't be merged into 1.4 anyhow? |
GCE e2e build/test passed for commit fbdbb1b. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. |
No, this is for 1.5. I went through current versions, just a few documentation nits found. Please rebase and finish the deleter! Reviewed 3 of 22 files at r1, 3 of 20 files at r2, 19 of 19 files at r3, 9 of 9 files at r4. pkg/api/types.go, line 746 [r4] (raw file):
IMO, this line should be in FlockerVolumeSource docs, i.e. two lines up ("only one of DatasetName and DatasetUUID should be set"). Otherwise it's godoc of DatasetName, which is quite confusing. pkg/api/v1/types.go, line 651 [r4] (raw file):
dtto, this is godoc of DatasetName, which does not make sense. Comments from Reviewable |
fbdbb1b
to
276a21d
Compare
Review status: 0 of 38 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. pkg/api/types.go, line 746 [r4] (raw file):
|
276a21d
to
bd245f7
Compare
47acefe
to
77bf500
Compare
Jenkins GCI GCE e2e failed for commit 77bf500. Full PR test history. The magic incantation to run this job again is |
@jsafrane if you have the chance, could you do another review please? From my point of view it's ready to merge., Do you think an exampe is necessary (like the ones in https://github.com/kubernetes/kubernetes/tree/master/examples/experimental/persistent-volume-provisioning)? |
Reviewed 1 of 24 files at r1, 1 of 20 files at r2, 6 of 42 files at r5, 14 of 34 files at r8, 13 of 13 files at r9, 9 of 9 files at r10. Comments from Reviewable |
almost LGTM, only all log messages should start with lowercase. Simple grep for glog will reveal them all. I should have noticed it earlier. |
77bf500
to
0f92148
Compare
* flocker datasets should be attached using an unique identifier. This is not the case for the name metadata used by datasetName * allow only one of datasetUUID / datasetName specified
Supports additional options for CreateDataset and DeleteDataset for dynamic provisioning. Bugfix for timeouts during CreateDataset
* Support provisioning * Support deletion * Use bind mounts instead of /flocker in containers * support ownership management or SELinux relabeling.
0f92148
to
cd08978
Compare
@jsafrane Ok I have downcased all the log messages |
Reviewed 1 of 24 files at r1, 1 of 20 files at r2, 6 of 42 files at r5, 5 of 13 files at r9, 1 of 23 files at r11, 13 of 13 files at r12, 9 of 9 files at r13. Comments from Reviewable |
LGTM. Thanks a lot for the PR! |
@jsafrane I think the only thing missing now is the reviewable "discussions" Thanks for reviewing! |
reviewable discussions are optional, important is that "Submit Queue" is green - it's in the queue and it will be merged eventually. |
@jsafrane 👍 thanks for clarifying, not too familiar with reviewable yet |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Jenkins GKE smoke e2e failed for commit cd08978. Full PR test history. The magic incantation to run this job again is |
Automatic merge from submit-queue |
I get an error from kubelet when mounting flocker volumes using hyperkube:1.5.1 Kube Setup: Flocker Setup: 1.15 Error log when deploying mysql: https://github.com/kubernetes/kubernetes/tree/master/examples/mysql-wordpress-pd. (I changed the volume YAML file such that a localhost volume is changed into a flocker dataset):
|
Refactor flocker volume plugin
I based my refactor work to replicate pretty much GCE-PD behaviour
Related issues: #29006 #26908
@jsafrane @mattbates @wallrj @wallnerryan
This change is