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

Allow override of AllowCreateOnUpdate with new argument to Update #65154

Merged

Conversation

@jennybuckley
Copy link
Contributor

commented Jun 15, 2018

What this PR does / why we need it:
Changes the Update function signature to include a new bool which tells storage to override what the UpdateStrategy returns for AllowCreateOnUpdate. This is not exposed to the user, the handler is the one that sets this override value. Eventually the patch handler will set this to true, in order to provide more consistent apply behavior, without changing the existing PUT behavior.

Redo of #65075 but on master to reduce number of conflicts when we merge feature-serverside-apply with master.

/sig api-machinery
/cc @apelisse @lavalamp

Release note:

NONE

No release note because this is just an internal change

@k8s-ci-robot k8s-ci-robot requested review from apelisse and lavalamp Jun 15, 2018

@jennybuckley jennybuckley changed the title Allow override AllowCreateOnUpdate with new argument to Update Allow override of AllowCreateOnUpdate with new argument to Update Jun 15, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:add-update-options-3 branch from ae3b2ce to c24b1a8 Jun 15, 2018

}

// DefaultUpdateConfig returns an UpdateConfig implementation.
func DefaultUpdateConfig(createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreateOnUpdate bool) UpdateConfig {

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jun 15, 2018

Member

What do you think about not taking "forceAllowCreateOnUpdate" as a parameter and instead having a .SetForceAllowCreateOnUpdate() method (builder pattern) to reduce call site churn when we add uses of this?

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 20, 2018

Author Contributor

I like this idea a lot. The only reason I didn't originally use that pattern is because I was trying to make it as similar to UpdatedObjectInfo as possible, but I think it would still be consistent using the builder pattern

@@ -96,6 +96,6 @@ func (r *StatusREST) Get(ctx context.Context, name string, options *metav1.GetOp
}

// Update alters the status subset of an object.
func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc) (runtime.Object, bool, error) {
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation)
func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, config rest.UpdateConfig) (runtime.Object, bool, error) {

This comment has been minimized.

Copy link
@deads2k

deads2k Jun 21, 2018

Contributor

This construct concerns me. I suspect it's being done to avoid "forcing" people to deal with compilation errors. But when a new concept arises, the implementers need to be aware of what's happening. A compilation error is that indication. If we had this container object before and simply added to it, custom storages could accidentally "work" but not actually support dry-run.

@liggitt @sttts @mfojtik we've all been on the receiving end of these signatures changing over the past 3-ish years. I'd rather break than "work" and fail weirdly at runtime. You guys?

This comment has been minimized.

Copy link
@sttts

sttts Jun 21, 2018

Contributor

+1. I think we had the discussion about the Update signature before while adding the two admission funcs. We intentionally decided for the current way, even if it looks a bit ugly to change all implementations. But admission is important enough to break compilation if something changes.

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 26, 2018

Author Contributor

That's a good point, but I'm not sure how practical adding a new argument would be for something like "forceAllowCreateOnUpdate". Do we just want the signature to keep growing?

Also, I think the possibility of custom storages accidentally working when a new field is added to one of the arguments already exists, in the case of DeleteOptions, GetOptions, and when we add CreateOptions, etc.

In the case of forceAllowCreateOnUpdate, the default behavior of the new field is the behavior that we already expected from the storage, and the behavior not being performed when the caller requests it isn't going to cause any problems.

Do you think it would possibly be enough to add a comment documenting that fields shouldn't be added to the UpdateConfig without considering the implications for custom storages?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jun 26, 2018

Member

One of the things I'm about to start arguing for more generally is "we never break api machinery consumers, ever". It is just a constraint on our output that consumers get to pick and choose the rate at which they adopt api machinery features: the old code must continue not only to compile, but also to behave the same.

We don't live in that world yet, but I want to get there.

Concretely, here, what that might look like is adding a parallel UpdateWithOptions function and allow users to implement that when they're ready to start accepting the options.

Since evolving function signatures with every change doesn't seem very maintainable, we can define a way for users to declare which semantics they understand/implement. That could just be an int that increments every type we add a feature, or it could be a struct with a bool for every feature, etc. The use sets this in their code and they only change it when they want to start using the new feature. We e.g. have a field in the config to hold this, and the user must set it. (This is probably not a fully-baked idea, just an example of the sort of thing that might solve the problem.)

In short, I agree silent failure is not allowable, but I don't believe that breaking compilation is the only (or best) way to fix that.

It does however seem we have to break compilation at least once to get things into the proper shape. We should make sure we're set up so that next time, we both don't break compilation AND users have to opt-in to the new behavior. (And we might as well use that mechanism so that users have to opt-in to supporting dry run, while we're at it.)

This comment has been minimized.

Copy link
@sttts

sttts Jun 30, 2018

Contributor

Concretely, here, what that might look like is adding a parallel UpdateWithOptions function and allow users to implement that when they're ready to start accepting the options.

Or we start versioning our interfaces including their methods:

type UpdaterV2 interface {
  UpdateV2(....)
}

Ugly, but might actually work and is more sustainable than UpdateWithOptions and UpdateWithMoreOptions.

@@ -207,6 +207,26 @@ type UpdatedObjectInfo interface {
UpdatedObject(ctx context.Context, oldObj runtime.Object) (newObj runtime.Object, err error)
}

// UpdateConfig provides information about the validation functions that
// the Updater should use, and allows for overriding certain UpdateStrategy behavior.
type UpdateConfig interface {

This comment has been minimized.

Copy link
@sttts

sttts Jun 21, 2018

Contributor

why do we need this abstraction? Why not a struct?

@@ -207,6 +207,26 @@ type UpdatedObjectInfo interface {
UpdatedObject(ctx context.Context, oldObj runtime.Object) (newObj runtime.Object, err error)
}

// UpdateConfig provides information about the validation functions that
// the Updater should use, and allows for overriding certain UpdateStrategy behavior.
type UpdateConfig interface {

This comment has been minimized.

Copy link
@sttts

sttts Jun 21, 2018

Contributor

why do we need this abstraction? Why not a struct?

This comment has been minimized.

Copy link
@sttts

sttts Jun 21, 2018

Contributor

It enforces that the constructor is called for the private struct. But other than that it feels overabstracted.

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 26, 2018

Author Contributor

I think that having it as an interface is the only way to make it possible to use the builder pattern,
suggested by this comment

What do you think about not taking "forceAllowCreateOnUpdate" as a parameter and instead having a .SetForceAllowCreateOnUpdate() method (builder pattern) to reduce call site churn when we add uses of this?

@lavalamp

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:add-update-options-3 branch from c24b1a8 to 0016d8b Jun 26, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:add-update-options-3 branch 5 times, most recently from b0f8e16 to ef6bd17 Jun 26, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:add-update-options-3 branch from ef6bd17 to d10e08f Jun 28, 2018

@apelisse

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

This does what we discussed. I'd like someone from RH to approve, but from a code perspective:
/lgtm

func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc) (runtime.Object, bool, error) {
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation)
func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool) (runtime.Object, bool, error) {
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate)

This comment has been minimized.

Copy link
@liggitt

liggitt Jun 29, 2018

Member

for subresources, I thought we weren't allowing create-on-update (c.f. #65150 (comment)). doesn't that mean subresource rest storage should always propagate false to it's underlying store?

same comment applies to all the subresource storages.

This comment has been minimized.

Copy link
@sttts

sttts Jun 29, 2018

Contributor

Which would mean to ignore the bool. Shouldn't this be an error instead?

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 29, 2018

Author Contributor

I think propagating false makes sense in this case. we can't guarantee that all resources with custom storage implementations will have the ability to create on update, in those cases it would probably be better to just not honor the flag, so they can still be updated.

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 29, 2018

Author Contributor

I'll change this one, and make sure no other subresources are doing it this way

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jun 29, 2018

Author Contributor

@liggitt
Okay, fixed this

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 29, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

/lgtm
/approve

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

/assign @deads2k

for approval

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, deads2k, jennybuckley, sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Automatic merge from submit-queue (batch tested with PRs 65299, 65524, 65154, 65329, 65536). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit dcf296a into kubernetes:master Jul 2, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@@ -326,7 +326,7 @@ func (rs *REST) healthCheckNodePortUpdate(oldService, service *api.Service, node
return true, nil
}

func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc) (runtime.Object, bool, error) {
func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool) (runtime.Object, bool, error) {
oldObj, err := rs.services.Get(ctx, name, &metav1.GetOptions{})

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 3, 2018

Member

@jennybuckley just noticed this... this will return a NotFound error and short-circuit, right? need a follow up for this and any other custom Update() impls to honor forceAllowCreate and add tests for it

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Jul 3, 2018

Author Contributor

Great catch! I'll make an issue to track it since it's too late for a TODO.
it shouldn't cause any bugs right now because forceAllowCreate will never be set to true yet.

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 3, 2018

Member

sounds good, thanks

k8s-github-robot pushed a commit that referenced this pull request Jul 9, 2018

Kubernetes Submit Queue
Merge pull request #65723 from jennybuckley/apply-can-post-6
Automatic merge from submit-queue.

[FEATURE BRANCH] Force allow create on patch if content type is apply

**What this PR does / why we need it**:
Force allow create on patch if content type is apply and change the apply test to use a resource which doesn't have create-on-update enabled.

**Special notes for your reviewer**:
Based on top of #65720 because it relies on #65154 from master.
Only 083a596 is part of this PR.

**Release note**:
```release-note
NONE
```

/sig api-machinery
/kind feature
/cc @seans3 @liggitt @apelisse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.