Do not exec into pods during rolling update

[ansd]

This closes #304

Exec into pods only if StatefulSet is ready and up to date.

Before this commit, we observed in #304 that the controller tried to exec into pods at the same time as the pods got updated due to a StatefulSet restart resulting in connection errors.

The main change is to not set plugins if

sts.Status.ReadyReplicas < desiredReplicas || sts.Status.UpdatedReplicas < desiredReplicas

because during a StatefulSet restart, it happened multiple times in #304 that since sts.Status.ReadyReplicas == desiredReplicas, the exec commands got executed and the connection got interrupted because the pod got updated.

The main finding is that sts.Status.ReadyReplicas == desiredReplicas can be true although the StatefulSet rolling update is still ongoing:

$ while true; do  kubectl get statefulsets.apps definition-rabbitmq-server --template={{.status}}; echo "" ; sleep 1; done

map[collisionCount:0 currentReplicas:2 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:3 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d]
…
map[collisionCount:0 currentReplicas:2 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:2 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d]
…
map[collisionCount:0 currentReplicas:2 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:2 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d updatedReplicas:1]
…
map[collisionCount:0 currentReplicas:1 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:3 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d updatedReplicas:1]
…
map[collisionCount:0 currentReplicas:1 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:2 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d updatedReplicas:1]
…
map[collisionCount:0 currentReplicas:1 currentRevision:definition-rabbitmq-server-56b65fdc4f observedGeneration:5 readyReplicas:2 replicas:3 updateRevision:definition-rabbitmq-server-6f667f5f7d updatedReplicas:2]
…

Thank you @Gsantomaggio for helping troubleshooting 🙂

[mkuratczyk]

Why do we even try to run rabbitmq-plugins set if the cluster is restarting? Since #224 is done, we should only exec into the container to enable/disable plugins if the plugins ConfigMap is changed while the cluster is running. If the cluster configuration is changed (a different ConfigMap), we should restart the cluster and it should load plugins on startup based on the plugins ConfigMap - no need to run rabbitmq-plugins set at all. What am I missing? :)

[Gsantomaggio]

It doesn't crash anymore, but I think there is still some problem.

I tried to change the config map, by changing some value:

Changed:

queue_master_locator                                     = random
cluster_formation.node_cleanup.interval         = 50

Then if you check the config map, the values don't change

 ~ kubectl get configmaps definition-rabbitmq-server-conf -o yaml | grep queue_master_locator
queue_master_locator                            = min-masters

[ansd]

@mkuratczyk as we discussed this morning, you're right, we should only exec into the container to enable/disable plugins if the plugins ConfigMap is changed. We can and should still use the same guards as in this PR to only exec if all replicas are ready and not in the middle of a rolling update.

[ansd]

@Gsantomaggio that's desired behaviour.

You changed the configmap directly (e.g. via kubectl edit configmap definition-rabbitmq-server-conf).
Your configmap changed indeed, but your changes got shortly thereafter overwritten by the controller reconcile loop.

You should instead apply the RabbitMQ Custom Resource setting the additionalConfig like:

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
...
spec:
  rabbitmq:
    additionalConfig: |
      queue_master_locator = random
      cluster_formation.node_cleanup.interval = 50

[Gsantomaggio]

Ok, thank you @ansd

So why does the operator reboot the pod when the config map is changed?
I mean, if the original configuration is restored, the operator should not reboot the pods, right? Or I am missing something :)!

[ansd]

Hey @Gsantomaggio 🙂

Whenever the configuration is updated, the operator restarts the StatefulSet.

That's desired behaviour because https://www.rabbitmq.com/configure.html states that

rabbitmq.conf and advanced.config changes take effect after a node restart

This logic happens in

cluster-operator/controllers/rabbitmqcluster_controller.go

Line 318 in 585c609

if builder.UpdateRequiresStsRestart() && operationResult == controllerutil.OperationResultUpdated {

and

cluster-operator/internal/resource/configmap.go

Line 53 in 585c609

return true

In your specific case, you're right that the operator unnecessarily restarts the StatefulSet. So that's a side effect of changing the configuration in the wrong way.

[Zerpet]

I made the change in the system tests to not import the internal library and use a test constant. Given that we are 3 people at the moment and 2/3 have reviewed this, I will merge the PR when the checks pass.