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

chore: change MTU only if not Azure CNI on Azure Stack #3367

Merged
merged 2 commits into from
May 29, 2020

Conversation

jadarsie
Copy link
Member

Reason for Change:

Azure CNI is not using encap, no need to reduce MTU on Azure Stack

Issue Fixed:

Related #1346

Requirements:

@jadarsie jadarsie requested a review from mboersma May 28, 2020 23:01
@acs-bot acs-bot added the size/S label May 28, 2020
@jadarsie jadarsie requested a review from jackfrancis May 28, 2020 23:01
echo " post-up /sbin/ifconfig eth0 mtu 1350" | sudo tee -a /etc/network/interfaces

ifconfig eth0 mtu 1350
{{- if not IsAzureCNI}}
Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary indentation

(the pattern we've been following thus far is not to indent actual static chars [e.g., the shell script in this case], so you can put the {{ and the echo statements on a common indentation indentation alignment at the beginning of the line)

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@jackfrancis
Copy link
Member

@jadarsie let me know when you've tested this successfully in an Azure Stack environment, and I'll merge

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #3367 into master will decrease coverage by 1.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3367      +/-   ##
==========================================
- Coverage   72.16%   70.96%   -1.20%     
==========================================
  Files         147      147              
  Lines       25419    26873    +1454     
==========================================
+ Hits        18343    19070     +727     
- Misses       5939     6563     +624     
- Partials     1137     1240     +103     
Impacted Files Coverage Δ
pkg/engine/templates_generated.go 47.14% <ø> (+2.66%) ⬆️
pkg/api/vlabs/validate.go 80.37% <0.00%> (-0.16%) ⬇️
pkg/api/defaults.go 92.58% <0.00%> (-0.07%) ⬇️

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 6d75e6e...a9fdf32. Read the comment docs.

@jadarsie jadarsie closed this May 29, 2020
@jadarsie jadarsie reopened this May 29, 2020
@jadarsie
Copy link
Member Author

@jadarsie let me know when you've tested this successfully in an Azure Stack environment, and I'll merge

this is good to go

Copy link
Member

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

@mboersma mboersma merged commit 607dcc7 into Azure:master May 29, 2020
@acs-bot
Copy link

acs-bot commented May 29, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, jadarsie, mboersma

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:
  • OWNERS [jackfrancis,mboersma]

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants