Strimzi-kafka-operator: [Bug] Broker rolling upgrade loop when default value is set

[PaulLiang1]

Describe the bug

When initialDelaySeconds is set to 0 (instead of omitted), it would cause broker to stuck in rolling upgrade spin loop.

# Source: kafka/templates/kafkacluster.yml
apiVersion: kafka.strimzi.io/v1beta1
kind: Kafka
metadata:
  name: kafka-cluster
spec:
  kafka:
    replicas: 5
    readinessProbe: 
      initialDelaySeconds: 0

To Reproduce
Steps to reproduce the behavior:

1. Set broker livenessProbe.initialDelaySeconds to 0;
2. broker will keep upgrading indefinitely, with STS generation keep being updated;

Operator logs for spin loop

2020-12-03 05:38:51 DEBUG KafkaAssemblyOperator:3264 - Reconciliation #1(watch) Kafka(kafka/kafka-cluster): Rolling pod kafka-cluster-kafka-0 due to [Pod has old generation]
2020-12-03 05:38:51 DEBUG KafkaRoller:687 - Reconciliation #1(watch) Kafka(kafka/kafka-cluster): Creating AdminClient for kafka-cluster-kafka-0.kafka-cluster-kafka-brokers.kafka.svc.cluster.local:9091,kafka-cluster-kafka-1.kafka-cluster-kafka-brokers.kafka.svc.cluster.local:9091,kafka-cluster-kafka-2.kafka-cluster-kafka-brokers.kafka.svc.cluster.local:9091,kafka-cluster-kafka-3.kafka-cluster-kafka-brokers.kafka.svc.cluster.local:9091,kafka-cluster-kafka-4.kafka-cluster-kafka-brokers.kafka.svc.cluster.local:9091
2020-12-03 05:38:53 INFO  KafkaRoller:500 - Reconciliation #1(watch) Kafka(kafka/kafka-cluster): Pod 0 needs to be restarted. Reason: [Pod has old generation]

Expected behavior

1. no spin loop

Environment (please complete the following information):

• Strimzi version: 0.20.0
• Installation method: helm charts
• Kubernetes cluster: eks 1.16
• Infrastructure: aws eks

---

It's believed when a default value is set, k8s api will return null instead of actual default, where Strimzi detects it as diff and triggers rolling upgrade.

This might be able to generalized into when default value is set on any field where k8s return null or omit from the api response.

Operator Log

2020-12-03 05:38:51 DEBUG KafkaSetOperator:102 - StatefulSet kafka/kafka-cluster-kafka already exists, patching it
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/revisionHistoryLimit"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/metadata/annotations/strimzi.io~1generation"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:102 - StatefulSet kafka/kafka-cluster-kafka differs: {"op":"add","path":"/spec/template/spec/containers/0/livenessProbe/initialDelaySeconds","value":0}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:103 - Current StatefulSet path /spec/template/spec/containers/0/livenessProbe/initialDelaySeconds has value 
2020-12-03 05:38:51 DEBUG StatefulSetDiff:104 - Desired StatefulSet path /spec/template/spec/containers/0/livenessProbe/initialDelaySeconds has value 0
2020-12-03 05:38:51 DEBUG StatefulSetDiff:102 - StatefulSet kafka/kafka-cluster-kafka differs: {"op":"add","path":"/spec/template/spec/containers/0/readinessProbe/initialDelaySeconds","value":0}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:103 - Current StatefulSet path /spec/template/spec/containers/0/readinessProbe/initialDelaySeconds has value 
2020-12-03 05:38:51 DEBUG StatefulSetDiff:104 - Desired StatefulSet path /spec/template/spec/containers/0/readinessProbe/initialDelaySeconds has value 0
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/containers/0/terminationMessagePath"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/containers/0/terminationMessagePolicy"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/dnsPolicy"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/initContainers/0/env/0/valueFrom/fieldRef/apiVersion"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/initContainers/0/terminationMessagePath"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/initContainers/0/terminationMessagePolicy"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/restartPolicy"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/serviceAccount"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/template/spec/volumes/4/configMap/defaultMode"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/volumeClaimTemplates/0/spec/volumeMode"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/spec/volumeClaimTemplates/0/status"}
2020-12-03 05:38:51 DEBUG StatefulSetDiff:86 - StatefulSet kafka/kafka-cluster-kafka ignoring diff {"op":"remove","path":"/status"}
2020-12-03 05:38:51 DEBUG KafkaSetOperator:54 - Changed template spec => needs rolling update
2020-12-03 05:38:51 DEBUG StatefulSetOperator:305 - Patching StatefulSet kafka/kafka-cluster-kafka
2020-12-03 05:38:51 DEBUG KafkaSetOperator:168 - StatefulSet kafka-cluster-kafka in namespace kafka has been patched
2020-12-03 05:38:51 DEBUG KafkaAssemblyOperator:927 - Kafka.spec.kafka.version unchanged
2020-12-03 05:38:51 DEBUG KafkaRoller:203 - Reconciliation #1(watch) Kafka(kafka/kafka-cluster): Initial order for rolling restart [0, 1, 2, 3, 4]

[scholzj]

Why would you set the initial delay to 0? Is there any real usecase behind it? The readiness will never pass right from the beginning when the broker is starting.

[PaulLiang1]

The main use case is in some environment we would like to have the delay (when there are lots of topic on broker it could take some time to start up) while some environment we don't really care (QA/Dev environment).

That value is by default to be 0 anyway, and the main issue is that this behaviour could potentially be triggered when a value is supplied with the same as k8s default value (sts diff tp cause spin loop on potentially any field that have a default value)

[scholzj]

The default value is 15 (in Strimzi). I agree this is a bug. But it is important for us to understand the priorities. Whether there is some important use case and it is needed in order to fix it quickly. That is what I'm trying to find out.

[PaulLiang1]

Sorry for mixing up the default values.
I feel like this bug could be more general than just initialDelaySeconds, where any field with default value could be affected.
In terms of fixes, it would be great if it can be fixed programmatically, but even just by documenting it it would be a good start.

[scholzj]

I opened a PR for this.

I feel like this bug could be more general than just initialDelaySeconds, where any field with default value could be affected.
In terms of fixes, it would be great if it can be fixed programmatically, but even just by documenting it it would be a good start.

Kubernetes is not consistent in how it handles some of these situations. Different fields use different default values etc. So this is something what needs to be taken field by field really.