Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Retry flanneld if it fails to start #5703

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions pkg/windows/flannel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,9 @@ func (f *Flannel) Start(ctx context.Context) error {

// Wait for the node to be registered in the cluster
if err := wait.PollImmediateWithContext(ctx, 3*time.Second, 5*time.Minute, func(ctx context.Context) (bool, error) {
logrus.Infof("Checking if node %s is already registered before starting flanneld", f.CNICfg.Hostname)
_, err := f.KubeClient.CoreV1().Nodes().Get(ctx, f.CNICfg.Hostname, metav1.GetOptions{})
if err != nil {
logrus.WithError(err).Warningf("Flanneld can't start because it can't find node, retrying %s", f.CNICfg.Hostname)
return false, nil
} else {
logrus.Infof("Node %s registered. Flanneld can start", f.CNICfg.Hostname)
Expand Down Expand Up @@ -293,15 +293,25 @@ func startFlannel(ctx context.Context, config *FlannelConfig, logPath string) {
fmt.Sprintf("--net-config-path=%s", filepath.Join(config.ConfigPath, FlanneldConfigName)),
}

logrus.Infof("Flanneld Envs: %s and args: %v", specificEnvs, args)
cmd := exec.CommandContext(ctx, "flanneld.exe", args...)
cmd.Env = append(specificEnvs)
cmd.Stdout = outputFile
cmd.Stderr = outputFile
if err := cmd.Run(); err != nil {
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++ {
Copy link
Contributor

@thomasferrandiz thomasferrandiz Apr 11, 2024 •
edited
Loading

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?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

@manuelbuil manuelbuil Apr 11, 2024 •
edited
Loading

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

Copy link
Contributor Author

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?

Copy link
Member

@brandond brandond Apr 11, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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

logrus.Infof("Running flanneld with envs: %s and args: %v", specificEnvs, args)
cmd := exec.CommandContext(ctx, "flanneld.exe", args...)
cmd.Env = append(specificEnvs)
cmd.Stdout = outputFile
cmd.Stderr = outputFile
if err := cmd.Run(); err != nil {
if errors.Is(err, context.Canceled) {
logrus.Error("Context was canceled. Not retrying flanneld")
break
}
logrus.Errorf("Flanneld has an error: %v. Check %s for extra information", err, logPath)
}
if i < (maxretries - 1) {
logrus.Error("Flanneld exited. Retrying.")
}
}
logrus.Error("Flanneld exited")
}

// ReserveSourceVip reserves an IP that will be used as source VIP by kube-proxy. It uses host-local CNI plugin to reserve the IP
Expand Down
Loading