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

app-admin/logstash-bin: bump to 5.5.3/5.6.1, drop old #5665

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@hydrapolic
Contributor

hydrapolic commented Sep 8, 2017

No description provided.

@gentoo-repo-qa-bot

This comment has been minimized.

Show comment
Hide comment
@gentoo-repo-qa-bot

gentoo-repo-qa-bot Sep 8, 2017

Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: app-admin/logstash-bin

app-admin/logstash-bin: @hydrapolic, erkiferenc[at]gmail.com, @gentoo/proxy-maint

Collaborator

gentoo-repo-qa-bot commented Sep 8, 2017

Pull Request assignment

Areas affected: ebuilds
Packages affected: app-admin/logstash-bin

app-admin/logstash-bin: @hydrapolic, erkiferenc[at]gmail.com, @gentoo/proxy-maint

@ferki

This comment has been minimized.

Show comment
Hide comment
@ferki

ferki Sep 8, 2017

Contributor

Nice, thanks!

Couple of quick questions (without testing these cases out yet):

  1. Would the move of the pidfile interfere with shutting down an already running instance? (Also, should existing users remove the old pidfile dir?)
  2. Should users move the data they may already have in /opt/logstash/data?

I guess it might be enough to just notify the users on upgrade, to make sure they can check if they need clean up and/or move data properly (on first install these doesn't seem to matter).

Contributor

ferki commented Sep 8, 2017

Nice, thanks!

Couple of quick questions (without testing these cases out yet):

  1. Would the move of the pidfile interfere with shutting down an already running instance? (Also, should existing users remove the old pidfile dir?)
  2. Should users move the data they may already have in /opt/logstash/data?

I guess it might be enough to just notify the users on upgrade, to make sure they can check if they need clean up and/or move data properly (on first install these doesn't seem to matter).

@hydrapolic

This comment has been minimized.

Show comment
Hide comment
@hydrapolic

hydrapolic Sep 10, 2017

Contributor
  1. Yes, the logstash instance will not stop and the user will notice as he will not be able to start a new one. But I do suppose the Gentoo users are smart enough to find the old process and kill it (there will be a hint in the log "already running"). The old piddir will disappear after the reboot, so no action is needed.

  2. I only run two instances of logstash, but haven't found anything valuable in the data directory - only the uuid that is generated automatically.

But I'm also fine by adding a note for those that are upgrading.

Contributor

hydrapolic commented Sep 10, 2017

  1. Yes, the logstash instance will not stop and the user will notice as he will not be able to start a new one. But I do suppose the Gentoo users are smart enough to find the old process and kill it (there will be a hint in the log "already running"). The old piddir will disappear after the reboot, so no action is needed.

  2. I only run two instances of logstash, but haven't found anything valuable in the data directory - only the uuid that is generated automatically.

But I'm also fine by adding a note for those that are upgrading.

@ferki

This comment has been minimized.

Show comment
Hide comment
@ferki

ferki Sep 18, 2017

Contributor

I believe the data directory mostly (only?) used for the persistent queue, which might be important during non-normal operation (e.g. abnormal pipeline termination, or similar), that might happen after moving the pidfile, and killing the process forcefully (see Shutting Down Logstash).

Other use case for the persistent queues is to provide buffering during bursts and/or when the delivery is slower than the incoming amount of logs. So normally there's no data, but if there is something, then it is most probably important (and need to be picked up by the new process).

I also think Gentoo users who are using persistent queues are also able to shut down the old instance properly, and check if there's any data to be moved. Though after thinking a lot about this, I'd say the ebuild should at least remind the user about these, as I'm afraid it can lead to a potential data loss. Something along the lines of:

The default pidfile directory has been changed from /run/logstash to /run. Please ensure any running logstash processes are shut down cleanly.
The default data directory has been moved from /opt/logstash/data to /var/lib/logstash/data. Please check and move its contents as necessary.

Maybe I'm just overly cautious :)

Contributor

ferki commented Sep 18, 2017

I believe the data directory mostly (only?) used for the persistent queue, which might be important during non-normal operation (e.g. abnormal pipeline termination, or similar), that might happen after moving the pidfile, and killing the process forcefully (see Shutting Down Logstash).

Other use case for the persistent queues is to provide buffering during bursts and/or when the delivery is slower than the incoming amount of logs. So normally there's no data, but if there is something, then it is most probably important (and need to be picked up by the new process).

I also think Gentoo users who are using persistent queues are also able to shut down the old instance properly, and check if there's any data to be moved. Though after thinking a lot about this, I'd say the ebuild should at least remind the user about these, as I'm afraid it can lead to a potential data loss. Something along the lines of:

The default pidfile directory has been changed from /run/logstash to /run. Please ensure any running logstash processes are shut down cleanly.
The default data directory has been moved from /opt/logstash/data to /var/lib/logstash/data. Please check and move its contents as necessary.

Maybe I'm just overly cautious :)

@hydrapolic

This comment has been minimized.

Show comment
Hide comment
@hydrapolic

hydrapolic Sep 19, 2017

Contributor

Yeah, that sounds reasonable @ferki. I'll add it to the PR and bump to 5.5.3 instead.

Thanks!

Contributor

hydrapolic commented Sep 19, 2017

Yeah, that sounds reasonable @ferki. I'll add it to the PR and bump to 5.5.3 instead.

Thanks!

@hydrapolic hydrapolic changed the title from app-admin/logstash-bin: bump to 5.5.2, drop old to app-admin/logstash-bin: bump to 5.5.3, drop old Sep 20, 2017

@hydrapolic

This comment has been minimized.

Show comment
Hide comment
@hydrapolic

hydrapolic Sep 20, 2017

Contributor

@ferki @orlitzky, ok with this PR?

Contributor

hydrapolic commented Sep 20, 2017

@ferki @orlitzky, ok with this PR?

@ferki

This comment has been minimized.

Show comment
Hide comment
@ferki

ferki Sep 21, 2017

Contributor

@hydrapolic: for me, it looks working correctly and as intended 👍

Contributor

ferki commented Sep 21, 2017

@hydrapolic: for me, it looks working correctly and as intended 👍

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Sep 21, 2017

Contributor

Personally I would remove LS_PIDFILE from the conf.d file and just set pidfile="/run/${RC_SVCNAME}.pid" (which will be unique, because the service name is). That way no one can try to change the PID file location and cause themselves problems. But the vulnerability is fixed, so it's good by me.

Contributor

orlitzky commented Sep 21, 2017

Personally I would remove LS_PIDFILE from the conf.d file and just set pidfile="/run/${RC_SVCNAME}.pid" (which will be unique, because the service name is). That way no one can try to change the PID file location and cause themselves problems. But the vulnerability is fixed, so it's good by me.

@hydrapolic

This comment has been minimized.

Show comment
Hide comment
@hydrapolic

hydrapolic Sep 25, 2017

Contributor

Yes that's a good point, I'll make it so.

Contributor

hydrapolic commented Sep 25, 2017

Yes that's a good point, I'll make it so.

hydrapolic added some commits Sep 25, 2017

app-admin/logstash-bin: bump to 5.5.3/5.6.1
Package-Manager: Portage-2.3.10, Repoman-2.3.3
app-admin/logstash-bin: drop old
Package-Manager: Portage-2.3.10, Repoman-2.3.3

@hydrapolic hydrapolic changed the title from app-admin/logstash-bin: bump to 5.5.3, drop old to app-admin/logstash-bin: bump to 5.5.3/5.6.1, drop old Sep 25, 2017

@gentoo-repo-qa-bot

This comment has been minimized.

Show comment
Hide comment
@gentoo-repo-qa-bot

gentoo-repo-qa-bot Sep 25, 2017

Collaborator

👍 All QA issues have been fixed!

Collaborator

gentoo-repo-qa-bot commented Sep 25, 2017

👍 All QA issues have been fixed!

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Sep 25, 2017

Contributor

Pulled, thank you!

Contributor

orlitzky commented Sep 25, 2017

Pulled, thank you!

@orlitzky orlitzky closed this Sep 25, 2017

@hydrapolic

This comment has been minimized.

Show comment
Hide comment
@hydrapolic

hydrapolic Sep 25, 2017

Contributor

Thanks @orlitzky and @ferki.

Contributor

hydrapolic commented Sep 25, 2017

Thanks @orlitzky and @ferki.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment