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

Change calico_feature_detect_override default value to ChecksumOffloadBroken=true #9261

Conversation

yankay
Copy link
Member

@yankay yankay commented Sep 8, 2022

What type of PR is this?

/kind documentation

What this PR does / why we need it:

When installed in the Rocky9, there are some bugs with calico and kernel. The network is broken. The kubespray's default configuration needs to provision a healthy cluster.

So I want to recommend to the metion it the calico.md.

The solution is also used by:

Rancher: https://github.com/rancher/rke2-charts/blob/main-source/packages/rke2-calico/generated-changes/patch/values.yaml.patch#L55
Azure: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/templates/addons/calico/patches/calico-node.yaml

So, I suggest setting the default value to the ChecksumOffloadBroken=true, to disable the VXLAN offload by default, so the calico can be worked in most cases.

Which issue(s) this PR fixes:

Fixes #8992

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Disalbe the Calico VXLAN hardware Offload by default with setting `ChecksumOffloadBroken=true`, to work around the issue: https://github.com/projectcalico/calico/issues/4727

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 8, 2022
@yankay
Copy link
Member Author

yankay commented Sep 9, 2022

HI @cristicalin, would you please review it :-) ?

@yankay
Copy link
Member Author

yankay commented Sep 15, 2022

ref to: projectcalico/calico#4727

@oomichi oomichi added the kind/bug Categorizes issue or PR as related to a bug. label Sep 16, 2022
@oomichi
Copy link
Contributor

oomichi commented Sep 16, 2022

Hi @yankay

Thank you for doing this.
After reading the related links, I have a question about this change.

The original issue depends on the combination of Linux kernel version and HW.
In addition, if setting ChecksumOffloadBroken=true on non-problematic environments, that could make performance impact. Is it correct?

Kubespray supports a lot of Linux distributions, so I want to make sure this point before setting it as the default.

@yankay
Copy link
Member Author

yankay commented Sep 16, 2022

HI @oomichi

On the kernel < 5.7, the VXLAN offload default is off. So the performance does not change.
On the kernel >= 5.7(Ubuntu 22.04, RockyLinux 9), The VXLAN offload default is on. So if the performance would be bad on the non-problematic environments, and the network would be broken on vSphere and Azure.

There are some performance data from: https://randomsecurity.dev/posts/vxlan-offload/. The VXLan offload increases the performance.

So I think, the default 'ChecksumOffloadBroken=true' to let the calico can be worked in most environment. If the user wants to tunning , he can set the 'ChecksumOffloadBroken=false' to enable the VXLAN offload (Requires kernel > 5.7 and HW support).

And it's a calico's bug, if calico solves the problem, the kubespray can change back.

@floryut
Copy link
Member

floryut commented Sep 16, 2022

HI @oomichi

On the kernel < 5.7, the VXLAN offload default is off. So the performance does not change. On the kernel >= 5.7(Ubuntu 22.04, RockyLinux 9), The VXLAN offload default is on. So if the performance would be bad on the non-problematic environments, and the network would be broken on vSphere and Azure.

There are some performance data from: https://randomsecurity.dev/posts/vxlan-offload/. The VXLan offload increases the performance.

So I think, the default 'ChecksumOffloadBroken=true' to let the calico can be worked in most environment. If the user wants to tunning , he can set the 'ChecksumOffloadBroken=false' to enable the VXLAN offload (Requires kernel > 5.7 and HW support).

And it's a calico's bug, if calico solves the problem, the kubespray can change back.

That's a good point, if we go this road a big warning in the release note needs to be there.

@yankay yankay force-pushed the change-calico_feature_detect_override-default-value branch from 22e861b to 5e7cef3 Compare September 19, 2022 02:13
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 19, 2022
@yankay
Copy link
Member Author

yankay commented Sep 19, 2022

Thanks @floryut ,
The calico docs have been updated, and add release note comments on the PR.

Would you please review it ?

@floryut
Copy link
Member

floryut commented Sep 22, 2022

Thanks @floryut , The calico docs have been updated, and add release note comments on the PR.

Would you please review it ?

Not sure we should rush this onto 2.20, WDYT @oomichi @cristicalin ?

@cristicalin
Copy link
Contributor

This has the potential to cause some head scratching on folks not affected by the bug so I would actually just document this in the docs and mention situations where it needs to be applied. If we can apply some detection logic it would be better but having kubespray figure all corner cases out of the fox seems like a recipe for complexity and code rot over time.

@yankay
Copy link
Member Author

yankay commented Sep 26, 2022

This has the potential to cause some head scratching on folks not affected by the bug so I would actually just document this in the docs and mention situations where it needs to be applied. If we can apply some detection logic it would be better but having kubespray figure all corner cases out of the fox seems like a recipe for complexity and code rot over time.

HI @cristicalin , thank you for the comments.

@oomichi
Copy link
Contributor

oomichi commented Sep 26, 2022

I hesitate to set the value which could cause performance side-effect on some environments as the default.

It would be easier to find the solution if some features(in this case, calico) don’t work, because the error messages would be output and be helpful.
However it is hard to find the solution if some performance issues happen on such environments.

One more thing is the default IPIP of calico doesn’t work on Azure and we need to specify VXLAN as https://projectcalico.docs.tigera.io/reference/public-cloud/azure
So I feel it is fine to keep ChecksumOffloadBroken=true on the document at this time (at least for Kubespray v2.20).

@yankay yankay force-pushed the change-calico_feature_detect_override-default-value branch from 5e7cef3 to 7c6161b Compare October 13, 2022 02:08
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yankay
Once this PR has been reviewed and has the lgtm label, please assign luckysb for approval by writing /assign @luckysb in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2022
@yankay yankay changed the title Change calico_feature_detect_override default value to ChecksumOffloadBroken=true Update calico.md to metion the ChecksumOffloadBroken=true Oct 13, 2022
@yankay yankay force-pushed the change-calico_feature_detect_override-default-value branch from 7c6161b to 753de23 Compare October 13, 2022 02:13
@yankay yankay force-pushed the change-calico_feature_detect_override-default-value branch from 753de23 to 447f1b7 Compare October 13, 2022 02:16
@yankay yankay closed this Oct 13, 2022
@yankay yankay changed the title Update calico.md to metion the ChecksumOffloadBroken=true Change calico_feature_detect_override default value to ChecksumOffloadBroken=true Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VXLAN: bad UDP checksums
5 participants