dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

apelisse · 2018-06-14T16:15:53Z

What this PR does / why we need it:
Create new options for Create and Update (through POST/PUT/PATCH).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

lavalamp · 2018-06-18T23:21:27Z

pkg/registry/apps/deployment/registry.go

@@ -65,7 +65,7 @@ func (s *storage) GetDeployment(ctx context.Context, deploymentID string, option
 }

 func (s *storage) CreateDeployment(ctx context.Context, deployment *extensions.Deployment, createValidation rest.ValidateObjectFunc) (*extensions.Deployment, error) {
-	obj, err := s.Create(ctx, deployment, createValidation, false)
+	obj, err := s.Create(ctx, deployment, createValidation, &metav1.CreateOptions{})


Maybe make a metav1.CreateOptionsTODO() function so you can be sure to find and fix all of these after the fact? If you go this route, anyway.

deads2k · 2018-06-21T12:11:56Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

+
+	// DryRun decides if the request should be completed or not.
+	// +optional
+	DryRun bool `json:"dryRun,omitempty" protobuf:"varint,1,opt,name=dryRun"`


I think the decision from the call yesterday was to make this an enumerated string.

Absolutely, I haven't taken the time to update yet.

deads2k · 2018-06-21T12:21:09Z

@sttts I'm behind at the moment. I've got a comment on the prereq pull and the API here needs an update. Got time while I deal with the downstream?

sttts · 2018-06-22T08:39:49Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

+
+	// If IncludeUninitialized is specified, the object may be
+	// returned without completing initialization.
+	IncludeUninitialized bool `json:"includeUninitialized,omitempty" protobuf:"varint,2,opt,name=includeUninitialized"`


this surprises me. How is this related to dry-run? Why do we expose that via an API "suddenly"?

Also note that initializers go away (if I have understood @smarterclayton correctly). We should not expose a parameter for something that is planned to be removed.

Ok, found it: 69e757a#diff-7861f33474e4eb8d21cf1d33de5e4d4eL114 had the logic before, with a manually parsed get var. So nvm my comment above.

sttts · 2018-06-22T08:42:10Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

@@ -260,6 +260,14 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco

 				ae := request.AuditEventFrom(ctx)
 				audit.LogRequestObject(ae, obj, scope.Resource, scope.Subresource, scope.Serializer)
+			} else {
+				if values := req.URL.Query(); len(values) > 0 {


this is guarded by if checkBody. Looks odd.

In the delete handler we call the guarding var allowsOptions.

Yeah, I'm not sure what the point of these variable is, but it looks like "checkBody" and "allowOptions" have the same goal, so this is "correct".

sttts · 2018-06-22T08:49:52Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

@@ -111,7 +111,9 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
 		}

 		// TODO: replace with content type negotiation?
-		includeUninitialized := req.URL.Query().Get("includeUninitialized") == "1"
+		options := &metav1.CreateOptions{


~~Shouldn't this be using the parameter decoder? This looks hacky.~~ Next commit.

sttts · 2018-06-22T09:11:38Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

+				}
+			}
+			// Timeout is not part of the options, drop it.
+			delete(values, "timeout")


DecodeParameters ignores params that are not in the struct (necessary for new clients speaking to old servers). So, no need for the delete.

sttts · 2018-06-22T09:17:08Z

pkg/master/reconcilers/lease.go

@@ -221,7 +221,8 @@ func (r *leaseEndpointReconciler) doReconcile(serviceName string, endpointPorts
 	}

 	glog.Warningf("Resetting endpoints for master service %q to %v", serviceName, masterIPs)
-	return r.endpointRegistry.UpdateEndpoints(ctx, e, rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc)
+	// PLEASE_PAY_ATTENTION(apelisse): No idea if this is what we need to do here. Does it make sense to have DryRun here anyway?


this is the leader election code which speaks directly to the registry to update the (kubernetes service) endpoints. So this certainly does not need do a dry-run here. It is very very special purpose.

deads2k · 2018-07-10T20:44:26Z

forgot we didn't settle the param type... string vs []string

Heh, I just thought to ask about that too.

/approve cancel

liggitt · 2018-07-12T13:49:53Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

@@ -453,6 +453,42 @@ type DeleteOptions struct {
 	// foreground.
 	// +optional
 	PropagationPolicy *DeletionPropagation `json:"propagationPolicy,omitempty" protobuf:"varint,4,opt,name=propagationPolicy"`
+
+	// Whether the request should be completed and modifications will not be persisted.


I'd suggest something more like this:

When present, indicates that modifications should not be persisted.
An invalid or unrecognized dryRun directive will result in a BadRequest response and no further processing of the request.

and then we can add details about what directives are supported as we determine that

apelisse · 2018-07-12T15:34:57Z

/retest

deads2k · 2018-07-12T15:38:13Z

type looks correct now.

/approve

liggitt · 2018-07-12T15:41:07Z

/hold cancel
/lgtm

k8s-ci-robot · 2018-07-12T15:41:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/master/OWNERS~~ [deads2k]
~~pkg/registry/OWNERS~~ [deads2k,liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apelisse · 2018-07-12T16:16:53Z

/retest

k8s-github-robot · 2018-07-12T20:53:04Z

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

@deads2k

Automatic merge from submit-queue (batch tested with PRs 66158, 66041, 66210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove manually written typed registries These were only used in a handful of places, and were not consistently available for all types. They add a lot of call sites for PRs like #65105 and are not generally useful (very few callers have the ability to construct the underlying store). This PR switches the scale subresources to use the underlying store directly (like the status subresources already were), and removes the manually written Registry impls. /sig api-machinery /kind cleanup /assign @deads2k /hold will hold for #65105 and rebase after that ```release-note NONE ```

@deads2k

Automatic merge from submit-queue (batch tested with PRs 66158, 66041, 66210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove manually written typed registries These were only used in a handful of places, and were not consistently available for all types. They add a lot of call sites for PRs like kubernetes/kubernetes#65105 and are not generally useful (very few callers have the ability to construct the underlying store). This PR switches the scale subresources to use the underlying store directly (like the status subresources already were), and removes the manually written Registry impls. /sig api-machinery /kind cleanup /assign @deads2k /hold will hold for kubernetes/kubernetes#65105 and rebase after that ```release-note NONE ``` Kubernetes-commit: 43b801d499f95a4a89fe3319347225e9ad643359

roycaihw · 2018-10-01T18:34:01Z

It looks like we added meta.v1.CreateOptions and meta.v1.UpdateOptions in this PR, but they weren't reflected in our openapi spec. The dryRun query parameter was only added to the delete operations in the spec. I'm wondering if this was intended?

k8s-ci-robot requested review from krousey and liggitt June 14, 2018 16:16

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 14, 2018

apelisse force-pushed the dry-run branch 4 times, most recently from 133048b to 45640e8 Compare June 18, 2018 16:10

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 18, 2018

apelisse force-pushed the dry-run branch from 45640e8 to c08181b Compare June 18, 2018 16:24

lavalamp reviewed Jun 18, 2018

View reviewed changes

apelisse force-pushed the dry-run branch 2 times, most recently from 9a4dd58 to cacf1ac Compare June 19, 2018 22:59

deads2k reviewed Jun 21, 2018

View reviewed changes

sttts reviewed Jun 22, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2018

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2018

liggitt mentioned this pull request Jul 10, 2018

Remove manually written typed registries #66041

Merged

apelisse force-pushed the dry-run branch from 23a2b9a to aa7be5e Compare July 11, 2018 21:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2018

apelisse force-pushed the dry-run branch from aa7be5e to b1ea57b Compare July 11, 2018 22:05

k8s-github-robot added the kind/old-docs label Jul 11, 2018

apelisse force-pushed the dry-run branch 2 times, most recently from 765dc08 to a4b8a7e Compare July 12, 2018 03:36

k8s-github-robot removed the kind/old-docs label Jul 12, 2018

liggitt reviewed Jul 12, 2018

View reviewed changes

Antoine Pelisse added 2 commits July 12, 2018 07:18

dry-run: Create new options for Update/Create and pass it along

03b1e14

dry-run: Run generate commands for new Options types

4bfd5ec

apelisse force-pushed the dry-run branch from a4b8a7e to 4bfd5ec Compare July 12, 2018 14:31

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 12, 2018

k8s-github-robot merged commit fe88461 into kubernetes:master Jul 12, 2018

This was referenced Oct 1, 2018

dry-run: openapi spec missing dryRun query parameter for POST/PUT/PATCH operations #69304

Closed

Add CreateOptions and UpdateOptions to apiserver rest parameter installation #69359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

apelisse commented Jun 14, 2018 •

edited

Loading

lavalamp Jun 18, 2018

deads2k Jun 21, 2018

apelisse Jun 21, 2018

deads2k commented Jun 21, 2018

sttts Jun 22, 2018

sttts Jun 22, 2018

sttts Jun 22, 2018

sttts Jun 22, 2018

sttts Jun 22, 2018

apelisse Jun 25, 2018 •

edited

Loading

sttts Jun 22, 2018 •

edited

Loading

sttts Jun 22, 2018

apelisse Jun 25, 2018

sttts Jun 22, 2018 •

edited

Loading

apelisse Jun 25, 2018

deads2k commented Jul 10, 2018

liggitt Jul 12, 2018

apelisse commented Jul 12, 2018

deads2k commented Jul 12, 2018

liggitt commented Jul 12, 2018

k8s-ci-robot commented Jul 12, 2018

apelisse commented Jul 12, 2018

k8s-github-robot commented Jul 12, 2018

roycaihw commented Oct 1, 2018

dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

Conversation

apelisse commented Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

sttts Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 10, 2018

Choose a reason for hiding this comment

apelisse commented Jul 12, 2018

deads2k commented Jul 12, 2018

liggitt commented Jul 12, 2018

k8s-ci-robot commented Jul 12, 2018

apelisse commented Jul 12, 2018

k8s-github-robot commented Jul 12, 2018

roycaihw commented Oct 1, 2018

apelisse commented Jun 14, 2018 •

edited

Loading

apelisse Jun 25, 2018 •

edited

Loading

sttts Jun 22, 2018 •

edited

Loading

sttts Jun 22, 2018 •

edited

Loading