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

migrate leader election to lease API #80508

Closed

Conversation

@ricky1993
Copy link
Contributor

ricky1993 commented Jul 24, 2019

Change-Id: I2e22c578408798bc1a52941898f92e15a216afcd

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Ref #80289

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 24, 2019

Hi @ricky1993. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Jul 24, 2019

/ok-to-test

@ricky1993 ricky1993 force-pushed the ricky1993:github/migrate_leader_election branch from 3747674 to 8736a44 Jul 24, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 24, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ricky1993
To complete the pull request process, please assign mikedanese
You can assign the PR to them by writing /assign @mikedanese in a comment when ready.

The full list of commands accepted by this bot can be found 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

@ricky1993

This comment has been minimized.

Copy link
Contributor Author

ricky1993 commented Jul 24, 2019

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-e2e-gce-100-performance

if err != nil {
return nil, err
}
// check if the older resource lock is released

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Jul 24, 2019

Member

Can you describe the logic behind this in more details - I think I'm not really following this.

This comment has been minimized.

Copy link
@ricky1993

ricky1993 Jul 24, 2019

Author Contributor

The main reason here is not to destroy the original interface, only return a resource lock, we want to panic at any time of the lock release. We should check if the earlier lock is expired. (Here should be renew time instead of acquire time, I will fix it tomorrow)

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Jul 24, 2019

Changed to "ref" the issue from "fix" it - we would need to clean that up in future release.

@@ -22,6 +22,7 @@ import (
"fmt"

"k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Jul 24, 2019

Member

I'm wondering what the approach you use here means for clients that are not in k/k repo, but are vendoring this package.
I like the overall idea you did, but we need to clearly describe consequences:

  • what are the next steps for us (to migrate things like scheduler, controller-manager to use just lease)
  • what are the steps we suggest all clients vendoring that should do
  • what changes we will do in this directory (and how it affects the above)

This comment has been minimized.

Copy link
@ricky1993

ricky1993 Jul 24, 2019

Author Contributor

There are two ideas in my mind about the next step.

  • The first idea is to inject the lease into the resource lock without breaking the original interface, and the client does not perceive the process of switching.
    Switch it to lease on the next release, but worry that someone will upgrade the version that skips this migration (required in the release note)
  • Another option is for the client to explicitly control the migration process through the flag.

I am implementing the former here, but I am not sure which one is better for us. We can continue to discuss this program in depth.

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Jul 25, 2019

Member

The first idea is to inject the lease into the resource lock without breaking the original interface, and the client does not perceive the process of switching.

That's not an option, because we don't control all users of the interface.

Another option is for the client to explicitly control the migration process through the flag.

That sounds like the only reasonable one to me.

Change-Id: I2e22c578408798bc1a52941898f92e15a216afcd
@ricky1993 ricky1993 force-pushed the ricky1993:github/migrate_leader_election branch from 8736a44 to 01cd380 Jul 24, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 24, 2019

@ricky1993: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 01cd380 link /test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-device-plugin-gpu 01cd380 link /test pull-kubernetes-e2e-gce-device-plugin-gpu
pull-kubernetes-kubemark-e2e-gce-big 01cd380 link /test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-e2e-gce-100-performance 01cd380 link /test pull-kubernetes-e2e-gce-100-performance

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Member

timothysc left a comment

I'm very confused by why you would want/need this PR...? Please outline the details in your PR message.

@@ -56,6 +58,16 @@ func (cml *ConfigMapLock) Get() (*LeaderElectionRecord, error) {
return nil, err
}
}
if cml.LeaseLock != nil {
leaseLock, err := cml.LeaseLock.Get()

This comment has been minimized.

Copy link
@timothysc

timothysc Jul 24, 2019

Member

Why is this code here? iirc this exists in the leader election code outside the resourcelock abstraction.

/hold

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Jul 24, 2019

You need to migrate callers of https://godoc.org/k8s.io/client-go/tools/leaderelection#NewLeaderElector . You can't fix the linked issue by modifying existing resource lock implementations. I suggest either using two leader election clients to implement the migration from one lock config to another lock config, or if that isn't feasible, implement a composite resource lock that is composed of two other resource locks.

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Jul 25, 2019

Given https://github.com/kubernetes/kubernetes/pull/80508/files#r307284991 and what @mikedanese wrote above (which I agree with) I think we should close this PR and fix that directly in clients of that library we control.

@roycaihw

This comment has been minimized.

Copy link
Member

roycaihw commented Jul 25, 2019

/cc @cheftako

@k8s-ci-robot k8s-ci-robot requested a review from cheftako Jul 25, 2019
@roycaihw

This comment has been minimized.

Copy link
Member

roycaihw commented Jul 25, 2019

/cc @deads2k

@k8s-ci-robot k8s-ci-robot requested a review from deads2k Jul 25, 2019
@ricky1993

This comment has been minimized.

Copy link
Contributor Author

ricky1993 commented Jul 26, 2019

/close

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 26, 2019

@ricky1993: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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.