resync status on apiservices for aggregator #55165
Conversation
|
@deads2k: Adding do-not-merge/release-note-label-needed because the release note process has not been followed. One of the following labels is required "release-note", "release-note-action-required", or "release-note-none". 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. |
k8s-ci-robot
added
do-not-merge/release-note-label-needed
| UpdateFunc: c.updateAPIService, | ||
| DeleteFunc: c.deleteAPIService, | ||
| }, | ||
| 30*time.Second) |
There was a problem hiding this comment.
How many threads does the controller have? Each will be blocked for 30 seconds in the worst-case.
There was a problem hiding this comment.
How many threads does the controller have? Each will be blocked for 30 seconds in the worst-case.
Each will be blocked at most 6 seconds, right? We have a time.After block. I've updated the code to start 5.
We don't back up infinitely since the queue.Add is fair (first in, first out) and eliminates duplicates. I can make it more if you like.
There was a problem hiding this comment.
Sounds good now. If we handle a handful external apiservers, it's ok.
deads2k
force-pushed
the
agg-01-resync
branch
from
2c287b1
to
145f041
Compare
| @@ -259,9 +265,9 @@ func (c *AvailableConditionController) Run(stopCh <-chan struct{}) { | |||
| return | |||
| } | |||
|
|
|||
| // only start one worker thread since its a slow moving API and the aggregation server adding bits | |||
There was a problem hiding this comment.
I think this comment was copy/pasted from the controller that tickles the proxy handler. I didn't see any links out from here that looked dangerous.
|
should avoid constructing a new client for every sync check if we're going to do it this often. there's nothing service or request-specific, so we should share to take advantage of open keep-alive requests if the IP doesn't change and there's already an open and valid connection |
deads2k
force-pushed
the
agg-01-resync
branch
from
145f041
to
14a0b0b
Compare
k8s-ci-robot
added
size/M
|
now re-using one client. |
deads2k
added
release-note-none
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k Associated issue requirement bypassed by: deads2k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
| @@ -248,7 +255,7 @@ func (c *AvailableConditionController) sync(key string) error { | |||
| return err | |||
| } | |||
|
|
|||
| func (c *AvailableConditionController) Run(stopCh <-chan struct{}) { | |||
| func (c *AvailableConditionController) Run(workers int, stopCh <-chan struct{}) { | |||
There was a problem hiding this comment.
nit: it's called threadiness elsewhere
| @@ -221,7 +221,7 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg | |||
| return nil | |||
| }) | |||
| s.GenericAPIServer.AddPostStartHook("apiservice-status-available-controller", func(context genericapiserver.PostStartHookContext) error { | |||
| go availableController.Run(context.StopCh) | |||
| go availableController.Run(5, context.StopCh) | |||
There was a problem hiding this comment.
maybe a comment why 5 is the perfect number here?
|
lgtm. Two nits. Tag when ready. |
deads2k
force-pushed
the
agg-01-resync
branch
from
14a0b0b
to
b7bc9b1
Compare
deads2k
added
the
lgtm
| // construct an http client that will ignore TLS verification (if someone owns the network and messes with your status | ||
| // that's not so bad) and sets a very short timeout. | ||
| discoveryClient := &http.Client{ | ||
| Transport: &http.Transport{ |
There was a problem hiding this comment.
pre-existing, but call SetTransportDefaults on this?
|
Automatic merge from submit-queue (batch tested with PRs 55403, 54660, 55165). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Adds a fairly tight (30 second) resync on the apiservices to force redetection of status. The checks aren't very expensive and there are relatively few apiservices. Taking a little resync pain here is cheaper than the fallout for all clients.