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

[3.0] network: set max mtu of a parent to the max of its children #1230

Closed
wants to merge 1 commit into from
Closed

[3.0] network: set max mtu of a parent to the max of its children #1230

wants to merge 1 commit into from

Conversation

Itxaka
Copy link
Member

@Itxaka Itxaka commented May 5, 2017

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.

(cherry picked from commit 9c7a1b3)

max_mtu_nic = all_children.max_by(&:mtu)
else
# parent is not a bond
if ifs.key?(parent_nic.name) &&

Choose a reason for hiding this comment

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

Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
Style/IfInsideElse: Convert if nested inside else to elsif.

# parent is a bond
# check bond slaves interfaces for mtu values
parent_nic.slaves.each do |bond_iface|
next if ifs[bond_iface.name] &&

Choose a reason for hiding this comment

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

Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)

@Itxaka
Copy link
Member Author

Itxaka commented May 9, 2017

Updated with latest changes from 9c7a1b3

max_mtu_nic = all_children.max_by{ |k, v| v["mtu"] }[0]
end
ifs[nic.parent]["mtu"] = ifs[max_mtu_nic]["mtu"]
parent_nic.mtu = ifs[max_mtu_nic]["mtu"]

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

all_children = ifs.select { |k, v| v["parent"] == nic.parent && v.key?("mtu") }
max_mtu_nic = all_children.max_by{ |k, v| v["mtu"] }[0]
end
ifs[nic.parent]["mtu"] = ifs[max_mtu_nic]["mtu"]

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

else
# no problems, set the highest mtu of all children in the same interface
all_children = ifs.select { |k, v| v["parent"] == nic.parent && v.key?("mtu") }
max_mtu_nic = all_children.max_by{ |k, v| v["mtu"] }[0]

Choose a reason for hiding this comment

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

Style/SpaceBeforeBlockBraces: Space missing to the left of {.

# no problems, set the highest mtu of the bond's children
all_children = ifs.select { |k, v| v["parent"] == nic.parent && v.key?("mtu") }
max_mtu_nic = all_children.max_by{ |k, v| v["mtu"] }[0]
else

Choose a reason for hiding this comment

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

Style/ElseAlignment: Align else with if.

end
# no problems, set the highest mtu of the bond's children
all_children = ifs.select { |k, v| v["parent"] == nic.parent && v.key?("mtu") }
max_mtu_nic = all_children.max_by{ |k, v| v["mtu"] }[0]

Choose a reason for hiding this comment

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

Lint/UselessAssignment: Useless assignment to variable - max_mtu_nic. (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars)
Style/IndentationConsistency: Inconsistent indentation detected.
Style/SpaceBeforeBlockBraces: Space missing to the left of {.

"wants a mtu of #{ifs[nic.name]["mtu"]}. This configuration is invalid."
Chef::Log.fatal(msg)
raise msg
end

Choose a reason for hiding this comment

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

Lint/BlockAlignment: end at 404, 10 is not aligned with parent_nic.slaves.each do |bond_iface| at 394, 8.

"custom mtu of #{ifs[bond_iface.name]["mtu"]} but children vlan #{nic.name} "\
"wants a mtu of #{ifs[nic.name]["mtu"]}. This configuration is invalid."
Chef::Log.fatal(msg)
raise msg

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

msg = "Interface #{bond_iface.name}, part of bond #{parent_nic.name} has a "\
"custom mtu of #{ifs[bond_iface.name]["mtu"]} but children vlan #{nic.name} "\
"wants a mtu of #{ifs[nic.name]["mtu"]}. This configuration is invalid."
Chef::Log.fatal(msg)

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

ifs[bond_iface.name] &&
ifs[bond_iface.name].key?("mtu") &&
ifs[bond_iface.name]["mtu"] < ifs[nic.name]["mtu"]
msg = "Interface #{bond_iface.name}, part of bond #{parent_nic.name} has a "\

Choose a reason for hiding this comment

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

Style/IndentationConsistency: Inconsistent indentation detected.

# parent is a bond
# check bond slaves interfaces for mtu values
parent_nic.slaves.each do |bond_iface|
next unless ifs[nic.name].key?("mtu") &&

Choose a reason for hiding this comment

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

Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
Style/IndentationWidth: Use 2 (not 0) spaces for indentation. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

@Itxaka
Copy link
Member Author

Itxaka commented May 16, 2017

updated with changes from c108dba

@Itxaka
Copy link
Member Author

Itxaka commented May 16, 2017

@Itxaka
Copy link
Member Author

Itxaka commented May 17, 2017

mkcloud HA failure seems related to manila no finding the flavor, doesnt seem to relate to this PR

@vuntz
Copy link
Member

vuntz commented Jun 1, 2017

@Itxaka I'd feel more comfortable cherry-picking #1084 and then #1228, instead of having one commit with both changes

@Itxaka
Copy link
Member Author

Itxaka commented Jun 1, 2017

@vuntz doesnt #1228 overrides #1084 anyway? or do you want to preserve the history of both patches together?

@Itxaka
Copy link
Member Author

Itxaka commented Jun 1, 2017

oh wait, there is a line missing from #1084 which is not on #1228 :( :(

@vuntz
Copy link
Member

vuntz commented Jun 2, 2017

@vuntz doesnt #1228 overrides #1084 anyway? or do you want to preserve the history of both patches together?

When we backport, we tend to cherry-pick to preserve history and to link to how the thing was fixed in the main branch so we have the full context.

@Itxaka
Copy link
Member Author

Itxaka commented Jun 5, 2017

Blocked until #1241 is merged first, to preserve proper history of the changes

@vuntz
Copy link
Member

vuntz commented Jun 28, 2017

For the record, this should also be blocked because #1228 is still not merged...

@Itxaka
Copy link
Member Author

Itxaka commented Jun 28, 2017

For the record, this should also be blocked because #1228 is still not merged...

I hope that was understood, even if not said :P

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.

(cherry picked from commit c108dba)
@Itxaka
Copy link
Member Author

Itxaka commented Jul 10, 2017

Needs update once #1228 is squashed and merged!

@Itxaka
Copy link
Member Author

Itxaka commented Oct 20, 2017

recovered at #1371

@Itxaka Itxaka closed this Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants