Change operation type to update for services #224
Conversation
The fact that we were using a "patch" operation on services is leading to some errors in the reconcile loop. The "patch" should be reserved to use cases where we read an object from the system, make some changes to our local copy, and then want to "patch" it using the API to only send those fields that have changed. What we were doing was creating a brand new object and attempting to "patch" that against what the running system already had which lead to trying to delete several runtime only attributes which I believe is what was leading to the errors. Signed-off-by: Allain Legacy <[email protected]>
| @@ -896,7 +896,7 @@ func (t *reconcilerTask) createService(ctx context.Context, resourceName string) | |||
| } | |||
|
|
|||
| t.logger.InfoContext(ctx, "[createService] Create/Update/Patch Service: ", "name", resourceName) | |||
| if err := utils.CreateK8sCR(ctx, t.client, newService, t.object, utils.PATCH); err != nil { | |||
| if err := utils.CreateK8sCR(ctx, t.client, newService, t.object, utils.UPDATE); err != nil { | |||
There was a problem hiding this comment.
Off topic/general comment...CreateK8sCR seems to do way too many things right? One func to do create, update, patch, taking ownership, doing all sorts of reflection...
There was a problem hiding this comment.
The purpose was to create and update objects in the same function so that the main reconciliation loop is cleaner. When you create an object, it's when you also specify its ownership, so these should be together anyway.
The inventory controller makes sure the servers exist at all times (so it recreates them if they've been deleted) and that they look as specified in the Inventory CR spec (so it updates/patches if someone edits them directly).
I suppose we could separate the updates from creation in different functions.
What enhancement did you have in mind for this?
There was a problem hiding this comment.
Few suggestions:
- We cant say "Create" and then take an operation parameter (which happens to an arbitrary string)
- This is not simply "Create", violating single responsibility principle. Instead we break it down into
updateOrPatchObjectcreateObjectetc etc. And then finally the caller should explicitly check for errors as needed e.g.k8srr.IsAlreadyExists(err)for create - Given we already have access all the generated code for all CRUD reqs....we should try to use them directly instead of figure out types at runtime with
Object. i.e c.Create(ctx, job) (job being a struct type job). This means more code but idiomatic/in-line with Go
wdyt?
There was a problem hiding this comment.
What's going to happen if we split it into a updateOrPatchObject and a createObject is that someone will get tired of seeing that pattern spread throughout the code and will create a wrapper for that flow called createOrUpdateObject and we're more or less back to this starting point.
I do agree that the name could be improved upon, and certainly how we use it will need to be addressed in the future because as of now the 'update' is rather unsophisticated and doesn't handle needing to modify objects very gracefully at all. They all end up being just a bulk replace of the object which is likely causing issues to anyone doing a watch on them (potentially)
There was a problem hiding this comment.
I agree that renaming it to smth like ensureK8sCR would be a better description of the functionality.
I don't really agree that this is violating the SRP principle, especially with the renaming and possibly enhancing the update.
There was a problem hiding this comment.
Yeah definitely doing createObject would ultimate be wrapped up. Which is why it's better to be explicit (the third point) and pass the exact typed...so instead we should have createJob or createDeployment instead of super generic funcs
So this is really an |
|
@Missxiaoguo The issue is that patch should not be used in this way. It was intended to be used like this: old := get("some object")
new := old.clone()
new.someField = "new value"
if err := utils.CreateK8sCR(ctx, t.client, new, old, utils.PATCH); err != nil {because it will try to do a diff between the old and new objects and only replace the fields that have been modified. BUT... if you do a diff between a statically created object and the current runtime version of an object the diff will contain many fields (some of which are critical) and so doing a patch on it will try to remove those fields which in turn leads to errors. If you look at the other usages of PATCH in our code they are done properly. This one for some reason was accidentally set to PATCH instead of UPDATE. |
Yes, I understand. Anyhow, we should switch to Update since it’s not supposed to be used that way. I just wanted to point out that with Update, the fields from the runtime version that aren’t present in the new object will also be removed as it's a replace. So, in this case, the incorrect use of Patch leads to the same result but yes, we shouldn't use Patch here. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: browsell The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
The fact that we were using a "patch" operation on services is leading to some errors in the reconcile loop. The "patch" should be reserved to use cases where we read an object from the system, make some changes to our local copy, and then want to "patch" it using the API to only send those fields that have changed. What we were doing was creating a brand new object and attempting to "patch" that against what the running system already had which lead to trying to delete several runtime only attributes which I believe is what was leading to the errors.