-
Notifications
You must be signed in to change notification settings - Fork 275
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
Retry flanneld if it fails to start #5703
Conversation
logrus.Errorf("Flanneld has an error: %v. Check %s for extra information", err, logPath) | ||
// We retry running Flanneld 5 times before giving up | ||
maxretries := 5 | ||
for i:=0; i < maxretries; i++ { |
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.
In flannel we use https://github.com/avast/retry-go/ for retries.
I don't know if it would be worth it here?
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.
Thanks for the suggestion Thomas! For this use case, I think it is fine with a simple loop. No need to carry another dependency
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.
What specifically are we trying to fix by retrying here? All other prerequisites are polled for readiness before attempting to start the component. Other components that are started in a retry loop don't have a limit on them and are just retried indefinitely while RKE2 is up.
I feel like we should figure out what is causing flannel to fail initially, and poll for that to become come available before starting flannel, rather than artificially limiting the number of restarts.
If we do for some reason need to artificially limit the restarts, shouldn't we logrus.Fatalf
after the for loop, rather than just leaving rke2 running without any further attempt to start the CNI?
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.
The first time flanneld runs, it creates the HNS network, which is an overlay flannel network bound with a physical interface. Because of how windows works, that interface then becomes unavailable for some time (according to my measurements from 10s - 25s). If it is closer to the 25s, flanneld dies because it failed to contact kube-api for too long. That's the case that we are trying to fix here. The 2nd time it runs, if the interface is back, as the network is already there, it does not create it, so there is no downtime for the connections
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.
One thing we could is: run flannel once and then, if it fails, check for the interface to come back. Once we are sure the interface is back, run flannel again. If it fails a second time, then do the fatal failure. What do you think?
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.
Does flannel have an option to do a one-shot startup that will just create the HNS network if necessary, and then exit? We could do that, wait for the network to stabilize, and then start the "permanent" flannel that we expect to not crash?
That's pretty similar to what you were suggesting I guess, except that we would expect flannel to always exit the first time.
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.
Unfortunately that option does not exist. I have an idea I could try tomorrow in Flannel, hopefully before code freeze
Signed-off-by: Manuel Buil <[email protected]>
dcf1b76
to
4d093c5
Compare
logrus.Errorf("Flanneld has an error: %v. Check %s for extra information", err, logPath) | ||
// We retry running Flanneld 5 times before giving up | ||
maxretries := 5 | ||
for i:=0; i < maxretries; i++ { |
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.
What specifically are we trying to fix by retrying here? All other prerequisites are polled for readiness before attempting to start the component. Other components that are started in a retry loop don't have a limit on them and are just retried indefinitely while RKE2 is up.
I feel like we should figure out what is causing flannel to fail initially, and poll for that to become come available before starting flannel, rather than artificially limiting the number of restarts.
If we do for some reason need to artificially limit the restarts, shouldn't we logrus.Fatalf
after the for loop, rather than just leaving rke2 running without any further attempt to start the CNI?
Trying this instead #5747, hopefully solving the problem. Closing for the moment |
Proposed Changes
This PR does two things:
1 - Slightly changes one log message because it was raising alarms in our customers/support even though it was harmless:
Flanneld can't start because it can't find node
==>Checking if node %s is already registered before starting flanneld
2 - Retrying executing flanneld if it failed. We try 5 times and give up if it always fails. We should retry at least 1 or 2 times, in the case of a windows node with only one interface as explained in the issue. 5 seems like a good number to give up
Types of Changes
Bugfix
Verification
The new log is easy to verify. Just check the rke2 logs
For the second part, make sure everything is running. Then execute:
Stop-Process -Name flanneld
. Then verify that the rke2 logs include:Testing
Linked Issues
#5702
User-Facing Change
Further Comments