-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
make sure worker nodes actually join control plane on restart #9476
make sure worker nodes actually join control plane on restart #9476
Conversation
} else { | ||
return errors.Wrapf(err, "cmd failed: %s\n%+v\n", joinCmd, out.Output()) | ||
} | ||
return errors.Wrapf(err, "cmd failed: %s\n%+v\n", joinCmd, out.Output()) |
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.
I do the same check for the error message like before but this time retry and also let user know we are retrying
already exists in the cluster. You must delete the existing Node or change the name of this new joining Node
if that error is there, we will klog.Info (so the IDE users who usually dont like to wait without any logs, know we are retrying
if rr.Output() has that ^^ error, {
klog.Info("retrying to re-register worker node to the controlplane.... ")
}
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.
also, RunCmd already includes the rr.Output in the error message,
return errors.Wrapf(err, "join cmd") is enough
} else { | ||
return errors.Wrapf(err, "cmd failed: %s\n%+v\n", joinCmd, out.Output()) | ||
} | ||
return errors.Wrapf(err, "cmd failed: %s\n%+v\n", joinCmd, out.Output()) |
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.
also, RunCmd already includes the rr.Output in the error message,
return errors.Wrapf(err, "join cmd") is enough
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.
lets also remove the manual time.Sleep in our test
minikube should wait enough for the kubelet thing itself so users dont have to do that.
https://github.com/medyagh/minikube/blob/aad43a2e43317540ef6167c17715a4dd449bcbaa/test/integration/multinode_test.go#L231
/ok-to-test |
kvm2 Driver Times for Minikube (PR 9476): 60.3s 62.0s 59.8s Averages Time Per Log
docker Driver Times for Minikube (PR 9476): 28.5s 31.1s 31.5s Averages Time Per Log
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh, sharifelgamal 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 |
Codecov Report
@@ Coverage Diff @@
## master #9476 +/- ##
=======================================
Coverage 29.05% 29.05%
=======================================
Files 171 171
Lines 10443 10443
=======================================
Hits 3034 3034
Misses 6987 6987
Partials 422 422 |
kvm2 Driver Times for Minikube (PR 9476): 59.6s 60.7s 61.3s Averages Time Per Log
docker Driver Times for Minikube (PR 9476): 29.4s 29.9s 28.9s Averages Time Per Log
|
This should fix #9456