Consider PVC with a WaitForFirstConsumer SC ready #11335
Conversation
As discussed in helm#10733. Charts which have PVC associated with resources not starting immediately after install (like a CronJob) will wait forever if the associated StorageClass has a volume binding mode of `WaitForFirstConsumer`. This PR attempts to solve this by querying the associated StorageClass (or the default one) and check its `volumeBindingMode` field. Signed-off-by: Adrien Fillon <adrien.fillon@manomano.com>
pull-request-size
bot
added
the
size/L
|
We encountered this bug in production. Is there any plan to review the PR solving it? |
| @@ -263,7 +273,13 @@ func (c *ReadyChecker) serviceReady(s *corev1.Service) bool { | |||
| return true | |||
| } | |||
|
|
|||
| func (c *ReadyChecker) volumeReady(v *corev1.PersistentVolumeClaim) bool { | |||
| func (c *ReadyChecker) volumeReady(v *corev1.PersistentVolumeClaim, storageClass *storagev1.StorageClass) bool { | |||
| if storageClass != nil && | |||
There was a problem hiding this comment.
Is there a reason to expect storageClass could be nil here? getStorageClassFromPvc shouldn't return a nil value (except on error) I think?
| @@ -131,7 +135,13 @@ func (c *ReadyChecker) IsReady(ctx context.Context, v *resource.Info) (bool, err | |||
| if err != nil { | |||
| return false, err | |||
| } | |||
| if !c.volumeReady(claim) { | |||
|
|
|||
| storageClass, err := getStorageClassFromPvc(ctx, c.client, claim) | |||
There was a problem hiding this comment.
If I were to nitpick, perhaps volumeReady should (continue) to have all "volume ready" logic. And should call getStorageClassFromPvc .
(perhaps this would make volumeReady a candidate for refactoring itself, since adding this call would increase complexity there; but I don't think the right approach is to put some of the functionality into the caller here)
|
This is pretty annoying, and it looks like there's a fix pending for close to year now. Can this be merged please? |
As discussed in #10733.
Charts that have PVCs associated with resources not starting immediately after install (like a CronJob) will wait forever if the associated StorageClass has a volume binding mode of
WaitForFirstConsumer.This PR attempts to solve this by querying the associated StorageClass (or the default one) and checking its
volumeBindingModefield.Signed-off-by: Adrien Fillon adrien.fillon@manomano.com
What this PR does / why we need it:
WaitForFirstConsumerto be readySpecial notes for your reviewer:
I'm not 100% sure of the UX Helm wish for this kind of behavior, I could also go for a flag similar to
WaitForJobsto have aSkipWaitForPvcor something 馃If applicable: