Check volume binding mode #479

dturn · 2019-05-01T00:30:45Z

If the StorageClass for a PVC has 'volumeBindingMode: WaitForFirstConsumer',
then it won't bind until after a Pod mounts it. But the PVC must be pre-deployed,
as the Pod requires it. So 'Pending' must be treated as a 'Success' state in this
scenario.

What are you trying to accomplish with this PR?
Automatic handling of the 'volumeBindingMode: WaitForFirstConsumer' condition in the StorageClass of a PVC resource.

How is this accomplished?
By determining the StorageClass of the PVC resource and finding the volumeBindingMode value. If it is WaitForFirstConsumer, then a PVC status of 'Pending' is treated as a success during deployment.

What could go wrong?
The code currently assumes the DefaultStorageClass admission plugin is turned on, and looks for a default StorageClass if one is not present in the PVC definition. There doesn't appear to be a way to determine if this admission plugin is turned on programmatically.

The code also checks for two other conditions and provides a warning:

if there are multiple Default StorageClasses defined; or
if the PVCs defined StorageClass is not found

@redmcg . When I pushed it went to origin instead of your repo. This still isn't done, needs better error messages, a test or two to catch some of the failure cases, and the test needs to clean up the storage class since its not namespaced. But thought I'd get your eyes early

If the StorageClass for a PVC has 'volumeBindingMode: WaitForFirstConsumer', then it won't bind until after a Pod mounts it. But the PVC must be pre-deployed, as the Pod requires it. So 'Pending' must be treated as a 'Success' state in this scenario.

redmcg · 2019-05-01T01:21:13Z

This looks much much better. My only concern was with using hostPath instead of local in the PV - as when reading this:
https://kubernetes.io/docs/concepts/storage/storage-classes/#volume-binding-mode

it sounded like volumeBindingMode: WaitForFirstConsumer wasn't supported by hostPath. But I did a couple of tests and it seems to be supported. So we can get rid of that initContainer too.

How do you want to proceed with the PR? Am I able to contribute to this branch? And should I delete my own PR?

dturn · 2019-05-01T15:14:22Z

How do you want to proceed with the PR? Am I able to contribute to this branch? And should I delete my own PR?

If its ok with you, using this branch is easier for us (makes running CI simpler). You should be able to push to this PR, just pull my changes into your branch and we'll make it work. It would be great if you could make the test changes you suggested!

redmcg · 2019-05-01T21:48:58Z

I couldn't push directly to this branch - I get:
Permission to Shopify/kubernetes-deploy.git denied to redmcg.

But I can create a new pull request against it. That does mean you'll have a PR against a PR though. I'll try that - but will of course close it if you prefer a different approach.

Edit: I've opened #480. With this I guess you can review each commit before accepting it in to this PR. Alternatively I noticed that you should be able to contribute to any branch with which I create a PR (i.e. by pushing to redmcg/kubernetes-deploy.git) - but that doesn't address making CI simpler.

* Remove redundant initContainer and volume * Delete storageclass on test completion * Assert all four resources finished in the correct state * Add additional tests * Fix line length to satisfy Policial

lib/kubernetes-deploy/kubernetes_resource/persistent_volume_claim.rb

timothysmith0609

Looking good to me, few minor style comments

lib/kubernetes-deploy/kubernetes_resource/persistent_volume_claim.rb

test/integration/kubernetes_deploy_test.rb

test/fixtures/pvc/storage_class.yml

timothysmith0609

One last question about failure_message but everything else is looking good for me.

lib/kubernetes-deploy/kubernetes_resource/persistent_volume_claim.rb

timothysmith0609 · 2019-05-14T16:24:53Z

lib/kubernetes-deploy/kubernetes_resource/persistent_volume_claim.rb

+      if storage_class_name.nil? && @storage_classes.count(&:default?) > 1
+        "PVC has no StorageClass specified and there are multiple StorageClasses " \
+        "annotated as default. This is an invalid cluster configuration."
+      end


Is there an else case to consider? If it just returns nil will that hide worthwhile information from the user?

The else case is status == "Lost" which didn't have any message before. I'm also not sure what we would helpfully say in the lost case.

RyanBrushett

🚢

redmcg · 2019-05-15T18:51:39Z

Thanks all for your work on this

KnVerey · 2019-05-27T19:20:41Z

lib/kubernetes-deploy/kubernetes_resource/persistent_volume_claim.rb

+        @storage_classes.detect { |sc| sc.name == storage_class_name }
+      # storage_class_name = "" is an explicit request for no storage class
+      # storage_class_name = nil is an impplicit request for default storage class
+      elsif storage_class_name != ""


Isn't this more specifically storage_class_name.nil??

KnVerey · 2019-05-27T19:35:10Z

