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 kcm and scheduler to lock on ConfigMaps. #45739

Merged
merged 3 commits into from
May 17, 2017

Conversation

timothysc
Copy link
Member

@timothysc timothysc commented May 12, 2017

What this PR does / why we need it:
Plumbs through the ability to lock on ConfigMaps through the kcm and scheduler.

Which issue this PR fixes
Fixes: #44857
Addresses issues with: #45415

Special notes for your reviewer:

Release note:

Add leader-election-resource-lock support to kcm and scheduler to allow for locking on ConfigMaps as well as Endpoints(default) 

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews @jamiehannaford @bsalamat @mikedanese

@timothysc timothysc added area/HA kind/enhancement sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 12, 2017
@timothysc timothysc added this to the v1.7 milestone May 12, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 12, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 12, 2017
Copy link
Contributor

@jamiehannaford jamiehannaford left a comment

Choose a reason for hiding this comment

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

Is it also possible to cherry pick my commit and add in your new fn signature? It's very similar to what I originally submitted :)

@@ -69,3 +74,29 @@ type Interface interface {
// into a string
Describe() string
}

// Manufacture will create a lock of a given type according to the input parameters
func Manufacture(lockType string, ns string, name string, client *cs.Clientset, rlc ResourceLockConfig) (Interface, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ns, name string

Copy link
Member

Choose a reason for hiding this comment

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

Why not just call this New? What is the significance of Manufacture in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

New works for me.

@timothysc
Copy link
Member Author

timothysc commented May 12, 2017

Is it also possible to cherry pick my commit and add in your new fn signature? It's very similar to what I originally submitted

Yes, Done.

@@ -814,6 +814,7 @@ func autoConvert_v1alpha1_LeaderElectionConfiguration_To_componentconfig_LeaderE
out.LeaseDuration = in.LeaseDuration
out.RenewDeadline = in.RenewDeadline
out.RetryPeriod = in.RetryPeriod
out.LockType = in.LockType
Copy link
Member

Choose a reason for hiding this comment

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

Technically, "endpoints" and "configmaps" are Kinds in our API. Types also include a version.

Copy link
Member Author

@timothysc timothysc May 12, 2017

Choose a reason for hiding this comment

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

I'm fine with changing the name.

  • LockKind (sounds weird)
  • ResourceLock
  • LockType

... I don't really care. Do you have preferences @mikedanese ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer LockResource. It also needs to be a group and resource, not just one or the other. You probably don't want to perform discovery, so I'd recommend doing LockResource and LockResourceGroup or similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

endpoints is a resource (addressable via the API) and is what most external components should use. Kind (Endpoints) is a schema, and multiple resources can have that schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're gradually phasing out use of GroupVersionKind in internal code, but it will take a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@timothysc timothysc May 15, 2017

Choose a reason for hiding this comment

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

I'll change the name to LockResource.

Re: grouping, that's done a layer above and passed into the Manufacture/New function, which keeps the library generic enough for general reuse.

That said, if we want to build it around the api w/Group, I think we could follow on with a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

LockResource SGTM

@timothysc
Copy link
Member Author

@k8s-bot gce etcd3 e2e test this

@timothysc
Copy link
Member Author

@mikedanese I believe I've addressed the comments, PTAL.

@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikedanese, timothysc
We suggest the following additional approvers: davidopp, pwittrock

Assign the PR to them by writing /assign @davidopp @pwittrock in a comment when ready.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@timothysc timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2017
@ravilr
Copy link
Contributor

ravilr commented May 15, 2017

@timothysc @deads2k the default system:.. rbac bootstrap policies for controller-manager and scheduler also needs update to allow create/update/delete on configmap resource here?

@timothysc
Copy link
Member Author

@ravilr - Do we not have RBAC on any of our e2es?

@mikedanese
Copy link
Member

@timothysc
Copy link
Member Author

timothysc commented May 15, 2017

Meh, I'll fix up this PR.

@timothysc timothysc added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2017
@timothysc
Copy link
Member Author

So the other change hits a different space, so I'm going to push this one through and follow up with a RBAC PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45374, 44537, 45739, 44474, 45888)

@k8s-github-robot k8s-github-robot merged commit 7f92d35 into kubernetes:master May 17, 2017
k8s-github-robot pushed a commit that referenced this pull request May 25, 2017
Automatic merge from submit-queue (batch tested with PRs 45269, 46219, 45966)

Update RBAC policy for configmap locked leader leasing.

**What this PR does / why we need it**:
Updates the bootstrap policy to allow for configmap get/update/list/watch for leader leasing. 

**Which issue this PR fixes** 
Follow on PR from: #45739

xref: #44857

**Special notes for your reviewer**:

**Release note**:

```
NONE
```

/cc @kubernetes/sig-auth-pr-reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/HA 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: Option allow components(sched+cm) to lock on ConfigMaps on startup
7 participants