Fix handler conflict with livenessProbe overrides #701
Conversation
|
| @@ -44,6 +44,18 @@ func PatchPodSpecWithOverride(spec, override *corev1.PodSpec) (*corev1.PodSpec, | |||
| return nil, fmt.Errorf("can't marshal pod spec override: %w", err) | |||
| } | |||
|
|
|||
| // If any of the override containers have livenessProbe overrides, the entire object should be replaced with the override. | |||
There was a problem hiding this comment.
More specifically, should we check and replace probeHandler instead of the whole probe?
There was a problem hiding this comment.
Replacing the entire probe seemed cleaner (instead of looking for individual probeHandlers). But, I am open to changing it if that causes less confusion.
| } | ||
| } | ||
| } | ||
|
|
||
| patchedJSON, err := strategicpatch.StrategicMergePatch(orginalSpec, overrideSpec, corev1.PodSpec{}) |
There was a problem hiding this comment.
To be more generic, we'd better support the directives in https://github.com/kubernetes/kubernetes/blob/7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go#L41
const (
directiveMarker = "$patch"
deleteDirective = "delete"
replaceDirective = "replace"
mergeDirective = "merge"There was a problem hiding this comment.
This does looks to be a cleaner approach. Will require a bit more digging on my part before I can submit a PR. Would be great, if you or someone can beat me to it :). Otherwise, will get to it as soon as I can.
There was a problem hiding this comment.
Agree with @yujunz !
A more generic solution would be better suited for this use case.
Ping me if you need some help.
There was a problem hiding this comment.
@alexandrevilain @yujunz I am not quite sure yet how I'd go about respecting the directives in the operator. If either of you already know of an approach, I request that you proceed with the implementation. Thx.
There was a problem hiding this comment.
The implementation would be close to func mergeMap https://github.com/kubernetes/kubernetes/blob/7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go#L1322
Not sure if we can find a public interface to reuse. Maybe func StrategicMergeMapPatch in https://github.com/kubernetes/kubernetes/blob/7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go#L856
|
Let's close this issue as #720 fixes this too :) |
#694