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

Interface mtu property #46

Merged
merged 3 commits into from
Jul 19, 2013

Conversation

jhoblitt
Copy link
Member

@jhoblitt jhoblitt commented Jul 9, 2013

I use jumbo frames on a few subnets so I've taken a stab at adding a mtu property to the network_config type and corresponding support in the redhat provider. This has only been tested on SL6.4. I haven't yet tried to implement debian family provider support but am happy to take that on before this PR is merged if this type API change is potentially mergable.

# puppet resource --modulepath=`pwd` network_config eth0
network_config { 'eth0':
  ensure    => 'present',
  family    => 'inet',
  ipaddress => '10.10.10.10',
  method    => 'static',
  mtu       => '9000',
  netmask   => '255.255.255.0',
  onboot    => 'true',
  options   => {'HWADDR' => '00:1F:C6:16:0A:91', 'IPV6INIT' => 'no', 'NM_CONTROLLED' => 'no', 'TYPE' => 'Ethernet'},
}

@ericsakowski
Copy link
Contributor

@jhoblitt - I'm confused by the need for this, why not just pass options => {'MTU' => '9000'} ?

@jhoblitt
Copy link
Member Author

@ericsakowski Setting the MTU on an interface is a property that should be universally abstractable for all platforms. Keys passed to options are not validated and are inherently platform specific. By your logic, why not remove all the properties from the the type except options?

@ericsakowski
Copy link
Contributor

@jhoblitt Good point -- sometimes I forget there are other platforms than RHEL.

# Frames small than 64bytes are discarded as runts. Smallest valid MTU
# is 42 with a 802.1q header and 46 without.
unless (value =~ /^[-+]?[0-9]+$/)
raise ArgumentError, "%s is not an valid mtu (must be an integer) " % value
Copy link
Member

Choose a reason for hiding this comment

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

It's generally preferred to use interpolation over format strings, EG "#{value} is not an valid mtu [...]

Copy link
Member

Choose a reason for hiding this comment

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

And tiny grammatical nitpick: it's better english to say #{value} is not a valid mtu :)

@adrienthebo
Copy link
Member

I'm 👍 on this, but I would like to add Debian support to this before it's merged, or add this as a feature.

@jhoblitt
Copy link
Member Author

ACK all those requests. I was hoping to get to the debian provider
yesterday but I'm currently in a $day_job high priority interrupt. :)

@jhoblitt
Copy link
Member Author

@adrienthebo I believe I've addressed your comments, made a few other minor tweaks and taken a stab the debian provider. The later still needs testing on real world boxen. It's the end of the day so I pushed but I can't keep github from updating the PR. I also rebased this branch on the current master.

I'm trying to decide if parsed mtu values should be checked inside of the provider or if trying to validate parsed values is the road to madness...

@jhoblitt
Copy link
Member Author

I've finally had some time to manually test this on a debian squeeze VM. I think it's ready for another review and/or merging.

# reject floating point and negative integers
# XXX this lets 1500.0 pass
unless (value =~ /^[+]?[0-9]+$/)
raise ArgumentError, "#{value} is not a valid mtu (must be a postive integer)"
Copy link
Member

Choose a reason for hiding this comment

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

Typo: 'postive' :)

@adrienthebo
Copy link
Member

@jhoblitt looks great! I had two tiny nitpicks, but once those are addressed I think this is good. :)

@jhoblitt
Copy link
Member Author

@adrienthebo I've fixed those two legitimate issues. ;) I think the patch set is in a mergable state now.

adrienthebo added a commit that referenced this pull request Jul 19, 2013
@adrienthebo adrienthebo merged commit 53a3d14 into voxpupuli:master Jul 19, 2013
@adrienthebo
Copy link
Member

BRILLIANT!

Merged into master in 53a3d14; this should be released in 0.5.0, thanks again! Awesome work!

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

Successfully merging this pull request may close these issues.

3 participants