Closed com6056 closed 1 month ago
@drivebyer any chance I could get a review of this? 🙏 Thanks! 🎉
@com6056 I noticed there are many changes in the API. Do these changes result in breaking changes?
@com6056 I noticed there are many changes in the API. Do these changes result in breaking changes?
They shouldn't be breaking, the probes had all of the same fields as the corev1.Probe struct, so it should be a no-op for anyone that doesn't actually set a probe handler
It looks like this was also originally added in https://github.com/OT-CONTAINER-KIT/redis-operator/pull/178, but then for some reason removed in https://github.com/OT-CONTAINER-KIT/redis-operator/pull/302
Attention: Patch coverage is 7.69231%
with 24 lines
in your changes are missing coverage. Please review.
Project coverage is 39.12%. Comparing base (
d121d86
) to head (7e3ecde
). Report is 56 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
k8sutils/statefulset.go | 0.00% | 18 Missing :warning: |
k8sutils/redis-replication.go | 0.00% | 2 Missing :warning: |
k8sutils/redis-sentinel.go | 33.33% | 2 Missing :warning: |
k8sutils/redis-standalone.go | 0.00% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Does Kubernetes validate probe fields if set InitialDelaySeconds
to -1? @com6056
Does Kubernetes validate probe fields if set
InitialDelaySeconds
to -1? @com6056
It doesn't appear that it does in CRDs (I'm guessing the operator would just fail to create the StatefulSet), and kubebuilder unfortunately doesn't seem to have a way to add validation to nested fields for this 😞
If we want to keep the existing validation we can just add the ProbeHandler
to the existing Probe
struct, happy to make that change if we want, but I figured switching to just the upstream corev1.Probe
would be better long-term rather than maintaining our own identical struct
but I figured switching to just the upstream corev1.Probe would be better long-term rather than maintaining our own identical struct
agree.
BTW, I test this yaml:
apiVersion: v1
kind: Pod
metadata:
name: nginx-pod
spec:
containers:
- name: nginx-container
image: nginx
ports:
- containerPort: 80
readinessProbe:
initialDelaySeconds: -1
periodSeconds: -1
httpGet:
path: /index.html
port: 80
➜ tools git:(main) ✗ kubectl -n mcamel-system --kubeconfig /Users/XXX/Documents/kubeconfig/xx-dev.yaml apply -f yamls/test.yaml
The Pod "nginx-pod" is invalid:
* spec.containers[0].readinessProbe.initialDelaySeconds: Invalid value: -1: must be greater than or equal to 0
* spec.containers[0].readinessProbe.periodSeconds: Invalid value: -1: must be greater than or equal to 0
Description
Add the ability to specify your own
ProbeHandler
, which switches theProbe
type to the upstreamcorev1.Probe
.It looks like this was originally added in https://github.com/OT-CONTAINER-KIT/redis-operator/pull/178, but then for some reason removed in https://github.com/OT-CONTAINER-KIT/redis-operator/pull/302 🤔
@iamabhishek-dubey can you provide any context here? Is this change reasonable?
Type of change
Checklist
Additional Context
For context, we really need to check more of the cluster state before rolling pods, so we want to add our own probe (something like this, although haven't tested it yet):
It looks like this was asked for in https://github.com/OT-CONTAINER-KIT/redis-operator/issues/170 as well, and I wonder if we should just build this functionality into the operator itself rather than having everyone have to figure out their own method for ensuring the cluster rolls safely.