/ok-to-test

/assign @mikedanese
/assign @lavalamp

@enisoc fyi, sounds serious for 1.9 release

I think we don't want this for the reasons mentioned in the original PR. The issue you mention (#22609) should be fixed by #51698 which we are currently transitioning to. cc @rphillips @ncdc

Lease endpoint reconciler is alpha feature and is not going to be a default reconciler anytime soon, IMHO it cannot be "solution" to this deadlock untils it is default

Original PR mentions hypothetical issues with ordering at the cost of a very real deadlock which at least 3 users experienced, spent time debugging and narrowing down to individual commit and then raising their concerns in that PR comments.

Original PR was merged within a day without any discussion.

Essence of original PR is that answer to following question:

Is having 1/3 of all requests fail for all clients worse then having all requests fail for 1/3 of clients?

was given essentially as "yes, it is better to have all requests of 1/3 users to fail", which is clearly wrong answer, as if you fail all requests for 1/3 of users they have NO CHANCE of recovery, given that there are enough singleton users of kubernetes API this should be obvious.

TLDR; Original PR does more damage than good and should be reverted.

I took some look into things mentioned in this PR. Some of my comments.

IMHO it cannot be "solution" to this deadlock untils it is default

+1

Because each apiserver serves from a cache (unless the linearize read flag is passed explicitly) and each apiserver cache could be a different degree behind etcd.

I don't fully understand this original argument. The requests that are served from apiserver cache are:

• WATCH (but this is be-design eventual consistency, and I'm not convinced that this matters here)
• GET, LIST - but ONLY when you explicitly opt-in for it (by setting ResourceVersion param).

And I really hope that all opt-ins for that were done carefully.

What we should fix is the latency of a bad endpoint being remove from the master service.

This was significantly improved since the this was mentioned. So maybe that's no longer that important now?

as if you fail all requests for 1/3 of users they have NO CHANCE of recovery, given that there are enough singleton users of kubernetes API this should be obvious.

+1

Personally, I have a slight preference for merging it (until we will have the new reconciler), but I don't have that strong opinion. I would like to hear opinions from others - including @lavalamp who LGTM-ed original PR and @bowei because I may be missing something.

The PR in question was merge a year and a half ago so I don't think it can be argued that this is urgent. I'm ok with whatever @wojtek-t and @lavalamp say since they are the experts. However this is a significant change that affects all in cluster clients and I am concerned about invalidating testing this late in a release cycle.

The PR in question was merge a year and a half ago so I don't think it can be argued that this is urgent

+1 - that's why I would like to hear other opinions.
If we will decide that it should be reverted, we may cherrypick it to previous releases if needed.
But that needs to be a conscious decision.

Original PR caused an outage for us. We actioned by manually overriding session affinity on the apiserver service. The original PR breaks HA apiservers in my opinion, and should be reverted.

Now that kubecon is over, any chance it can be looked at?:)

@lavalamp - friendly ping

@lavalamp , can you have a look and add your opinion on this, please?

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale

The justification I gave for LGTMing the original PR is no longer valid, as consistent reads are on by default now. I think I may have also thought that session affinity was implemented by caching and not by static hashing--I am not sure how it is implemented at the moment.

We should stop using the --master-count mechanism ASAP, though, now that we have something better.

My apologies that you had to wait for an additional kubecon before I noticed this :)

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, redbaron

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retest

/test pull-kubernetes-e2e-gce

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

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.