Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: configurable Linux eth0 MTU #4352

Merged
merged 5 commits into from
Apr 7, 2021
Merged

Conversation

jackfrancis
Copy link
Member

@jackfrancis jackfrancis commented Apr 1, 2021

Reason for Change:

This PR introduces a eth0MTU int property to linuxProfile, that can be used to configure either 1500 or 3900 for the eth0 interface.

We don't accept this configuration vector if you are in a kubenet configuration.

Issue Fixed:

In service of #4350

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Apr 1, 2021

Codecov Report

Merging #4352 (ecd5e77) into master (47c439d) will increase coverage by 0.01%.
The diff coverage is 92.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4352      +/-   ##
==========================================
+ Coverage   72.07%   72.08%   +0.01%     
==========================================
  Files         141      141              
  Lines       21640    21662      +22     
==========================================
+ Hits        15596    15616      +20     
- Misses       5093     5094       +1     
- Partials      951      952       +1     
Impacted Files Coverage Δ
pkg/api/types.go 92.85% <ø> (ø)
pkg/api/vlabs/types.go 73.04% <ø> (ø)
pkg/engine/templates_generated.go 43.56% <ø> (ø)
pkg/api/defaults.go 93.15% <60.00%> (-0.30%) ⬇️
pkg/api/converterfromapi.go 95.70% <100.00%> (+<0.01%) ⬆️
pkg/api/convertertoapi.go 94.02% <100.00%> (+0.01%) ⬆️
pkg/api/vlabs/validate.go 81.78% <100.00%> (+0.25%) ⬆️
pkg/engine/template_generator.go 68.39% <100.00%> (+0.19%) ⬆️
pkg/api/common/versions.go 96.37% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47c439d...ecd5e77. Read the comment docs.

@jackfrancis jackfrancis changed the title experiment: use 3900 MTU for eth0 feat: configurable Linux eth0 MTU Apr 5, 2021
@aramase
Copy link
Member

aramase commented Apr 5, 2021

/cc

@@ -452,6 +452,9 @@ MASTER_CONTAINER_ADDONS_PLACEHOLDER
mount --bind $MOUNT_DIR $MOUNT_DIR
fi
mount --make-shared $MOUNT_DIR
{{- if IsAzureCNI}}
ifconfig eth0 mtu {{GetEth0MTU}} up
Copy link
Contributor

@khenidak khenidak Apr 5, 2021

Choose a reason for hiding this comment

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

This is not persisted between restarts. Shouldn't that go in /etc/network/interfaces ?

Copy link
Member Author

Choose a reason for hiding this comment

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

putting the statement into this script means the ifconfig update happens before every systemd kubelet startup

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, doing it this way tightly couples the configuration to kubelet/k8s (even if the MTU is changed out of band for some reason, kubelet will always set it to the desired value before it starts up) rather than making an OS-wide config one time at cluster startup.

(Also, this was the easiest way to do it, so... :))

pkg/api/vlabs/validate.go Show resolved Hide resolved
allowedMTUs += strconv.Itoa(mtu) + ", "
}
allowedMTUs = strings.TrimRight(allowedMTUs, ", ")
return errors.Errorf("Invalid linuxProfile eth0MTU value \"%d\", please use one of the following versions: %s", a.LinuxProfile.Eth0MTU, allowedMTUs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return errors.Errorf("Invalid linuxProfile eth0MTU value \"%d\", please use one of the following versions: %s", a.LinuxProfile.Eth0MTU, allowedMTUs)
return errors.Errorf("Invalid linuxProfile eth0MTU value \"%d\", please use one of the following values: %s", a.LinuxProfile.Eth0MTU, allowedMTUs)

nit

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

lgtm

@acs-bot
Copy link

acs-bot commented Apr 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, jackfrancis

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

The pull request process is described 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

@jackfrancis jackfrancis merged commit 4e47f3d into Azure:master Apr 7, 2021
@jackfrancis jackfrancis deleted the mtu-3900 branch April 7, 2021 00:02
@bridgetkromhout bridgetkromhout mentioned this pull request Mar 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants