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

Create "Lease" API in the new "coordination.k8s.io" api group #64246

Merged
merged 4 commits into from Jun 27, 2018

Conversation

@wojtek-t
Copy link
Member

wojtek-t commented May 24, 2018

Part of "Efficient Node heartbeats" KEP:
https://github.com/kubernetes/community/blob/master/keps/0009-node-heartbeat.md

Part of: #14733

Create "coordination.k8s.io" api group with "Lease" api in it.

@wojtek-t wojtek-t self-assigned this May 24, 2018

@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao and cjcullen May 24, 2018

@wojtek-t wojtek-t changed the title Create "Lease" object type [WIP] Create "Lease" object type May 24, 2018

@wojtek-t wojtek-t force-pushed the wojtek-t:lease_object_type branch from 0022e67 to 3702402 May 24, 2018

@wojtek-t wojtek-t force-pushed the wojtek-t:lease_object_type branch from 3702402 to 92bd65a May 24, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 24, 2018

/test pull-kubernetes-integration

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 24, 2018

/retest

@wojtek-t wojtek-t removed their assignment May 24, 2018

@wojtek-t wojtek-t changed the title [WIP] Create "Lease" object type Create "Lease" object type May 24, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 24, 2018

@bgrant0607 @dchen1107 @derekwaynecarr @liggitt - I'm not sure if we want to merge it with 1.11 timeframe, but if so, the PR adding the "Lease" API is essentially ready for review.

@wojtek-t wojtek-t changed the title Create "Lease" object type Create "Lease" API in the new "coordination.k8s.io" api group May 24, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 26, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@bgrant0607 @dchen1107 @derekwaynecarr @liggitt @mtaufen @wojtek-t

Pull Request Labels
  • sig/architecture sig/cluster-lifecycle sig/node sig/scalability sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help

// ValidateLease validates a Lease.
func ValidateLease(lease *coordination.Lease) field.ErrorList {
allErrs := ValidateLeaseSpec(&lease.Spec, field.NewPath("spec"))

This comment has been minimized.

@liggitt

liggitt Jun 26, 2018

Member

what are the name restrictions around Lease objects? I expected a call to ValidateObjectMeta here with a name validator

This comment has been minimized.

@mtaufen

mtaufen Jun 26, 2018

Contributor

We don't have a step that does generic ObjectMeta validation?
Edit: I guess not... good to know.

This comment has been minimized.

@liggitt

liggitt Jun 26, 2018

Member

it does generic objectmeta validation with backstop restrictions on object names (disallows / and % characters, disallows . and .. names), but I'm assuming we want something more restrictive than that here

This comment has been minimized.

@wojtek-t

wojtek-t Jun 27, 2018

Author Member

@liggitt So given that we do generic ObjectMetadata validation in generic code (in staging/src/k8s.io/apiserver/pkg/registry/rest/), what other validation you would like to see here?

I'm not sure we would like to be more restrictive here.

This comment has been minimized.

@wojtek-t

wojtek-t Jun 27, 2018

Author Member

Discussed offline with Jordan and added ValidateObjectMeta call. Hopefully that's all we need.


// AllowCreateOnUpdate is true for Lease; this means you may create one with a PUT request.
func (leaseStrategy) AllowCreateOnUpdate() bool {
return false

This comment has been minimized.

@liggitt

liggitt Jun 26, 2018

Member

this seems like an object it would be useful to allow this on... it would let us grant named create/update permissions on specific lease instances. as long as we don't allow unconditional update (which it looks like we don't), create-on-update is something I'd want on this resource

This comment has been minimized.

@mtaufen

mtaufen Jun 26, 2018

Contributor

Also the godoc doesn't match the implementation here.

This comment has been minimized.

@wojtek-t

wojtek-t Jun 27, 2018

Author Member

Thanks for pointing on that Jordan - I completely agree we should allow for that (and after change godoc is finally correct :))


// AllowUnconditionalUpdate is the default update policy for Lease objects.
func (leaseStrategy) AllowUnconditionalUpdate() bool {
return false

This comment has been minimized.

@liggitt

liggitt Jun 26, 2018

Member

+1 for this

@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented Jun 26, 2018

/lgtm cancel
so we can address @liggitt's comments

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

@wojtek-t wojtek-t force-pushed the wojtek-t:lease_object_type branch from de84b51 to 134ce7b Jun 27, 2018

wojtek-t added some commits May 22, 2018

@wojtek-t wojtek-t force-pushed the wojtek-t:lease_object_type branch from 134ce7b to 0950084 Jun 27, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented Jun 27, 2018

PTAL

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 27, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 27, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 27, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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

k8s-github-robot commented Jun 27, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 27, 2018

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

@k8s-github-robot k8s-github-robot merged commit 6d3bba7 into kubernetes:master Jun 27, 2018

12 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation wojtek-t authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross 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-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details

@wojtek-t wojtek-t deleted the wojtek-t:lease_object_type branch Jul 2, 2018

@zparnold

This comment has been minimized.

Copy link
Member

zparnold commented Jul 16, 2018

Hey there @wojtek-t, I was reading through this PR and associated KEP (I'm the Docs 1.12 release lead) do you think this is something that would need documentation around it? I was thinking it could be something mentioned in the large cluster section of the docs. https://kubernetes.io/docs/setup/cluster-large/ What do you think?

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented Jul 16, 2018

Hey there @wojtek-t, I was reading through this PR and associated KEP (I'm the Docs 1.12 release lead) do you think this is something that would need documentation around it? I was thinking it could be something mentioned in the large cluster section of the docs. https://kubernetes.io/docs/setup/cluster-large/ What do you think?

Yes - it requires release note. Though, it's not large-cluster specific. The change was motivated by large clusters, but it will touch clusters of all sizes. So it should be in some more generic place (though I don't know where exactly).
I'm leaving for paternity leave within days (hours?) so @mtaufen will be able to assist you with that.

@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented Jul 16, 2018

@zparnold, I'm working on the node heartbeats side of this, so let's definitely stay in touch regarding docs and where to put them.

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Sep 12, 2018

This should have a release note.

@wojtek-t Can you edit the main PR body to include the release note? Thanks!

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.