test/integration/kubernetes_deploy_test.rb

+
+    assert_logs_match_all([
+      "Failed to deploy 1 priority resource",
+      "PersistentVolumeClaim/with-storage-class: TIMED OUT (timeout: 20s)",


Is this test taking a full 20s? Can we use a custom timeout to cut that down? Same question for one of the tests above.

Good eye, it does :(. Should be able to decrease it, just haven't tried yet.

This is already using a custom timeout via a kubernetes-deploy.shopify.io/timeout-override:
https://github.com/Shopify/kubernetes-deploy/blob/ef17097b42b5875e15c138b74f7a9eb80485af9c/test/fixtures/pvc/pvc.yml#L5

The default was 300s - so I shortened it to 20s. I chose 20s to be sure it wasn't so short as to create a false positive - but 20s is still pretty generous.

redmcg and others added 11 commits April 20, 2019 12:01

Fix formatting to satisfy Policial

56e380e

Assign @storage_class using the ||= operator

8e86647

Immediately return with the default storage class

05f28f9

Add pvc tests for volumeBindingMode: WaitForFirstConsumer

d711bab

Remove redundant 'return' keyword (to satisfy policial)

cf90b45

Allow volumeBindingMode test to run on any cluster

4208c0b

Fix formatting for policial

b9c4b54

Merge branch 'master' into check_volumeBindingMode

f2f8f71

Support both beta and GA version of 'is-default-class'

4d88029

Refactor pass

e320da9

redmcg mentioned this pull request May 1, 2019

Check volumeBindingMode in StorageClass of PVC #469

Closed

redmcg and others added 2 commits May 3, 2019 09:59

Check volume binding mode (additional tests) (#480)

1237ef1

* Remove redundant initContainer and volume * Delete storageclass on test completion * Assert all four resources finished in the correct state * Add additional tests * Fix line length to satisfy Policial

Tweak error messages

923ddfe

dturn requested review from timothysmith0609 and KnVerey and removed request for timothysmith0609 May 3, 2019 17:30

dturn self-assigned this May 3, 2019

RyanBrushett reviewed May 9, 2019

View reviewed changes

timothysmith0609 reviewed May 10, 2019

View reviewed changes

dturn force-pushed the check_volumeBindingMode branch from a127f3b to 54ebdc8 Compare May 13, 2019 16:21

PR feedback

9f4bb59

dturn force-pushed the check_volumeBindingMode branch from 54ebdc8 to 9f4bb59 Compare May 13, 2019 16:52

dturn requested review from RyanBrushett and timothysmith0609 and removed request for RyanBrushett May 13, 2019 17:01

dturn requested a review from RyanBrushett May 13, 2019 17:01

timothysmith0609 reviewed May 14, 2019

View reviewed changes

Add comment explaining storge_class_name == ''

353e3fd

timothysmith0609 approved these changes May 14, 2019

View reviewed changes

RyanBrushett approved these changes May 15, 2019

View reviewed changes

dturn merged commit 0f55c66 into master May 15, 2019

dturn deleted the check_volumeBindingMode branch May 15, 2019 15:22

KnVerey reviewed May 27, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check volume binding mode #479

Check volume binding mode #479

dturn commented May 1, 2019 •

edited

Loading

redmcg commented May 1, 2019

dturn commented May 1, 2019

redmcg commented May 1, 2019 •

edited

Loading

timothysmith0609 left a comment

timothysmith0609 left a comment

timothysmith0609 May 14, 2019

dturn May 14, 2019

RyanBrushett left a comment

redmcg commented May 15, 2019

KnVerey May 27, 2019

KnVerey May 27, 2019 •

edited

Loading

dturn May 27, 2019

redmcg May 28, 2019

Check volume binding mode #479

Check volume binding mode #479

Conversation

dturn commented May 1, 2019 • edited Loading

redmcg commented May 1, 2019

dturn commented May 1, 2019

redmcg commented May 1, 2019 • edited Loading

timothysmith0609 left a comment

Choose a reason for hiding this comment

timothysmith0609 left a comment

Choose a reason for hiding this comment

timothysmith0609 May 14, 2019

Choose a reason for hiding this comment

dturn May 14, 2019

Choose a reason for hiding this comment

RyanBrushett left a comment

Choose a reason for hiding this comment

redmcg commented May 15, 2019

KnVerey May 27, 2019

Choose a reason for hiding this comment

KnVerey May 27, 2019 • edited Loading

Choose a reason for hiding this comment

dturn May 27, 2019

Choose a reason for hiding this comment

redmcg May 28, 2019

Choose a reason for hiding this comment

dturn commented May 1, 2019 •

edited

Loading

redmcg commented May 1, 2019 •

edited

Loading

KnVerey May 27, 2019 •

edited

Loading