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

Expose ring parameters in the network plugin #827

Merged
merged 2 commits into from
Jun 17, 2016
Merged

Conversation

davide125
Copy link
Contributor

Extend the network plugin to expose interface ring parameters using ethtool.

I'm a Facebook employee and @jaymzh will do the final merge.

@chef-supermarket
Copy link

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

@lamont-granquist
Copy link
Contributor

@davide125 you should probably still do the minimum viable clickiness to shut up currybot, even though you're covered under facebook...

@thommay
Copy link
Contributor

thommay commented May 31, 2016

👍

@lamont-granquist
Copy link
Contributor

i find it amusing that you care this much about the TX and RX ring buffer settings in your infrastructure (having worked at amazon for 5 years back in the dark ages where we abused the crap out of all our networking equipment as well...)

👍

@jaymzh
Copy link
Collaborator

jaymzh commented Jun 1, 2016

@lamont-granquist as mentioned in several meetings with various members of Chef, no one else at Facebook can sign the CCLA... cause reasons. Legal stuff sucks. That's why we have this stupid "phil has to merge it to make sure it legally falls under phil's CCLA" crap.

@lamont-granquist
Copy link
Contributor

sigh, lawyers... okay...

next unless iface[tmp_int][:encapsulation] == "Ethernet"
so = shell_out("#{ethtool_binary} -g #{tmp_int}")
Ohai::Log.debug("Parsing ethtool output: #{so.stdout}")
# Nasty, brittle regex to capture the ethtool output
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything that would make this less nasty and brittle? Multiple queries for each match? Or just that we're reading from something that may change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to suggestions. The main thing that makes this brittle to me is the reliance on perfect output matching, which means that any change in ethtool formatting of this would break it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can think of two things to make this less brittle:

  1. Easy: Don't assume "RX" will always be first, so stick a .* before that in case they add preceeding lines
  2. A bit less easy: Parse the output line by line so you don't assume any order:
type = nil
lines.each do |line|
  next if line.start_with?('Ring params for')
  next if line.strip.nil?
  if line =~ /Pre-set maximums/
    type = 'max'
    next
  end
  if line =~ /Current hardware settings/
    type = 'current'
    next
  end
  key, val = line.split(/:\s+/)
  iface[tmp_int]["ring_params"]["#{type}_{key.downcase}"] = val
end

This will also pull in other settings which will be awesome.

@jaymzh
Copy link
Collaborator

jaymzh commented Jun 15, 2016

This looks good to me, I'm 👍 - though I'm a bit biased, cause I basically write this version. :)

Can one of you @chef/client-core ack this version as well before I merge?

@mcquin
Copy link
Contributor

mcquin commented Jun 16, 2016

💯

@jaymzh jaymzh merged commit cd5324d into chef:master Jun 17, 2016
@tas50 tas50 changed the title expose ring parameters in the network plugin Expose ring parameters in the network plugin Jun 17, 2016
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants