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

network: set max mtu of a parent to the max of its children #1371

Merged
merged 2 commits into from
Nov 14, 2017

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Oct 18, 2017

(originally at #1228, description copied)

iterate over all the children to obtain the max mtu that we need to
set to a parent nic. Before this, only the first vlan children of a parent
was used to set an mtu value, failing afterwards if the next vlan had a bigger
value.

Introduced in #1084

nicolasbock
nicolasbock previously approved these changes Oct 18, 2017
rhafer
rhafer previously approved these changes Nov 2, 2017
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

There is still @vuntz comment regarding a possible optimization: #1228 (comment)

But, I'd suggest to address that in a follow up PR to get this one finally merged.

@cmurphy
Copy link
Contributor Author

cmurphy commented Nov 2, 2017

@nicolasbock could you re-review? I had to correct a rebase error since your first review


unless all_children.empty?
max_mtu_nic = all_children.max_by { |k, v| v["mtu"] }[0]
ifs[nic.parent]["mtu"] = ifs[max_mtu_nic]["mtu"]
Copy link
Member

Choose a reason for hiding this comment

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

There was some .to_i in the initial PR. It's not strictly required, I guess, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you're right that it was there. I removed it because it there before the change and because it's an assignment, not a comparison, so it doesn't matter that much. I'll add it back though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cmurphy
Copy link
Contributor Author

cmurphy commented Nov 2, 2017

Please limit commit subject line to 50 characters.

Ha, you got me

Itxaka added 2 commits November 2, 2017 11:47
iterate over all the children to obtain the max mtu that we need to
set to a parent nic. Before this, only the first vlan children of a parent
was used to set an mtu value, failing afterwards if the next vlan had a bigger
value.
Navin found during testing that some configurations
could lead to the var all_children being empty and
that would break the chef run.

This introduces a guard against that.
@Itxaka
Copy link
Member

Itxaka commented Nov 2, 2017

Ha, you got me

🤣

@vuntz
Copy link
Member

vuntz commented Nov 3, 2017

Please limit commit subject line to 50 characters.

Ha, you got me

What about the missing uppercase? ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants