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

Fix syncing logs when shutting down a server #4807

Merged
merged 2 commits into from Apr 10, 2019

Conversation

Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented Sep 29, 2018

This fixes syncing logs when rebooting, powering off or halting the instance.
Ubuntu adopted systemd in 15.04 version. Currently there is an init script (running on supervisor's exit) which does not work for xenial and more recent distributions. Therefore my proposition is to use systemd service directive for sending logs before shutdown.

I decided to run the syncing script on exit of syslog.service, because it allows syncing logs before rsyslogd is terminated. Attaching it to supervisor or running it as a separate service didn't produce deterministic results.

I've also extracted commands for syncing logs from playbooks/roles/vhost/templates/etc/init/sync-on-stop.conf.j2 to playbooks/roles/vhost/templates/sync-logs-on-exit.j2 to avoid code repetition.

JIRA ticket: OSPR-2639

Merge deadline: ASAP - it would need to go into Hawthorn

Testing instructions (example for S3, but can be easily adapted for other storages):
ACTIONS = {REBOOT, POWER OFF, HALT}
For each ACTION in $ACTIONS:

  • Create an instance with this config.
  • Add something to logs: e.g. echo TEST | sudo tee --append /edx/var/log/tracking/tracking.log.
  • $ACTION the server.
  • List the logs: s3cmd --access_key $AWS_S3_LOGS_ACCESS_KEY_ID --secret_key $AWS_S3_LOGS_SECRET_KEY ls s3://$COMMON_OBJECT_STORE_LOG_SYNC_BUCKET/$COMMON_OBJECT_STORE_LOG_SYNC_PREFIX$(ec2metadata --security-groups | head -1)/$(ec2metadata --instance-id)-$(ec2metadata --local-ipv4)/.
  • Get the log file (it should be the last one): s3cmd --access_key $AWS_S3_LOGS_ACCESS_KEY_ID --secret_key $AWS_S3_LOGS_SECRET_KEY get s3://$COMMON_OBJECT_STORE_LOG_SYNC_BUCKET/$COMMON_OBJECT_STORE_LOG_SYNC_PREFIX$(ec2metadata --security-groups | head -1)/$(ec2metadata --instance-id)-$(ec2metadata --local-ipv4)/$FILE_NAME.
  • Ensure that the downloaded file contains the sent phrase: e.g. with: zgrep TEST $DOWNLOADED_FILE.

Reviewers


Make sure that the following steps are done before merging:

  • A DevOps team member has approved the PR.
  • Are you adding any new default values that need to be overridden when this change goes live? If so:
    • Update the appropriate internal repo (be sure to update for all our environments)
    • If you are updating a secure value rather than an internal one, file a DEVOPS ticket with details.
    • Add an entry to the CHANGELOG.
  • If you are making a complicated change, have you performed the proper testing specified on the Ops Ansible Testing Checklist? Adding a new variable does not require the full list (although testing on a sandbox is a great idea to ensure it links with your downstream code changes).

@openedx-webhooks
Copy link

Thanks for the pull request, @Agrendalath! I've created OSPR-2639 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 29, 2018
@Agrendalath Agrendalath changed the title Add systemd directive for sending logs Fix syncing logs when shutting down a server Sep 29, 2018
@natabene
Copy link
Contributor

natabene commented Oct 1, 2018

This is great, thank you for your contribution. @edx/devops can you take a look at this?

Copy link
Contributor

@rocioar rocioar left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: Run a server, wrote the logs, performed a reboot and then confirmed the logs with were updated in s3.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@Agrendalath
Copy link
Member Author

@nedbat, are there any updates on this one?

@Agrendalath
Copy link
Member Author

@nedbat, is there anything new about this PR?

@nedbat
Copy link
Contributor

nedbat commented Nov 9, 2018

@Agrendalath I'm not directly involved with this pull request. @mduboseedx and @natabene are shepherding it, but it needs attention from our devops team for review.

@Agrendalath
Copy link
Member Author

@nedbat, all right, thanks for info!

@natabene
Copy link
Contributor

@Agrendalath This is waiting on the devops team. I will check with them again.

@Agrendalath
Copy link
Member Author

@natabene, @nedbat, is this something that could make it into Ironwood?

@natabene
Copy link
Contributor

@feanil Could you give this a look? Ned would take it into Ironwood if you can review. Thanks.

@Agrendalath
Copy link
Member Author

@feanil, are there any updates on this?

@natabene
Copy link
Contributor

@edx/devops Could you give this a look as soon as you can?

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the change. Rather than making changes directly to the RSyslog unit file, I think it makes more sense to create a new service that is started before rsyslog and stopped after systlog.

I don't want to make changes to the unit files of services that ship from standard packages if at all possible.

Take a look here for someone who was trying to solve the same problem: https://unix.stackexchange.com/questions/347415/stop-systemd-service-after-another-at-shutdown-and-reboot

@Agrendalath
Copy link
Member Author

Agrendalath commented Mar 13, 2019

@feanil, good point, thanks for catching it!

Rather than making changes directly to the RSyslog unit file, I think it makes more sense to create a new service that is started before rsyslog and stopped after systlog.

Just a nit here: after testing it I believe that we should start this service after the rsyslog service. If its ExecStop ran after closing rsyslog (in the reverse order at shutdown), the logrotate wouldn't be able to compress the logs and they would be uploaded to a storage as uncompressed files.

I addressed the review and moved syncing logs to a separate systemd service.

@Agrendalath
Copy link
Member Author

@feanil, are there any updates on this?

@natabene
Copy link
Contributor

@Agrendalath Sorry, we have not had a chance to look into this yet.

@Agrendalath
Copy link
Member Author

@feanil, did you have a chance to look at this?
cc: @natabene

@natabene
Copy link
Contributor

natabene commented Apr 8, 2019

@Agrendalath Is this time sensitive or has a specific deadline?

@Agrendalath
Copy link
Member Author

@natabene, this prevents logs from being lost in cases mentioned in the PR's description, so the sooner the better.

@natabene
Copy link
Contributor

@bderusha @feanil If you can look at this sooner than later it would be great.

@feanil feanil merged commit 92d634a into openedx:master Apr 10, 2019
@openedx-webhooks
Copy link

@Agrendalath 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath Agrendalath deleted the agrendalath/xenial-log-sync-on-exit branch April 15, 2019 11:45
@Agrendalath
Copy link
Member Author

@nedbat, can this fix be cherry-picked into open-release/ironwood.master branch?
cc: @natabene

@nedbat
Copy link
Contributor

nedbat commented Apr 16, 2019

I've cherry-picked these two commits onto open-release/ironwood.master.

@Agrendalath
Copy link
Member Author

Great, thank you @nedbat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants