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

Delete: needs more preconditions #73648

Closed
lavalamp opened this issue Feb 1, 2019 · 9 comments · Fixed by #74040
Closed

Delete: needs more preconditions #73648

lavalamp opened this issue Feb 1, 2019 · 9 comments · Fixed by #74040
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@lavalamp
Copy link
Member

lavalamp commented Feb 1, 2019

Today you can use UID as a precondition for delete; this allows you to avoid deleting an object that has been replaced with an object of the same name.

As reported by @fasaxc on slack, there's also a need to ensure a delete fails if an unobserved change happens to an object. This could be implemented as either an RV or a Generation precondition.

Scenario:

  1. A observes object foo and sends a delete request.
  2. B observes object foo and modifies it in a way that would have prevented its deletion.
  3. A's delete request happens to arrive afterwards; the object is deleted and B's change is lost.

/sig api-machinery

@lavalamp lavalamp added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 1, 2019
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 1, 2019
@ajatprabha
Copy link
Contributor

@lavalamp I'd like to work on this issue.

@lavalamp
Copy link
Member Author

lavalamp commented Feb 1, 2019

@ajatprabha Go for it!

@krmayankk
Copy link

Are we saying that , if I get an object, store its resourceVersion and then issue a delete on the resourceVersion for that object , even though the object has been modified and hence its resourceVersion changed, the delete still succeeds ?

@ajatprabha
Copy link
Contributor

@krmayankk I think there is no RV based precondition check on delete. Only UID based check is there.

// Checking the Preconditions here to fail early. They'll be enforced later on when we actually do the deletion, too.
if options.Preconditions != nil && options.Preconditions.UID != nil && *options.Preconditions.UID != objectMeta.GetUID() {
return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *options.Preconditions.UID, objectMeta.GetUID()))
}

I'm a little confused when you say issue a delete on resourceVersion for that object. Do you mean like delete the object with old stored RV? In that case, I think currently the delete succeeds as I can't find a check.

@ajatprabha
Copy link
Contributor

@lavalamp I'm looking at #22965 to see which places need precondition checks. Will a simple Preconditions.ResourceVersion check suffice to solve the issue here? Also, how will a Generation precondition work if it is to be used?

@lavalamp
Copy link
Member Author

lavalamp commented Feb 4, 2019

RV precondition: only delete the object if its current RV equals the specified one.

Generation precondition: only delete the object if its current metadata.Generation equals the specified one.

Either one works, the generation one is more tolerant of random status updates. There's possibly room in the world for both.

@deads2k might have an opinion?

If we're going to have more than 2 or three of these things we probably need to make a general mechanism :/ But I think people can use RV or Generation preconditions to stand in for an arbitrary check, so if anything deserves a top level precondition, it'd be those.

@lavalamp
Copy link
Member Author

Thanks, @ajatprabha!

@krmayankk
Copy link

This is good to know, I thought the RV based update/delete worked and would always result in conflict if RV changed. I wonder where do these kinds of changes get documented . May be worth an entry in the api conventions doc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants