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.

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

Bug fixes #1947

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Bug fixes #1947

merged 3 commits into from
Apr 23, 2024

Conversation

thomasferrandiz
Copy link
Contributor

@thomasferrandiz thomasferrandiz commented Apr 17, 2024

  • show content of stdout and stderr when running iptables-restore returns an error
  • fix comparison with previous networks in SetupAndEnsureMasqRules
  • add empty WindowsTrafficManager to make logs clearer

Related issue: #1939

main.go Outdated Show resolved Hide resolved
pkg/trafficmngr/iptables/iptables.go Show resolved Hide resolved
pkg/trafficmngr/iptables/iptables.go Show resolved Hide resolved
main.go Outdated
Comment on lines 565 to 568
if runtime.GOOS == "windows" {
log.Info("Starting flannel in windows mode...")
return &windows.WindowsManager{}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this and creating a new file, why don't you just increase the verbosity required for the ErrUnimplemented error to be logged and return nil? I think this is too much just to make the logs clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is that in this way I never display the "Starting flannel in iptables mode" message which was confusing users.
Also I hoped that it would allow me to delete the iptables_windows.go file but it's still needed to compile on windows.

I was also thinking since we have more requests for flannel on windows there could at some point be something to do in the windows traffic manager even it does't use iptables or nftables.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main problem is that I find it confusing having so many windows files around. Why can't we remove iptables-_windows.go and nftables_windows.go?

I don't think we will ever need Windows doing anything around traffic manager because the forwarding is always there and the natting is also default when creating the overlay network.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove those files then the windows builds fail because all the other files in the package have this constraint:

//go:build !windows
// +build !windows

I have another idea though: instead of having a dedicated windows package which does nothing, I can move the log line inside the traffic manager Init methds so that the log can be different based on the OS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

@thomasferrandiz thomasferrandiz merged commit 66b7dc3 into flannel-io:master Apr 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants