[lustre] Add Lustre filesystem plugin #1058

utopiabound · 2017-07-13T17:26:53Z

This collects some basic information about the configuration
of Lustre on either a client or a server, including:

Interal ring buffer log
Basic configuration information
Quota information
Configuration of LNet
JobID info
LDLM connection states

Signed-off-by: Nathaniel Clark Nathaniel.Clark@misrule.us

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?

utopiabound · 2017-07-13T17:28:36Z

This is the fixed version of Pull request #1057

bmr-cymru · 2017-07-13T18:10:26Z

I'm just heading out for the day but based on a quick read this looks good - thanks for making the changes.

I see Adam's added his ack so I'll try to get a proper review done in the morning so we can go ahead and merge.

bmr-cymru · 2017-07-13T18:12:14Z

sos/plugins/lustre.py

+    def setup(self):
+        self.add_cmd_output(["lctl debug_kernel",
+                             "lctl device_list"])
+


The conventional style in sos is as in the example I gave:

self.add_cmd_output([ "command one", "command two", "command ..." ])

If you wouldn't mind respinning this and updating the branch, it helps to have as consistent a style throughout as we can (otherwise it's a minor tweak I can fix up when I merge).

Sorry, I missed the formatting; I'll do that.

bmr-cymru · 2017-07-13T18:12:51Z

sos/plugins/lustre.py

+
+        if (self.is_installed('lustre-client')):
+            self.add_cmd_output(["lfs df",
+                                 "lfs df -i"])


Ditto here.

bmr-cymru · 2017-07-13T18:15:25Z

sos/plugins/lustre.py

