[gluster_block] Added new plugin gluster_block #1041

vredara · 2017-06-19T07:09:54Z

gluster_block plugin collects config and log files related to blocks

Signed-off-by: venkata edara redara@redhat.com

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?

bmr-cymru · 2017-06-19T10:15:08Z

In future please fill out the pull request template - it is there to save everyone time (including you, by avoiding unnecessary requests to respin the branch).

bmr-cymru

This should be a very short and simple plugin: just detect the presence of gluster_block and collect the logs. Nothing more.

Duplicating the targetcli plugin functionality is not acceptable - once all that is removed we can merge the rest as a simple log collector for the block service.

bmr-cymru · 2017-06-19T10:16:15Z

sos/plugins/gluster_block.py

+    plugin_name = 'gluster_block'
+    profiles = ('storage',)
+    packages = ["gluster-block"]
+    files = ["/usr/sbin/gluster-block"]


Why lists for these? The normal sos style is to use a tuple (since they are invariant and do not need the baggage of a mutable list type).

ok. I am changing lists to tuples.

bmr-cymru · 2017-06-19T10:16:48Z

sos/plugins/gluster_block.py

+    config_to_collect = ["/sys/kernel/config/target", "/etc/target"]
+
+    def setup(self):
+        self.add_cmd_output("targetcli ls")


Nack. We have the targetcli plugin for that.

Sure. removed this cmd.

bmr-cymru · 2017-06-19T10:17:29Z

sos/plugins/gluster_block.py

+
+        # collect config files
+        for config_file in self.config_to_collect:
+            self.add_copy_spec(config_file)


Nack. Please look at the API documentation. The add_copy_spec() method accepts either a single string, or a list of strings. This is just pointless make-work.

ok, will collect only "/sys/kernel/config/target" which is not being collected by targetcli.

It needs to go into the targetcli plugin - you can either file a separate pull request for that, or just amend your existing commit (to address the review items), and then add a new commit with the change for targetcli. Once that's done you can update the branch for the PR with git push --force.

Note that we don't accept 'fixup' commits in master so you'll need to respin the branch anyway for the review items.

bmr-cymru · 2017-06-19T10:17:49Z

sos/plugins/gluster_block.py

+            limit = self.get_option("log_size")
+        else:
+            limit = 0
+


Ack, thanks for implementing log limiting.

bmr-cymru · 2017-06-19T10:19:36Z

sos/plugins/gluster_block.py

+            limit = 0
+
+        if limit:
+            for f in glob.glob("/var/log/gluster-block/*.log"):


Ack. This is ugly, but it is in keeping with other plugins that try to work around the current limits API shortcomings. We can clean it up later when those are addressed.

bmr-cymru · 2017-06-19T10:20:05Z

sos/plugins/gluster_block.py

+    profiles = ('storage',)
+    packages = ["gluster-block"]
+    files = ["/usr/sbin/gluster-block"]
+    config_to_collect = ["/sys/kernel/config/target", "/etc/target"]


Nack: you're just duplicating the targetcli plugin under a different name!

Didnt observed the targetcli plugin, I will remove targetcli from gluster-block plugin.

vredara · 2017-06-19T13:02:53Z

I changed the code in latest commit. please review the latest changes.

bmr-cymru · 2017-06-19T13:21:37Z

Please don't push fixup commits ("update gluster_block.py" - and even without being a fixup, that subject does not comply with the commit formatting guidelines - every commit must be in [component] short description of change format).

Use git reset and git commit --amend, or git rebase -i to produce a single commit with the changes you want to merge, and then push the result with git push --force.

bmr-cymru

Need to remove targetcli change - can either move it to targetcli in this PR, or start a new one - I don't mind, but it does not belong in this plugin.

Once that change is done & the extra commit folded back into the original this should be ready for merge.

bmr-cymru · 2017-06-19T13:23:41Z

sos/plugins/gluster_block.py

-        for config_file in self.config_to_collect:
-            self.add_copy_spec(config_file)
+        # collect config
+        self.add_copy_spec("/sys/kernel/config/target")


As discussed, this needs to go in the targetcli plugin.

bmr-cymru · 2017-06-19T13:24:15Z

sos/plugins/gluster_block.py

@@ -43,3 +40,4 @@ def setup(self):
                self.add_copy_spec(f, limit)
        else:
            self.add_copy_spec("/var/log/gluster-block")
+


Other than the remaining targetcli line these changes look fine, once they are folded into the original commit and the branch updated.

vredara · 2017-06-20T08:41:07Z

Sorry for too many updates. I am contributing to sos report for 1st time. I made code changes according to guidelines and submitted new commit. the new commit also has target cli changes in targetcli plugin only. so gluster block plugin only collects logs related to it.

