Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Ignore "404 Not Found" when patching after deletion during handling #160

Merged
merged 2 commits into from
Aug 8, 2019

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Aug 1, 2019

Ignore errors when patching an object that was removed too quickly, during the handling cycle or inside of the handlers.

Issue : #139

Description

If a handler deletes the object being handled, Kopf will try to patch its status after the handler as usually, and will fail, since the object does not exist anymore.

The same happens if the object is deleted too quickly, while the handler is running.

Previously, this was prevented by always having a finalizer before any handling. Since #24, the finalizer is added only when there are non-optional delete handlers, and skipped otherwise. So, it is now possible that the object is deleted while we handling it.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)

Review

  • Tests
  • Documentation

@nolar nolar added the bug Something isn't working label Aug 1, 2019
@nolar nolar requested a review from samurang87 as a code owner August 1, 2019 19:45
@zincr
Copy link

zincr bot commented Aug 1, 2019

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented Aug 1, 2019

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

@s-soroosh s-soroosh self-requested a review August 7, 2019 12:04
await loop.run_in_executor(config.WorkersConfig.get_syn_executor(), obj.patch, patch)
except pykube.ObjectDoesNotExist:
pass
except requests.exceptions.HTTPError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain in what circumstances this exception might raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Good that you've noticed that.

Pykube has its own handling of all exceptions, and overrides it with its own pykube.HTTPError only if they are applicaton/json and its json has kind=="Status" — regardless of the status code. I don't know how reliable this condition is, so it might happen that the requests's exceptions leak out.

But that pykube's exception should be caught too, same as in the auth.py (on verification).

And, unlike the GET-methods in fetching.py, pykube.ObjectDoesNotExist will not be raised in PATCH/POST/PUT-methods. But that might change in the future, so we at least expect it (for forward-compatibility).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with a new commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, being more conservative in there regards shouldn't hurt

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #160 into master will decrease coverage by 1.53%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   82.76%   81.23%   -1.54%     
==========================================
  Files          32       32              
  Lines        1596     1652      +56     
  Branches      231      241      +10     
==========================================
+ Hits         1321     1342      +21     
- Misses        230      262      +32     
- Partials       45       48       +3
Impacted Files Coverage Δ
kopf/clients/patching.py 87.09% <73.33%> (-12.91%) ⬇️
kopf/clients/events.py 80% <0%> (-4.85%) ⬇️
kopf/reactor/running.py 23.52% <0%> (-4.42%) ⬇️
kopf/structs/hierarchies.py 100% <0%> (ø) ⬆️
kopf/__init__.py 100% <0%> (ø) ⬆️
kopf/reactor/handling.py 85.85% <0%> (+0.21%) ⬆️

@nolar nolar merged commit ff8af12 into zalando-incubator:master Aug 8, 2019
@nolar nolar deleted the patch-of-unexistent branch August 8, 2019 11:59
embano1 pushed a commit to embano1/kopf-operator-vmworld that referenced this pull request Nov 1, 2019
Finalizer will address an issue with cleanup when the operator is down during deletion

Catch some notFound exections when deleting gone vSphere objects

Upgrade to kopf v0.22 to avoid [404s](zalando-incubator/kopf#160)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants