Adding Ability to MergePatch CRDs properly #488
Conversation
|
I need to think about this one and look at available methods/APIs. This is a pretty sledgehammery solution. Are there instances that AREN'T CRDs where a StrategicMergePatch would fail but a MergePatch would work, and cause unintended consequences? Can we mark this entire set of code as immediately deprecated pending removal once Server Side Apply is in? |
|
Another solution we discussed that might be more viable - take the union of the object being applied and the server side object and apply the object's view of that as a merge patch, so that only the fields that the applying object are concerned with are applied. This solves the same semantic while being applicable to any JSON. |
|
Also we should add a test for the issue that this fixes. |
|
@kensipe Probably. If so, this could be extracted as a function that returns a ConstantPatch for reconcile to use. This would be a great small step! That way, we're not trying to test this massive reconcile function. |
We have now a test :) We ended up discussing more around those lines here https://kubernetes.slack.com/archives/CG3HTFCMV/p1562610928382300 and decided to not extract it into a function. |
| @@ -499,7 +499,12 @@ func (r *ReconcilePlanExecution) Reconcile(request reconcile.Request) (reconcile | |||
| log.Printf("Error getting patch between truth and obj: %v\n", err) | |||
| } else { | |||
| err = r.Client.Patch(context.TODO(), truth, client.ConstantPatch(types.StrategicMergePatchType, rawObj)) | |||
| log.Printf("PlanExecutionController: CreateOrUpdate Patch: %v", err) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Can we check the specific error here?
| // | ||
| // Reason: "UnsupportedMediaType" Code: 415 | ||
| switch e := err.(type) { | ||
| case *errors.StatusError: |
There was a problem hiding this comment.
Looks good to me. If you want, Kubernetes has some helper functions you can use for this: https://godoc.org/k8s.io/apimachinery/pkg/api/errors#IsUnsupportedMediaType
if errors.IsUnsuportedMediaType(err) {}
There was a problem hiding this comment.
Looks good to me. If you want, Kubernetes has some helper functions you can use for this: https://godoc.org/k8s.io/apimachinery/pkg/api/errors#IsUnsupportedMediaType
if errors.IsUnsuportedMediaType(err) {}
Ah nice! Didn't know about this helper function!
What type of PR is this?
/component operator
/kind bug
What this PR does / why we need it:
In #447 we've added StrategicMergePatchs, however custom resources apparently don't work currently with SMPs. With this patch, CustomResources can behave properly without throwing:
It works by being more verbose and catching the error of a failed StrategicMergePatch and applying an old MergePatch before throwing an error. When testing this worked with Zookeeper, Kafka and Flink.
Without this, a more complex "extension", e.g. like in the Flink demo in which we require instantiation of e.g. other Operators first wasn't working. Having for example https://github.com/kudobuilder/operators/blob/master/repository/flink/docs/demo/financial-fraud/demo-operator/templates/zookeeper.yaml as an
Instanceto be applied in a Phase would throw the above error.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
First, this needs further testing. In particular for the scenario like in the flink financial fraud demo.
To try it out run in the
kudobuilder/operatorsrepo:kudo install flink/docs/demo/financial-fraud/demo-operatorThe expected output should be:
Does this PR introduce a user-facing change?: