Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn on inconsistencies of a patch and a patched object #527

Merged
merged 6 commits into from
Sep 3, 2020

Conversation

nolar
Copy link
Owner

@nolar nolar commented Aug 31, 2020

What do these changes do?

Warn (in logs) if the patched object does not match the patch as a result of the PATCH operation. This should make such cases easier to debug, as they will be at least visible (previously, completely hidden).

Description

In some cases, when Kopf wanted to PATCH an object, the PATCH API request was performed, but the object didn't actually change. K8s API returns status 2xx for that, so the request is believed to be successful. This issue happened on multiple occasions:

  • The most often one was caused by the introduction of "structural schemas" in K8s 1.16+, when CRDs didn't have x-kubernetes-preserve-unknown-fields: true set on .status. As a result, state persistence was lost after the first handler, and the next handling cycle was not invoked.
  • With "status as a sub-resource" kind of CRDs, while Kopf didn't know about sub-resources yet. The results were the same: the status was not persisted, the next cycle was not called.
  • It can also happen in the future if another logic is added to K8s, so that in a PATCH request the data is not stored (speculatively: e.g. because of the field ownership by different operators).

Now, Kopf reconstructs the patched object from the responses of the PATCH requests (1 or 2 of per call) and compares that reconstructed object with the patch. If some patched fields mismatch, a warning is logged.

Note that the reconstructed object is not exactly the full object: it can be only the status stanza, or the main object without status, or an empty object — depending on which PATCH requests were actually needed and performed.

All object fields that are not in the patch, are ignored: this is normal that something changes remotely without this specific operator knowing: e.g. metadata's resourceVersion/generation fields are updated by K8s itself, other operators and controllers can store additional status fields or labels or annotations. As long as they are not in the scope of the current operator's intentions to modify the object, they are irrelevant.

Issues/PRs

Issues: #308 #321 #358 (and maybe more)

Related: replaces #339

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change which does not alter the behaviour)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

@nolar nolar added the bug Something isn't working label Aug 31, 2020
@nolar nolar force-pushed the warn-on-patch-inconsistencies branch from 862e0ed to 4337e7b Compare September 2, 2020 07:47
@nolar nolar marked this pull request as ready for review September 2, 2020 20:29
The implementation is conceptually based on another PR,
but it was heavily reworked to a forked repo, which went far ahead,
and the implementation is significantly changed:

* Instead of the RFC patch simulation locally, a one-way diff is used
  to check if the selected fields of the response
  are different from what is intended in the patch.
  Other fields and changes are ignored.
* The patch-vs-response check is also performed in daemons/timers.
* The tests are different.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant