Delete pods if StatefulSet object is updated #307
Conversation
indradhanush
force-pushed
the
indradhanush/delete-pods-on-sts-change
branch
2 times, most recently
from
e439477
to
4b3f0b0
Compare
pkg/controller/v1beta2/controller.go
Outdated
| @@ -748,3 +766,18 @@ func (hc *HabitatController) findConfigMapInCache(cm *apiv1.ConfigMap) (*apiv1.C | |||
|
|
|||
| return obj.(*apiv1.ConfigMap), nil | |||
| } | |||
|
|
|||
| func (hc *HabitatController) deleteStatefulSetPods(hab *habv1beta1.Habitat, sts *v1beta2.StatefulSet) error { | |||
There was a problem hiding this comment.
The sts parameter seems to be unused here.
There was a problem hiding this comment.
Also, should we set the update strategy back to ondelete to avoid messing with the statefulset controller while it is busy deleting the pods?
pkg/controller/v1beta2/controller.go
Outdated
| func (hc *HabitatController) deleteStatefulSetPods(hab *habv1beta1.Habitat, sts *v1beta2.StatefulSet) error { | ||
| fs := fields.SelectorFromSet(fields.Set(map[string]string{ | ||
| habv1beta1.HabitatLabel: "true", | ||
| habv1beta1.HabitatNameLabel: hab.Name, |
There was a problem hiding this comment.
Hm, don't pods have any statefulset specific label to use in matching? I'm wondering if this is not going to be too wide. For example have three separate manifests specifying redis to be run and update just one of them. This selector will likely pick up all the pods from all three manifests, right?
There was a problem hiding this comment.
Also, could you test if just updating a size in the persistent storage causes all the pods to be deleted at once? Not sure if the persistent storage size change will be reflected in the pod template…
There was a problem hiding this comment.
Hm, don't pods have any statefulset specific label to use in matching?
No labels at the moment. The pods have a reference to the StatefulSet in the OwnerReference field though. But I was unable to use them as part of the FieldSelector query. It appears they're not supported at the moment. We could add labels while creating the StatefulSet though.
I can use the OwnerReference but that would mean manually iterating the list of pods and checking for that field. In that case, using DeleteCollection won't work as well and I'd have to use Delete in a for loop.
There was a problem hiding this comment.
Also, could you test if just updating a size in the persistent storage causes all the pods to be deleted at once? Not sure if the persistent storage size change will be reflected in the pod template…
Already discussed on Slack, but commenting for posterity's sake. Updating size of persistent storage is prohibited in StatefulSet objects and would require deleting and recreating it instead.
There was a problem hiding this comment.
So, I had a deeper look at the habv1beta1.HabitatNameLabel: hab.Name selector you are using. I thought that hab.Name something like redis or nginx, that is - a name that is used to construct a path to user.toml file (/hab/user/redis/config/user.toml), so certainly something that is not unique. I was wrong, hab.Spec.V1beta2.Service.Name is used for this purpose. And hab.Name needs to be a unique name of a habitat resource. hab.Name also becomes a unique name of the generated statefulset and hab.Name is put into pod template as habv1beta1.HabitatNameLabel, so it is not possible for two pods with the same value of the habv1beta1.HabitatNameLabel label to belong to two differently named statefulsets. So the selector you used is correct.
Also I had a look at statefulsets, because I was wondering how does a statefulset know which pods belong to it if there is no statefulset-specific label in the pod to describe this relationship. For that there is a thing in statefulset named podselector, which the statefulset can use to get all its pods. The selector is available under .Spec.Selector in statefulset. And it almost the same as the one you used. So I'd suggest passing the statefulset to the deleteStatefulSetPods function and using sts.Spec.Selector in the LabelSelector field in listOptions variable. The sts.Spec.Selector is a pointer, but there's no need to check for nil - this field must be specified in k8s >= 1.8 or the validation would fail. See: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#pod-selector
indradhanush
force-pushed
the
indradhanush/delete-pods-on-sts-change
branch
from
96b88b1
to
bb6631a
Compare
A supervisor bug prevents pods from successfully completing a leader election when an update is made to the StatefulSet object. This arises from the RollingUpdate behaviour of StatefulSet. However, deleting them all at once fixes the problem. In this commit we compare the older StatefulSet object with the updated one and delete all pods that belong to that object if the StatefulSet objects before and after the update are different. This check was necessary because the StatefulSet keeps getting updated even though nothing has changed as a result of the ConfigMap cache getting updated frequently. More about the bug here: habitat-sh/habitat#5264 Signed-off-by: Indradhanush Gupta <indra@kinvolk.io>
Signed-off-by: Indradhanush Gupta <indra@kinvolk.io>
indradhanush
force-pushed
the
indradhanush/delete-pods-on-sts-change
branch
from
bb6631a
to
a202b1b
Compare
|
LFAD. |
A supervisor bug prevents pods from successfully completing a leader
election when an update is made to the StatefulSet object. This arises
from the RollingUpdate behaviour of StatefulSet.
However, deleting them all at once fixes the problem. In this commit
we compare the older StatefulSet object with the updated one and
delete all pods that belong to that object if the StatefulSet objects
before and after the update are different. This check was necessary
because the StatefulSet keeps getting updated even though nothing has
changed as a result of the ConfigMap cache getting updated frequently.
More about the bug here: habitat-sh/habitat#5264