pilot: use per revision leader for namespace controller #56595
Conversation
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
|
this doesn't seem to make sense. In 99% of the cases they are going to be writing the same configmap to the same namespace which kind of defeats the purpose of the leader election entirely no? Would almost be better off not having leader election at all.. |
it's an edge case from real world, what about enable it when discovery selector is not nil? |
Signed-off-by: zirain <zirain2009@gmail.com>
b11b735 to
3a8ddfe
Compare
TBH, I'm not sure if this will introduce more problems. |
There was a problem hiding this comment.
If you're using discoverySelectors and performing a canary upgrade, there will now be a situation where two different revisions with the same discoverySelectors exist, and they have separate leader elections ie leader election does not do anything.
I agree that we might just want to drop leader election from the NamespaceController. Even with this, you will always be able to construct a combination of revision name and discoverySelector that overlaps, causing problems.
Update: actually, thinking about it, this patch is probably less problematic than the status quo and avoids a big change like dropping leader election, so I'm fine with it
Please provide a description of this PR:
Fixes: #56594
This should be per revision, otherwise multi reversions with
discoverySelectorwon't work as #56594 mentioned.