vredara · 2017-06-27T10:41:29Z

can you please review my latest commit. I incorporated all the comments.

bmr-cymru · 2017-06-27T12:02:57Z

You should either note that targetcli change in the commit message or (better, but I won't insist on it), in a separate commit (although technically, to comply with our commit formatting, you'd then need to have a subject of [targetcli, gluster_block] add new plugin and one item to targetcli, which just looks weird..).

It's obfuscating to hide a change to another plugin inside a commit that say "add a new plugin"; it's potentially confusing for people reading the logs and trying to track down changes.

The point is to make it easy for folks reviewing for changes, or filtering commits by affected plugins.

gluster_block plugin collects config/log files related to blocks Signed-off-by: venkata edara <redara@redhat.com>

vredara · 2017-06-28T07:08:33Z

I migrated all changes of targetcli to other commit of branch "target-cli-collect-kernel-config". I created new pull request for targetcli separately. gluster-block-plugin has only gluster-block changes. Let me know if the commit changes are good.

…

-Venkat

On 06/27/2017 05:32 PM, Bryn M. Reeves wrote: You should either note that |targetcli| change in the commit message or (better, but I won't insist on it), in a separate commit (although technically, to comply with our commit formatting, you'd then need to have a subject of |[targetcli, gluster_block] add new plugin and one item to targetcli|, which just looks weird..). It's obfuscating to hide a change to another plugin inside a commit that say "add a new plugin"; it's potentially confusing for people reading the logs and trying to track down changes. The point is to make it easy for folks reviewing for changes, or filtering commits by affected plugins. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1041 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYE71nQ-u3XFSQoP0MM9YJADRyxBD_tTks5sIO9ygaJpZM4N91UR>.

bmr-cymru

ack

bmr-cymru · 2017-06-28T16:41:30Z

Looks good, thanks for re-working. 👍

bmr-cymru self-requested a review June 19, 2017 10:15

bmr-cymru self-assigned this Jun 19, 2017

bmr-cymru requested changes Jun 19, 2017

View reviewed changes

vredara force-pushed the gluster-block-plugin branch from 1cd8706 to 0028b5f Compare June 20, 2017 07:28

[gluster_block] Add new plugin gluster_block

66e11ab

gluster_block plugin collects config/log files related to blocks Signed-off-by: venkata edara <redara@redhat.com>

vredara force-pushed the gluster-block-plugin branch from 0028b5f to 66e11ab Compare June 28, 2017 06:38

bmr-cymru approved these changes Jun 28, 2017

View reviewed changes

adam-stokes approved these changes Jun 28, 2017

View reviewed changes

adam-stokes merged commit 07e6ce3 into sosreport:master Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gluster_block] Added new plugin gluster_block #1041

[gluster_block] Added new plugin gluster_block #1041

vredara commented Jun 19, 2017 •

edited

bmr-cymru commented Jun 19, 2017

bmr-cymru left a comment •

edited

bmr-cymru Jun 19, 2017

vredara Jun 19, 2017

bmr-cymru Jun 19, 2017

vredara Jun 19, 2017

bmr-cymru Jun 19, 2017

vredara Jun 19, 2017

bmr-cymru Jun 19, 2017 •

edited

bmr-cymru Jun 19, 2017

bmr-cymru Jun 19, 2017

bmr-cymru Jun 19, 2017

vredara Jun 19, 2017

vredara commented Jun 19, 2017

bmr-cymru commented Jun 19, 2017

bmr-cymru left a comment

bmr-cymru Jun 19, 2017

bmr-cymru Jun 19, 2017

vredara commented Jun 20, 2017

vredara commented Jun 27, 2017

bmr-cymru commented Jun 27, 2017

vredara commented Jun 28, 2017 via email

bmr-cymru left a comment

bmr-cymru commented Jun 28, 2017

[gluster_block] Added new plugin gluster_block #1041

[gluster_block] Added new plugin gluster_block #1041

Conversation

vredara commented Jun 19, 2017 • edited

bmr-cymru commented Jun 19, 2017

bmr-cymru left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmr-cymru Jun 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vredara commented Jun 19, 2017

bmr-cymru commented Jun 19, 2017

bmr-cymru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vredara commented Jun 20, 2017

vredara commented Jun 27, 2017

bmr-cymru commented Jun 27, 2017

vredara commented Jun 28, 2017 via email

bmr-cymru left a comment

Choose a reason for hiding this comment

bmr-cymru commented Jun 28, 2017

vredara commented Jun 19, 2017 •

edited

bmr-cymru left a comment •

edited

bmr-cymru Jun 19, 2017 •

edited