Ensure protokube can connect to kube-apiserver before starting the sync loop #11093
Conversation
k8s-ci-robot
added
cncf-cla: yes
protokube/pkg/protokube/kube_boot.go
Outdated
| for { | ||
| client, err := k.Kubernetes.KubernetesClient() | ||
| if err != nil { | ||
| klog.Warningf("could not create kubernetes client: %v", err) |
There was a problem hiding this comment.
This one probably needs a sleep and continue? Or klog.Fatalf? I think we'll panic on the client.CoreV1() call anyway..
There was a problem hiding this comment.
We may as well panic here. There isn't much we can do if this one fails.
protokube/pkg/protokube/kube_boot.go
Outdated
| klog.Info("connected to the apiserver") | ||
| break | ||
| } | ||
| klog.Infof("Could not connect to apiserver: %v", err) |
There was a problem hiding this comment.
Nit: something to indicate that this is normal(ish) might be helpful here. e.g. "polling for apiserver readiness; not yet ready: %v" or something like that?
|
This is a nice win. LGTM; one error handling comment and one nit. /approve /hold Feel free to remove the hold if you don't think the nits are worth it :-) |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
lgtm
…nc loop This commit will have protokube eagerly retry to connect to kube-apiserver instead of the 1 minute sync loop. Another benefit is that we know that tasks are run in order and that labels are always applied before channels etc
olemarkus
force-pushed
the
protokube-eager-connect
branch
from
cbf5ac1 to
cde6679
Compare
k8s-ci-robot
removed
the
lgtm
|
FWIW, Looking at the protokube logs from the first commit's kubernetes-aws and k8s-containerd jobs it seems we retried 19 and 22 times respectively. We'll see how the most recent jobs perform as well. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Looks good, going to remove the hold (I had meant it to mean "feel free to remove hold when you want it to merge") /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
…093-origin-release-1.20 Automated cherry pick of #11093: Ensure protokube can connect to kube-apiserver before
This commit will have protokube eagerly retry to connect to kube-apiserver instead of the 1 minute sync loop.
Another benefit is that we know that tasks are run in order and that labels are always applied before channels etc
With very few samples, it looks like a cluster gets the DNS entry about 1-2 minutes earlier, which I think makes sense.
Ref #11050