Replace Deployment with StatefulSet

[dylanpiergies]

Signed-off-by: Dylan Piergies [email protected]

[dylanpiergies]

Migrated from helm/charts#23126.

[torstenwalter]

I would rather not do this in a minor update, but we could include it for #41

[UkonnRa]

+1 any update?

[torstenwalter]

For me this looks good. I just need to do some testing and would love to get feedback from other maintainers as well.

As 3.0.0 is a breaking change we could also consider use volumeClaimTemplates.

Any idea on how a migration path could look like. Could we support users with that?

[wmcdona89]

While I get moving to a StatefulSet might be the right thing to do in principle, I need some convincing it's the right thing to do in practice for Jenkins.

Going beyond horizontal scalability considerations (since Jenkins doesn't scale horizontally), the arguments for StatefulSet over Deployment talk about how it eliminates the risk of accidentally deleting a PVC and how it addresses the problem of a rolling update getting stuck due to a ReadWriteOnce PVC.

Both of these issues can be addressed via chart configuration when using Deployment. Accidental PVC deletion can be avoided by setting persistence.annotations to "helm.sh/resource-policy": keep (which the chart could start doing by default). And the rolling update problem can be avoided by setting the update strategy to Recreate (which the chart already does by default).

Other considerations...

• Deployment gives users the flexibility of deleting the PVC on purpose such as when testing Jenkins.
• Moving to StatefulSet and volumeClaimTemplates will mean an existing claim could only be used if it follows the volumeClaimTemplates naming convention of [name + pod-name + ordinal number].

Anything else I should be considering?

[dylanpiergies]

@torstenwalter As this PR currently stands, there's no migration needed... the PVC provisioned in the chart can be used as-is. If we were to switch to using a volumeClaimTemplate by default, the migration will be more involved. This PR was originally targeted at a minor release, so I was deliberately avoiding breaking changes. Now that it's targeted at a major release, we might want to discuss further.

@wmcdona89

Both of these issues can be addressed via chart configuration when using Deployment. Accidental PVC deletion can be avoided by setting persistence.annotations to "helm.sh/resource-policy": keep (which the chart could start doing by default). And the rolling update problem can be avoided by setting the update strategy to Recreate (which the chart already does by default).

Whilst use of the Recreate update strategy does mitigate the contention problem surrounding ReadWriteOnce volumes, it does not solve it completely. In the case where a helm upgrade modifies the Deployment, it yields the correct behaviour: all Pods in the previous version are terminated, and no Pods of the new revision are created until the old ones have been removed. However, this is not the case if the Pod is deleted by the user or if it's evicted from its current Node or otherwise rescheduled for some reason; in these cases, the ReplicaSet managing the Pods will not await removal of the terminating Pod before creating a new one, and you still encounter errors when the replacement Pod tries to start. In practice, the removed Pod will eventually terminate, releasing its volume mounts, but the new Pod has to rely on retries to mount its volumes successfully. To put it bluntly: it's sloppy. As a rule of thumb, if you're running anything that uses ReadWriteOnce volumes, you should almost certainly be using a StatefulSet. The lifecycle of Pods managed by a StatefulSet is slightly different: the StatefulSet will await the removal of old Pods before creating replacements and the contention for the ReadWriteOnce PVC cannot occur.

• Deployment gives users the flexibility of deleting the PVC on purpose such as when testing Jenkins.

Moving to a StatefulSet does not prevent users from doing this. The change from Deployment to StatefulSet alone has no impact whatsoever. If we use volumeClaimTemplate as well, it would mean users will have to delete the volume explicitly (it won't get removed when the chart is uninstalled).

• Moving to StatefulSet and volumeClaimTemplates will mean an existing claim could only be used if it follows the volumeClaimTemplates naming convention of [name + pod-name + ordinal number].

As things currently stand, we are still using the PVC defined in home-pvc.yaml; since Jenkins doesn't scale horizontally, it's not imperative that we do use a volumeClaimTemplate. The real difference in doing so would mean that the created PersistentVolume is no longer managed by Helm, or even by the StatefulSet. It's simply created if it does not exist, and is then an independent resource that's left to the user to manage. As you rightly point out, though, switching to support only volumeClaimTemplate (or making it the default) would be a breaking change that would require attention during upgrade.

[torstenwalter]

Thanks for the detailed explanation. I am in favor of using a StatefulSet and keeping the existing PVC.

I've seen quite often that Jenkins could not be started as the PVC was still mounted at a different node

@wmcdona89 What's your opinion?

[wmcdona89]

@dylanpiergies thanks for the detailed response!

if the Pod is deleted by the user or if it's evicted from its current Node or otherwise rescheduled for some reason; in these cases, the ReplicaSet managing the Pods will not await removal of the terminating Pod before creating a new one, and you still encounter errors when the replacement Pod tries to start.

I see...I guess I've been fortunate to have never been bitten by this (or at least not in recent memory)

Moving to a StatefulSet does not prevent users from doing this. The change from Deployment to StatefulSet alone has no impact whatsoever. If we use volumeClaimTemplate as well, it would mean users will have to delete the volume explicitly (it won't get removed when the chart is uninstalled).

Ah...that's right. Since we're not using volumeClaimTemplates, the chart will still own the PVC and thus can delete it.

My remaining reservations are only with volumeClaimTemplates and since StatefulSet will resolve the Pod deletion/rescheduling issue then I'm good switching to StatefulSet without volumeClaimTemplates.

[dylanpiergies]

@torstenwalter @wmcdona89 Once burned... the first time I encountered the deadlock situation (it was a database of some sort, IIRC), I took some time to properly research and understand the various controller resources and their use cases. I've never personally used the Recreate deployment strategy. I guess the primary use case is for when you can tolerate some downtime as the rollout happens and can't supply the extra compute resource necessary to do a rolling update, or if your application cannot tolerate the situation where both old and new revisions are running simultaneously.

Anyway, I digress...

I don't really have any strong opinions on using volumeClaimTemplate. For new installations, I'd probably prefer to use a volumeClaimTemplate for the safety reasons others have mentioned, but for existing installations I probably wouldn't want to do the work to migrate, as it would be fiddly and error prone.

In any case, if we were to support volumeClaimTemplate at all, I do agree that it should be as an option and I'd prefer to do this as a separate change, since the complexity will be much higher and the implications for testing much greater.

[torstenwalter]

@dylanpiergies Thank you for this! It's a big step forward to improve this chart.