[opendaylight] Update directory for openDaylight logs #1438

vpickard · 2018-09-25T15:07:37Z

OpenDaylight karaf logs are now located in:
/var/log/containers/opendaylight/karaf/logs, so
update the plugin to get the karaf.log files
from new location.

Resolves: #1793566
Depends-on: https://review.openstack.org/#/c/603907/
Signed-off-by: Victor Pickard vpickard@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?

pmoravec · 2018-10-16T18:50:21Z

sos/plugins/opendaylight.py

@@ -29,12 +29,12 @@ def setup(self):
        if self.get_option("all_logs"):
            self.add_copy_spec([
                "/opt/opendaylight/data/log/",
-                "/var/log/containers/opendaylight/",
+                "/var/log/containers/opendaylight/karaf/logs/",


This change means restriction in logs collected - is that intentional? Currently (when --all-logs option is used), whole directory /var/log/containers/opendaylight/ is collected. You request collecting just subdirectory here? (could be reasonable, just want to be sure it is the intention)

Yes, this is intentional. All ODL logs are now under /var/log/containers/opendaylight/karaf/logs. For example, we would have:

/var/log/containers/opendaylight/karaf/logs/karaf.log
/var/log/containers/opendaylight/karaf/logs/karaf.log.1

Whereas before, all ODL logs would be:

/var/log/containers/opendaylight/karaf.log
/var/log/containers/opendaylight/karaf.log.1

pmoravec · 2018-10-16T18:51:32Z

sos/plugins/opendaylight.py

            ])
        else:
            self.add_copy_spec([
                "/opt/opendaylight/data/log/*.log*",
-                "/var/log/containers/opendaylight/*.log*",
+                "/var/log/containers/opendaylight/karaf/logs/*.log*",


I guess the above change and also this one is triggered by moving the logfiles of interest from /var/log/containers/opendaylight/ to /var/log/containers/opendaylight/karaf/logs dir - if I am right, the change makes sense.

Yes, exactly. Thanks.

pmoravec · 2018-10-16T18:53:46Z

Does merging this PR depend on the ODL change? What if sosreport is called on a system with older ODL version that has logs in the current place - they wont be collected..?

(does not it make sense to collect both dirs / *.log* files from both dirs?)

vpickard · 2018-10-16T19:06:45Z

Does merging this PR depend on the ODL change? What if sosreport is called on a system with older ODL version that has logs in the current place - they wont be collected..?

(does not it make sense to collect both dirs / *.log* files from both dirs?)

Yes, this pull request depends on tripleO change:
https://review.openstack.org/#/c/603907/
which has not yet been merged, just waiting on gate jobs to pass. Hopefully later today or tomorrow.

That is a good question about running 'sosreport' on older versions. I think your suggestion is a good one, just collect the logs from both places to handle older versions. Let me check with couple other
team members to be sure, will push updated patch if they agree.

bmr-cymru · 2018-10-17T10:18:45Z

Can you be absolutely certain that the new release of sos will only ever be used by distributions with the new release of ODL? If not then you cannot just remove the old paths now - you need to add the new ones, mark the old ones as deprecated and we can remove them when the old version has vanished from the field.

vpickard · 2018-10-19T15:05:29Z

Can you be absolutely certain that the new release of sos will only ever be used by distributions with the new release of ODL? If not then you cannot just remove the old paths now - you need to add the new ones, mark the old ones as deprecated and we can remove them when the old version has vanished from the field.

Agree, can't be absolutely certain, so will do as you suggest, thanks.

What is the correct way to mark the old paths as deprecated, just a comment? Any examples?

bmr-cymru · 2018-10-19T15:17:48Z

What is the correct way to mark the old paths as deprecated, just a comment? Any examples?

Yeah - just a comment for now. We don't have a formal way of marking collection bits as deprecated.

It's informal - just meant as a reminder/note for future readers and editors, so something along the lines of:

    # These commands/paths are specific to deprecated version x.y.z and may be removed
    # in a future update.

vpickard · 2018-10-19T15:48:47Z

Yeah - just a comment for now. We don't have a formal way of marking collection bits as deprecated.

Thanks,
I have updated and pushed a new patch to my fork. Appreciate the review comments. Please let me know if the new patch looks ok, or if I need to make any changes to it.

pmoravec · 2018-10-20T07:31:52Z

Please fix pep8 / pycodestyle formatting issues from https://travis-ci.org/sosreport/sos/jobs/443763833 :

sos/plugins/opendaylight.py:31:77: W291 trailing whitespace
sos/plugins/opendaylight.py:32:61: W291 trailing whitespace
sos/plugins/opendaylight.py:44:77: W291 trailing whitespace
sos/plugins/opendaylight.py:45:61: W291 trailing whitespace

Further:

why to have two self.add_copy_spec calls (in either if-then-else branch) - why dont merge them?
or at least why to collect /opt/opendaylight/data/log/ or /opt/opendaylight/data/log/*log twice?

vpickard · 2018-10-22T16:00:17Z

Please fix pep8 / pycodestyle formatting issues from https://travis-ci.org/sosreport/sos/jobs/443763833 :

sos/plugins/opendaylight.py:31:77: W291 trailing whitespace
sos/plugins/opendaylight.py:32:61: W291 trailing whitespace
sos/plugins/opendaylight.py:44:77: W291 trailing whitespace
sos/plugins/opendaylight.py:45:61: W291 trailing whitespace

Done, thanks.

Further:

* why to have two `self.add_copy_spec` calls (in either if-then-else branch) - why dont merge them?
* or at least why to collect `/opt/opendaylight/data/log/` or `/opt/opendaylight/data/log/*log` twice?

Good point, doesn't make sense to collect the same twice! I have cleaned this up and pushed a new patch, thanks for the review comments.

bmr-cymru · 2018-10-23T15:30:40Z

Something a bit funny in that most recent commit - it seems to have lost its subject line?

First line of the commit should be:

    [module] brief description of change

(with a single blank line between the subject and the body of the commit).

vpickard · 2018-10-23T15:52:39Z

Something a bit funny in that most recent commit - it seems to have lost its subject line?

First line of the commit should be:
    [module] brief description of change
(with a single blank line between the subject and the body of the commit).

Apologies for that, I just pushed a new patch and added the subject line back.

This is my first go around working will pull requests on a forked repo, appreciate your comments and patience.

Thanks,
Vic

bmr-cymru · 2018-12-18T11:45:45Z

You need to clean up this branch before it is in a state to merge - currently, when pulling from your repo I end up with:

$ git log --oneline master..HEAD
269b0c90 (HEAD -> bmr-merge) [opendaylight] Update directory for openDaylight logs
fdbc81aa OpenDaylight karaf logs are now located in: /var/log/containers/opendaylight/karaf/logs, so update the plugin to get the karaf.log files from the new location.
bf7a48f5 [opendaylight] Update directory for openDaylight logs
483f9fb5 [opendaylight] Update directory for openDaylight logs

This does not comply with our requirements for commits to sos:

Each patch needs a proper subject in [module] change description format
One change is one commit: do not make multiple commits with the same message

You need to take all these changes, and either use git reset HEAD~4, and then re-do the commit with the right message and all the changes in one place, or you can use git rebase -i and the squash command to merge them together - either is fine and gives the same end result.

vpickard · 2018-12-18T14:01:27Z

On Tue, Dec 18, 2018 at 6:45 AM Bryn M. Reeves ***@***.***> wrote: You need to clean up this branch before it is in a state to merge - currently, when pulling from your repo I end up with: $ git log --oneline master..HEAD 269b0c90 (HEAD -> bmr-merge) [opendaylight] Update directory for openDaylight logs fdbc81aa OpenDaylight karaf logs are now located in: /var/log/containers/opendaylight/karaf/logs, so update the plugin to get the karaf.log files from the new location. bf7a48f5 [opendaylight] Update directory for openDaylight logs 483f9fb5 [opendaylight] Update directory for openDaylight logs This does not comply with our requirements for commits to sos: - Each patch needs a proper subject in [module] change description format - One change is one commit: do not make multiple commits with the same message You need to take all these changes, and either use git reset HEAD~4, and then re-do the commit with the right message and all the changes in one place, or you can use git rebase -i and the squash command to merge them together - either is fine and gives the same end result.

Thanks, I will take care of this shortly. Vic

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1438 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQXYWBkwJ5SeTrT0uFgmFqGoknj00_O-ks5u6NVqgaJpZM4W42YQ> .

OpenDaylight karaf logs are now located in: /var/log/containers/opendaylight/karaf/logs, so deprecate the old paths, and update the plugin to get the karaf.log files from new location. Signed-off-by: Victor Pickard <vpickard@redhat.com>

pmoravec reviewed Oct 16, 2018

View reviewed changes

vpickard mentioned this pull request Oct 16, 2018

Does merging this PR depend on the ODL change? What if sosreport is called on a system with older ODL version that has logs in the current place - they wont be collected..? #1454

Closed

vpickard force-pushed the master branch from 3a3edc9 to bc301d8 Compare December 18, 2018 15:44

bmr-cymru closed this in 27c61e1 Jan 8, 2019

bmr-cymru added this to the 3.7 milestone Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[opendaylight] Update directory for openDaylight logs #1438

[opendaylight] Update directory for openDaylight logs #1438

vpickard commented Sep 25, 2018 •

edited

pmoravec Oct 16, 2018

vpickard Oct 16, 2018

pmoravec Oct 16, 2018

vpickard Oct 16, 2018

pmoravec commented Oct 16, 2018 •

edited

vpickard commented Oct 16, 2018

bmr-cymru commented Oct 17, 2018

vpickard commented Oct 19, 2018

bmr-cymru commented Oct 19, 2018

vpickard commented Oct 19, 2018

pmoravec commented Oct 20, 2018

vpickard commented Oct 22, 2018

bmr-cymru commented Oct 23, 2018

vpickard commented Oct 23, 2018

bmr-cymru commented Dec 18, 2018

vpickard commented Dec 18, 2018 via email

[opendaylight] Update directory for openDaylight logs #1438

[opendaylight] Update directory for openDaylight logs #1438

Conversation

vpickard commented Sep 25, 2018 • edited

pmoravec Oct 16, 2018

Choose a reason for hiding this comment

vpickard Oct 16, 2018

Choose a reason for hiding this comment

pmoravec Oct 16, 2018

Choose a reason for hiding this comment

vpickard Oct 16, 2018

Choose a reason for hiding this comment

pmoravec commented Oct 16, 2018 • edited

vpickard commented Oct 16, 2018

bmr-cymru commented Oct 17, 2018

vpickard commented Oct 19, 2018

bmr-cymru commented Oct 19, 2018

vpickard commented Oct 19, 2018

pmoravec commented Oct 20, 2018

vpickard commented Oct 22, 2018

bmr-cymru commented Oct 23, 2018

vpickard commented Oct 23, 2018

bmr-cymru commented Dec 18, 2018

vpickard commented Dec 18, 2018 via email

vpickard commented Sep 25, 2018 •

edited

pmoravec commented Oct 16, 2018 •

edited