temporal-operator icon indicating copy to clipboard operation
temporal-operator copied to clipboard

Need to be able to override/replace livenessProbe handler

Open uritig opened this issue 2 years ago • 3 comments

The default livenessProbe for a service looks like:

livenessProbe:
      failureThreshold: 3
      initialDelaySeconds: 30
      periodSeconds: 10
      successThreshold: 1
      tcpSocket:
        port: rpc
      timeoutSeconds: 1

We are able to override all the fields, except for the handler. For example, folks should be able to add overrides like below:

livenessProbe:
          grpc:
                    port: 7233
                    service: frontend.temporal.temporal.svc.cluster.local

or

livenessProbe:
          exec:
                    command: [
                              "/etc/temporal/grpc-health-probe/grpc_health_probe",
                              "-addr=:7233"
                              ...
                    ]

Unfortunately, right now this results in error : Deployment.extensions "temporal-frontend" is invalid: spec.template.spec.containers[0].livenessProbe.tcpSocket: Forbidden: may not specify more than 1 handler type as the operator attempts to add both handlers.

uritig avatar Apr 18 '24 01:04 uritig

Tried https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md#replace-directive

livenessProbe:
   $patch: replace
   failureThreshold: 3
   initialDelaySeconds: 30
   periodSeconds: 10
   successThreshold: 1

It seems not working.

I thought operator leverages strategic-merge-patch for overriding. Could you confirm? @alexandrevilain

yujunz avatar Apr 18 '24 01:04 yujunz

Default value seems hardcoded in https://github.com/alexandrevilain/temporal-operator/blob/c9e7054e2f8724017953007cdc7687442d9e1f6d/internal/resource/base/deployment_builder.go#L99

And overrides by https://github.com/alexandrevilain/temporal-operator/blob/c9e7054e2f8724017953007cdc7687442d9e1f6d/internal/resource/base/deployment_builder.go#L377

Going deep, it calls

https://github.com/alexandrevilain/temporal-operator/blob/c9e7054e2f8724017953007cdc7687442d9e1f6d/pkg/kubernetes/overrides.go#L114

https://github.com/alexandrevilain/temporal-operator/blob/c9e7054e2f8724017953007cdc7687442d9e1f6d/pkg/kubernetes/overrides.go#L86

https://github.com/alexandrevilain/temporal-operator/blob/c9e7054e2f8724017953007cdc7687442d9e1f6d/pkg/kubernetes/overrides.go#L47

It does call strategicpatch.StrategicMergePatch from https://pkg.go.dev/k8s.io/[email protected]/pkg/util/strategicpatch#StrategicMergePatch

Not sure why $patch: replace doesn't work.

yujunz avatar Apr 18 '24 12:04 yujunz

Hi!

Thanks for reporting the issue! I’ll review the fix as soon as possible!

alexandrevilain avatar Apr 20 '24 09:04 alexandrevilain