-
Notifications
You must be signed in to change notification settings - Fork 187
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
New proxy server counting logic for agent and server #634
Conversation
Hi @carreter. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: carreter 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 |
@avrittrohwer for visibility |
/ok-to-test |
@carreter: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
@@ -63,6 +64,8 @@ type ClientSet struct { | |||
warnOnChannelLimit bool | |||
|
|||
syncForever bool // Continue syncing (support dynamic server count). | |||
|
|||
respectReceivedServerCount bool // Respect server count received from proxy server rather than relying on the agent's own server counter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should chat on this. During certain events with multiple KASs/Konnectivity servers we can get different server counts from different servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, @avrittrohwer also raised this issue. One potential solution would be to cache the last n received server counts and take the highest value. Ultimately this gets solved by having proxy agents count the leases themselves.
An initial pass at the groundwork for #358 and #273 (i.e. allowing a dynamic proxy server count on both the server and the agent in a backwards-compatible manner). Currently a draft, depends on #632. Design doc here.
Changes
New package:
pkg/servercounter
This package is structured around the
ServerCounter
interface:Currently provided are
StaticServerCounter
, which simply returns a stored int, andCachedServerCounter
, which wraps anotherServerCounter
and provides catching functionality with a configurable cache expiry time.Proxy server
This PR wires the
ServerCounter
into the proxy server without changing its behavior by creating aStaticServerCounter
with the command-line provided count.Proxy agent
This PR wires the
ServerCounter
into the proxy agent without changing its behavior by creating aStaticServerCounter
with a default count of 0.This PR also adds the
respectReceivedServerCount
field to the agentClientSet
struct. This field toggles whether the server count received from the proxy server should overwrite the proxy agent's server counter. Since I didn't want to change the behavior of the agent yet, this is set totrue
by default.Open questions
Should
CountServers()
return anerror
?Sometimes the
ServerCounter
will fail to get an updated server count. In this case, we could haveCountServers()
return(int, error)
instead.I opted against this because we can handle errors in the
ServerCounter
itself. This simplifies things on the caller side and allows for including a fallback server count in theServerCounter
.Should
ServerCounter
include anUpdateCount(int)
method?It might be useful for clients to (a) temporarily override the server count or (b) request an update to any cached server counts. They could do this by calling
UpdateCount()
with a non-negative value to temporarily override the count or with -1 to reset the override and request a count update.I opted against this because the client can accomplish (a) using a
StaticServerCounter
(downside: this overwrites the previousServerCounter
if stored in the same field), and (b) seems like it would have limited use as theServerCounter
should be completely responsible for managing the server count.A possible workaround is to add the
UpdateCount()
method to justCachedServerCounter
rather than the entire interface. This allows for finer control of caching while maintaining a simple API for the general case.Should
respectReceivedServerCount
betrue
orfalse
by default?My take: have it
true
by default unless an alternative server count method is provided in the commandline args to the proxy agent. This ensures backwards compatibility with old proxy servers.