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

Make disabling TX checksum for Antrea gateway #3957

Closed
wants to merge 1 commit into from

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Jun 30, 2022

This is a supplement to PR #3832. When disableTXChecksumOffload
is true, TX checksum offload of Antrea gateway should be also
disabled, otherwise for the cases in which the datapath doesn't
support TX checksum offloading, packets sent from Antrea gateway
could be dropped due to bad checksum.

Note that, when changing disableTXChecksumOffload from true back
to false, TX checksum offload of Antrea gateway will not be enabled
automatically, and TX checksum offload can be enabled manually with
ethtool. Another way is to remove Antrea gateway interface before
updating disableTXChecksumOffload.

Signed-off-by: Hongliang Liu [email protected]

@hongliangl hongliangl requested review from tnqn and antoninbas June 30, 2022 14:40
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2022

Codecov Report

Merging #3957 (4454d71) into main (0f77529) will increase coverage by 0.01%.
The diff coverage is 71.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3957      +/-   ##
==========================================
+ Coverage   62.26%   62.27%   +0.01%     
==========================================
  Files         385      361      -24     
  Lines       54501    52244    -2257     
==========================================
- Hits        33933    32536    -1397     
+ Misses      18069    17275     -794     
+ Partials     2499     2433      -66     
Flag Coverage Δ
integration-tests 34.94% <ø> (+0.09%) ⬆️
kind-e2e-tests 42.41% <76.66%> (-5.76%) ⬇️
unit-tests 45.03% <0.00%> (+1.17%) ⬆️
Impacted Files Coverage Δ
cmd/antrea-agent/agent.go 0.00% <0.00%> (ø)
pkg/agent/agent_linux.go 5.42% <33.33%> (+0.77%) ⬆️
pkg/agent/agent.go 50.45% <87.50%> (-0.58%) ⬇️
pkg/agent/proxy/endpointslicecache.go 0.00% <0.00%> (-83.60%) ⬇️
pkg/features/antrea_features.go 8.00% <0.00%> (-52.00%) ⬇️
...g/agent/controller/serviceexternalip/controller.go 35.83% <0.00%> (-45.74%) ⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go 29.62% <0.00%> (-22.23%) ⬇️
cmd/antrea-agent/options.go 0.00% <0.00%> (-21.23%) ⬇️
pkg/agent/util/net.go 33.33% <0.00%> (-20.23%) ⬇️
pkg/agent/flowexporter/connections/conntrack.go 75.75% <0.00%> (-18.19%) ⬇️
... and 80 more

return fmt.Errorf("error when disabling TX checksum offload on %s: %v", i.hostGateway, err)
}
} else {
if err := ethtool.EthtoolTXHWCsumSwitch(i.hostGateway, ethtool.TXCSUM_ON); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if tx checksum is always turned on by default on linux platforms and always configurable?
If not, this change would affect cases in which tx checksum was off, but the default value "disabledTXChecksumOffload=false" would try to turn it on.

Copy link
Contributor Author

@hongliangl hongliangl Jul 1, 2022

Choose a reason for hiding this comment

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

I have investigated about TX Checksum Offload and found that TX Checksum Offload is not always supported since the driver of a NIC may not supports that. I think TX Checksum Offload is turned on default on Linux if the driver of a NIC supports TX Checksum Offload.

Maybe we can remove this since TX Checksum Offload cannot be set to on in some environment. However, I don't have a good idea how to deal with the case:

A user sets disabledTXChecksumOffload=true, then sets it back to false, but TX Checksum Offload of Antrea gateway is still off, maybe we could log something like "If you set disabledTXChecksumOffload=false back again and you should set TX Checksum Offload of Antrea gateway to on manually"?

@tnqn
Copy link
Member

tnqn commented Jun 30, 2022

This is a supplement to PR #3832. When disableTXChecksumOffload is true, TX checksum offload should be also disabled, otherwise for the cases in which the datapath doesn't support TX checksum offloading, packets received on Antrea gateway could be dropped due to bad checksum.

  1. TX checksum offload of Antrea gateway should also be disabled
  2. packets sent from Antrea gateway

@@ -312,3 +313,12 @@ func (i *Initializer) RestoreOVSBridge() {
func (i *Initializer) setInterfaceMTU(iface string, mtu int) error {
return i.ovsBridgeClient.SetInterfaceMTU(iface, mtu)
}

func (i *Initializer) setTXChecksumOffload() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name it as disableTXChecksumOffload?
You mentioned when changing disableTXChecksumOffload from true back to false, TX checksum offload of Antrea gateway will not be enabled automatically, If user changes disableTXChecksumOffload in the config file, I assume Antrea agent need to be restarted? then I think we can check and enable it again automatically in codes?

Btw, you need re-base the codes and add more test cases for code coverage improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe name it as disableTXChecksumOffload?

There is variable of Initializer which is named disableTXChecksumOffload

Btw, you need re-base the codes and add more test cases for code coverage improvement.

Rebased, and could you provide some examples about such test?

This is a supplement to PR antrea-io#3832. When `disableTXChecksumOffload`
is true, TX checksum offload of Antrea gateway should be also
disabled, otherwise for the cases in which the datapath doesn't
support TX checksum offloading, packets sent from Antrea gateway
could be dropped due to bad checksum.

Note that, when changing `disableTXChecksumOffload` from true back
to false, TX checksum offload of Antrea gateway will not be enabled
automatically, and TX checksum offload can be enabled manually with
ethtool. Another way is to remove Antrea gateway interface before
updating `disableTXChecksumOffload`.

Signed-off-by: Hongliang Liu <[email protected]>
@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2022
@github-actions github-actions bot closed this Mar 28, 2023
@hongliangl hongliangl deleted the tx_offload branch December 5, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants