-
Notifications
You must be signed in to change notification settings - Fork 740
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
Use patch to set finalizers #317
Use patch to set finalizers #317
Conversation
e5ce293
to
aa596b4
Compare
No objections to changing this to patch. I'm curious why you think gatekeeper would change the deletion timestamp? Gatekeeper does not modify those. I do see this bit of information WRT deletion timestamps and pods, unsure if it's relevant: |
Just a guess. Something pretty strange is going on! |
@maxsmythe are there integration tests which explicitly test for finalizer removal? |
Yep, the config controller tests look for sync finalizer removal:
|
Would be nice to add a test for sync finalizer removal in e2e, similar to here: https://github.com/open-policy-agent/gatekeeper/blob/master/test/bats/test.bats#L82 |
@ritazh Is this blocking for you? |
no. I have created #323 to track. |
For pods in terminating state, their deletion timestamp is constantly increasing, I'm not sure why but I suspect its do to with gatekeeper. As a result, their resource version increases. This means that when we try to remove the finalizer, there is an error 'this object has already changed'. I think that using a patch will fix this. Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
aa596b4
to
0b1e14a
Compare
For pods in terminating state, their deletion timestamp is constantly
increasing, I'm not sure why but I suspect its do to with gatekeeper. As
a result, their resource version increases. This means that when we try
to remove the finalizer, there is an error 'this object has already
changed'. I think that using a patch will fix this.