+        else:
+            self.get_params("osd", ["osd-*.*.{mntdev,files*," +
+                                    "kbytes*,blocksize,brw_stats}"])
+            self.get_params("quota", ["osd-*.*.quota_slave." +


These are OK as they are; if anything we'd pregenerate the list in a variable, but it does not seem worth it here since there is no real commonality beyond the "osd-.." prefix.

These may want to be protected by an if not self.is_installed(): if the previous conditional is removed and these commands will spit error messages - in this case, because the command does exist on the system we will be able to run it, and will collect whatever output it generates in the report.

utopiabound · 2017-07-17T13:03:21Z

This new patch should address all the previous comments.

utopiabound · 2017-08-03T15:46:55Z

Is there anything else that's needed for this to land?

bmr-cymru · 2017-08-31T16:43:32Z

sos/plugins/lustre.py

+        self.get_params("ldlm-states", ["*.*.state"])
+        self.get_params("jobid", ["jobid_name", "jobid_var"])
+
+        if (self.is_installed('lustre-client')):


There's no real reason to make this conditional - all command execution in sos is speculative (it's not a big deal, but it tends to make plugins cleaner/simpler if they follow the "forgiveness rather than permission" approach).

bmr-cymru

The plugin is suitable for merging now - thanks for working on this. There are a couple of very minor tweaks that would improve readability but they are not blockers for including the changes.

bmr-cymru · 2017-08-31T16:45:23Z

sos/plugins/lustre.py

+        else:
+            self.get_params("osd", ["osd-*.*.{mntdev,files*," +
+                                    "kbytes*,blocksize,brw_stats}"])
+            self.get_params("quota", ["osd-*.*.quota_slave." +


These may want to be protected by an if not self.is_installed(): if the previous conditional is removed and these commands will spit error messages - in this case, because the command does exist on the system we will be able to run it, and will collect whatever output it generates in the report.

jgrund · 2017-10-18T20:03:59Z

Is this able to land? Would like to have this in a sos report.

bmr-cymru · 2017-10-19T09:42:45Z

We will try to pull it in for 3.5 (a few weeks overdue currently).

This collects some basic information about the configuration of Lustre on either a client or a server, including: * Interal ring buffer log * Basic configuration information * Quota information * Configuration of LNet * JobID info * LDLM connection states Signed-off-by: Nathaniel Clark <Nathaniel.Clark@misrule.us>

utopiabound · 2017-11-15T18:19:09Z

I've updated the plugin with more information collection. I removed the server/client distinction and just collect everything as that will be cleaner if for instance, the modules aren't installed via the "normal" rpms.

utopiabound · 2018-01-31T15:56:31Z

This seems to have missed 3.5, can we land it for 3.6?

bmr-cymru · 2018-03-27T14:05:46Z

This seems to have missed 3.5, can we land it for 3.6?

It just got to the front of the review queue, so I hope so - we're expecting to release 3.6 next month.

bmr-cymru

Ack

adam-stokes approved these changes Jul 13, 2017

View reviewed changes

adam-stokes requested a review from bmr-cymru July 13, 2017 17:30

bmr-cymru reviewed Jul 13, 2017

View reviewed changes

sos/plugins/lustre.py Outdated

if (self.is_installed('lustre-client')):

self.add_cmd_output(["lfs df",

"lfs df -i"])

Copy link

Member

bmr-cymru Jul 13, 2017

Choose a reason for hiding this comment

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

Ditto here.

bmr-cymru reviewed Jul 13, 2017

View reviewed changes

utopiabound force-pushed the feature/lustre branch from 51485c4 to 96908e0 Compare July 13, 2017 18:25

bmr-cymru reviewed Aug 31, 2017

View reviewed changes

bmr-cymru approved these changes Aug 31, 2017

View reviewed changes

utopiabound force-pushed the feature/lustre branch from 96908e0 to 9eb08a5 Compare November 15, 2017 18:17

TurboTurtle added this to the 3.6 milestone Feb 7, 2018

bmr-cymru approved these changes Mar 27, 2018

View reviewed changes

bmr-cymru closed this in 9e698dc Mar 27, 2018

utopiabound deleted the feature/lustre branch March 27, 2020 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lustre] Add Lustre filesystem plugin #1058

[lustre] Add Lustre filesystem plugin #1058

utopiabound commented Jul 13, 2017

utopiabound commented Jul 13, 2017

bmr-cymru commented Jul 13, 2017

bmr-cymru Jul 13, 2017

utopiabound Jul 13, 2017

bmr-cymru Jul 13, 2017

bmr-cymru Jul 13, 2017

bmr-cymru Aug 31, 2017

utopiabound commented Jul 17, 2017

utopiabound commented Aug 3, 2017

bmr-cymru Aug 31, 2017

bmr-cymru left a comment

bmr-cymru Aug 31, 2017

jgrund commented Oct 18, 2017

bmr-cymru commented Oct 19, 2017

utopiabound commented Nov 15, 2017

utopiabound commented Jan 31, 2018

bmr-cymru commented Mar 27, 2018

bmr-cymru left a comment

[lustre] Add Lustre filesystem plugin #1058

[lustre] Add Lustre filesystem plugin #1058

Conversation

utopiabound commented Jul 13, 2017

utopiabound commented Jul 13, 2017

bmr-cymru commented Jul 13, 2017

bmr-cymru Jul 13, 2017

Choose a reason for hiding this comment

utopiabound Jul 13, 2017

Choose a reason for hiding this comment

bmr-cymru Jul 13, 2017

Choose a reason for hiding this comment

bmr-cymru Jul 13, 2017

Choose a reason for hiding this comment

bmr-cymru Aug 31, 2017

Choose a reason for hiding this comment

utopiabound commented Jul 17, 2017

utopiabound commented Aug 3, 2017

bmr-cymru Aug 31, 2017

Choose a reason for hiding this comment

bmr-cymru left a comment

Choose a reason for hiding this comment

bmr-cymru Aug 31, 2017

Choose a reason for hiding this comment

jgrund commented Oct 18, 2017

bmr-cymru commented Oct 19, 2017

utopiabound commented Nov 15, 2017

utopiabound commented Jan 31, 2018

bmr-cymru commented Mar 27, 2018

bmr-cymru left a comment

Choose a reason for hiding this comment