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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Jun 27, 2018
Merged

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

merged 4 commits into from Jun 27, 2018

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented May 24, 2018 •
edited

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 added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 24, 2018
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels May 24, 2018
@wojtek-t wojtek-t changed the title Create "Lease" object type [WIP] Create "Lease" object type May 24, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2018
@wojtek-t wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 24, 2018
@wojtek-t
Copy link
Member Author

/test pull-kubernetes-integration

@wojtek-t
Copy link
Member Author

/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
@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels May 24, 2018
@wojtek-t
Copy link
Member Author

@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
@wojtek-t wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 24, 2018
@k8s-github-robot
Copy link

[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

liggitt
liggitt reviewed Jun 26, 2018
View reviewed changes
pkg/apis/coordination/validation/validation.go Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mtaufen mtaufen Jun 26, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

liggitt
liggitt reviewed Jun 26, 2018
View reviewed changes
pkg/registry/coordination/lease/strategy.go Outdated

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

@liggitt liggitt Jun 26, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the godoc doesn't match the implementation here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

liggitt
liggitt reviewed Jun 26, 2018
View reviewed changes
pkg/registry/coordination/lease/strategy.go

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this

@mtaufen
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 "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2018
@wojtek-t
Copy link
Member Author

PTAL

@liggitt
Copy link
Member

liggitt commented Jun 27, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2018
@k8s-ci-robot
Copy link
Contributor

[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
Copy link

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

@k8s-github-robot
Copy link

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
@wojtek-t wojtek-t deleted the lease_object_type branch July 2, 2018 08:29
@zparnold
Copy link
Member

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
Copy link
Member Author

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
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.

@mtaufen mtaufen mentioned this pull request Jul 17, 2018
Closed
6 tasks
@liggitt liggitt mentioned this pull request Aug 8, 2018
Closed
@kdomanski kdomanski mentioned this pull request Aug 8, 2018
Closed
@nikhita
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!

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 13, 2018
mars1024 pushed a commit to mars1024/kubernetes that referenced this pull request Nov 26, 2019
@furykerry
Merge branch 'develop-tmp-zhuling' into 'develop-tmp'
33cf2d4 
Make periodic NodeStatus updates cheaper

cherry-pick 
master, create lease api, kubernetes#64246
master, node lifecycle controller, kubernetes#69241
kubelet, kubernetes#66257

See merge request !184866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaufen mtaufen mtaufen left review comments

@liggitt liggitt liggitt left review comments

@apelisse apelisse apelisse left review comments

@bgrant0607 bgrant0607 bgrant0607 left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@cjcullen cjcullen Awaiting requested review from cjcullen

Assignees

@mtaufen mtaufen

@liggitt liggitt

@derekwaynecarr derekwaynecarr

@bgrant0607 bgrant0607

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

None yet