-
Notifications
You must be signed in to change notification settings - Fork 701
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
[Failing Test] kubeadm-kinder-super-admin-latest keeps failing for timeout #2995
Comments
A local test shows that this is caused by kubernetes/kubernetes#122529 @neolit123 @SataQiu |
where is this error happening? |
we should stop using exp backoff in kubeadm.
500ms / 1min is 120 retries |
I tried to fix in kubernetes/kubernetes#122802. I tested locally and it works.
|
i will try locally as well. but it's surprising that locally it fails with a 1m timeout. |
I have test the backoff mode, it takes about ~320ms. func TestBackoff(t *testing.T) {
backoff := wait.Backoff{
Steps: 4,
Duration: 10 * time.Millisecond,
Factor: 5.0,
Jitter: 0.1,
}
start := time.Now()
err := wait.ExponentialBackoff(backoff, func() (bool, error) {
return false, nil
})
t.Logf("elapsed time: %v", time.Since(start))
if err != nil {
t.Error(err)
}
} |
~320ms Steps
|
is the error actually due to timeout? |
We have a 5 minutes timeout for each step. pre-init will trigger 4 renew and other steps. kubeadm/kinder/ci/workflows/super-admin-tasks.yaml Lines 80 to 88 in 4598c42
The timeout happened in the forth renew which is almost 5minutes. |
yes, but these certs renew calls should complete right away. they do not block the execution for 1 minute each. |
my local test is one minute each |
i understand what the problem is now:
the way some parts of kubeadm are designed is:
in such conditions we should pass a shorter timeout to kubeadm, but not use a expbackoff. |
Adding another short timeout would be a better solution. I roughly revert that part😄 which may be not a final solution. |
yeah, like i mentioned a few times we should no longer use exp backoff in kubeadm (just for consistency) |
@pacoxu please LGTM your PR is correct, but i used Poll* for consistency instead. |
https://testgrid.k8s.io/sig-cluster-lifecycle-kubeadm#kubeadm-kinder-super-admin-latest
yesterday, after merging kubernetes/kubernetes#122735 and kubernetes/kubernetes#122529.
The text was updated successfully, but these errors were encountered: