Decouple log syncing from AWS and OpenStack roles #4398

xitij2000 · 2018-03-20T19:11:58Z

This PR splits out the log syncing bits of the openstack and AWS roles to their own roles such that either or both can be enabled.

The aim is to decouple the log syncing destination from AWS/OpenStack so a server deployed on OpenStack can sync to S3 and a server deployed to AWS can sync to SWIFT, or a server can sync to both.

This replaces #3522

Sandbox URL:

Merge deadline: We would love to get this into Hawthorn

Testing instructions:

Deploy a server using default log syncing settings
- Log syncing should be disabled by default
- Log rotation should still work
Deploy a server with COMMON_OBJECT_STORE_LOG_SYNC set, and AWS log sync settings configured
Logs should sync to S3
Deploy a server with COMMON_S3_LOG_SYNC set, and the AWS log sync settings configured
- Logs should sync to S3
Deploy a server with COMMON_SWIFT_LOG_SYNC set, and OpenStack settings configured.
- Logs should sync to SWIFT
Deploy a server with both `COMMON_S3_LOG_SYNC, and COMMON_SWIFT_LOG_SYNC`` set, and with both AWS and OpenStack settings configured.
- Logs should sync to both S3 and SWIFT

Reviewers

@pomegranited
edX reviewer[s] TBD

Settings

COMMON_SWIFT_LOG_SYNC: true

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 · 2018-03-20T19:12:06Z

Thanks for the pull request, @xitij2000! I've created OSPR-2311 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.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

nedbat · 2018-03-21T18:27:19Z

@edx/devops More deploy-agnosticism to review!

pomegranited

This works great @xitij2000 ! I'm ready to approve once my comments below have been addressed and re-tested.

Would like to see this run on an AWS deployment though.. @feanil is this covered by the automated tests, or do we need to set up an AWS sandbox?

I tested this on OpenStack deployments using the various configuration options, by deploying a new OpenStack Open edX appserver using each of the following configurations, and force-rotating the tracking logs to ensure that they were synced to the intended storage locations.
Permutations tested:
- No log syncing enabled: Log rotation works without error.
  Default behaviour; requires no special configuration.
- S3 only. Backwards compatibility with the COMMON_OBJECT_STORE_LOG_SYNC* variables is also preserved.
```
  COMMON_S3_LOG_SYNC: true
  COMMON_S3_LOG_SYNC_BUCKET: bucket-name
  AWS_S3_LOGS_ACCESS_KEY_ID: ***
  AWS_S3_LOGS_SECRET_KEY: ***
```
- SWIFT only. Backwards compatibility with the COMMON_OBJECT_STORE_LOG_SYNC_* variables is also preserved.
```
 COMMON_SWIFT_LOG_SYNC: true
 COMMON_SWIFT_LOG_SYNC_BUCKET: container-name
 SWIFT_LOG_SYNC_AUTH_URL: ***
 SWIFT_LOG_SYNC_PASSWORD: ***
 SWIFT_LOG_SYNC_REGION_NAME: ***
 SWIFT_LOG_SYNC_TENANT_NAME:  ***
 SWIFT_LOG_SYNC_USERNAME: ***
```
- SWIFT + S3. Achieved by combining the two stanzas above.
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation.

pomegranited · 2018-03-22T00:29:21Z

playbooks/roles/log_sync_s3/templates/send-logs-to-s3.j2

+{% else %}
+sec_grp=""
+{% endif %}
+


I understand that this change was introduced to avoid having unavailable/ show up in the S3 log path. But since it doesn't cause any issues, and this adds some unneeded complexity, I'd rather e419f58 and the associated b17204a were reverted.

The most important thing is that the log file paths are unique so existing log files are not overridden with different data. This behaviour is guaranteed from an OpenStack perspective by the ${instance_id}, and from an AWS perspective by the ${sec_grp}/${instance_id}.

pomegranited · 2018-03-22T00:41:28Z

playbooks/roles/aws/defaults/main.yml

-  - boto=="{{ common_boto_version }}"
-  - s3cmd==1.6.1
-
-aws_redhat_pkgs: []


Hmm.. AWS instances need these aws_debian_pkgs, aws_pip_pkgs, and aws_pip_pkgs installed regardless of whether they're doing log syncing.

Please reinstate these lines and the aws/task/main.yml lines below which use them, and instead, make the aws role a meta dependency of log_sync_s3, instead of the other way around.

We can ask edX for help deploying an AWS sandbox using this configuration, to make double sure it works, if the automated tests aren't already covering this.

@pomegranited I made log_sync_s3 a dependency of aws for backwards compatibility because currently people running that role might expect it to also set up log syncing.

Additionally, I don't know the side-effect of running the AWS role on a non-AWS instance, so I wasn't sure about making log syncing to S3 depend on that role.

I made log_sync_s3 a dependency of aws for backwards compatibility because currently people running that role might expect it to also set up log syncing.

Ok that makes sense, since we don't know what playbook people might be running the aws role from.

Thanks for the explanation!

pomegranited · 2018-03-22T01:10:53Z

playbooks/roles/log_sync_s3/defaults/main.yml

+AWS_S3_LOGS_SECRET_KEY: ""
+
+aws_s3_sync_script: "{{ vhost_dirs.home.path }}/send-logs-to-s3"
+aws_s3_logfile: "{{ vhost_dirs.logs.path }}/s3-log-sync.log"


These lines (and these, in the log_sync_swift role), are why these files get created in /edx/app/openstack when installed on Openstack VMs.

Since the file names are different, and symlinks are created, I don't think this is a problem to leave as-is.

pomegranited · 2018-03-22T01:37:33Z

playbooks/roles/log_sync_s3/defaults/main.yml

+
+aws_s3_sync_script: "{{ vhost_dirs.home.path }}/send-logs-to-s3"
+aws_s3_logfile: "{{ vhost_dirs.logs.path }}/s3-log-sync.log"
+aws_region: "us-east-1"


Irrelevant to this change, but I'd really like to see this variable be made configurable, to better support other AWS regions.

@pomegranited I've added a setting to override this.

Oo thank you! 😄

pomegranited · 2018-03-22T01:48:47Z

playbooks/roles/log_sync_s3/templates/send-logs-to-s3.j2

@@ -15,10 +15,9 @@ fi
 # Ensure the log processors can read without
 # running as root
 if [ ! -f "{{ aws_s3_logfile }}" ]; then
-  sudo -u syslog touch "{{ aws_s3_logfile }}"
-else
-  chown syslog.syslog "{{ aws_s3_logfile }}"


Thank you for fixing this! It's always bugged me 😄

pomegranited

👍 Thanks for addressing my comments @xitij2000 !

I re-tested this as described in Decouple log syncing from AWS and OpenStack roles #4398 (review)
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation

xitij2000 · 2018-03-23T08:10:40Z

@nedbat @feanil This PR is ready for review now.

nedbat · 2018-03-23T20:19:53Z

@xitij2000 Thanks. Could you please squash the commits?

xitij2000 · 2018-03-23T20:29:58Z

@nedbat Sure! Just rebased and squashed.

…ck roles Allow enabling either/both/neither log syncing roles irrespective of deployment platform

mduboseedx · 2018-05-04T20:40:03Z

@edx/devops I believe this is now ready for review

feanil · 2018-05-07T15:46:45Z

@mduboseedx I've updated the ticket status and prioritized it in our backlog.

For all concerned, I think we'll be able to start this review next week or so.

pomegranited · 2018-05-07T15:54:33Z

Awesome, thank you @feanil and @mduboseedx !

xitij2000 · 2018-05-07T18:19:06Z

Thanks!

xitij2000 · 2018-06-07T16:12:34Z

@feanil Were you able to review this then? I'd love to get some feedback in case we need to make changes.

Thanks!

nedbat · 2018-06-13T18:53:28Z

@edx/devops this is a request we'd like in Hawthorn.

nedbat · 2018-06-14T19:50:12Z

@edx/devops Can we get a response about when this might be reviewed? It's mentioned on the Hawthorn wiki page

nedbat · 2018-06-19T15:48:34Z

@edx/devops @fredsmith @jibsheet @coryleeio @jdmulloy @feanil Is there some other process we should be using to communicate about pull requests?

coryleeio · 2018-06-19T21:11:55Z

The communication channel is correct. Unfortunately, we haven't had time to review due to internal deadlines. I am bumping the review ticket and we will check it's priority in standup in the morning.

xitij2000 · 2018-07-19T21:55:45Z

We're closing this PR since we've decided to phase out our use of SWIFT in our Open edX deployments.

While it would be nice to see support for alternative storage backends in the Open edX platform, a combination of factors such as issues with our SWIFT provider, issues with the SWIFT integration in Open edX and the general lack of reactivity in getting fixes like this merged, make this work untenable for us.

We would be happy to help anyone willing to take over that work though. We could also work on removing support for SWIFT if preferred.
CC: @johnmark @nedbat

openedx-webhooks · 2018-07-19T21:55:56Z

@xitij2000 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Mar 20, 2018

openedx-webhooks added awaiting prioritization and removed needs triage labels Mar 21, 2018

pomegranited requested changes Mar 22, 2018

View reviewed changes

pomegranited reviewed Mar 22, 2018

View reviewed changes

xitij2000 force-pushed the kshitij/log_sync branch from b17204a to aceb544 Compare March 22, 2018 17:58

pomegranited approved these changes Mar 23, 2018

View reviewed changes

xitij2000 changed the title ~~WIP: Split out log sync into its own role~~ Decouple log syncing from AWS and OpenStack roles Mar 23, 2018

xitij2000 force-pushed the kshitij/log_sync branch from 9634254 to da6dfe7 Compare March 23, 2018 20:27

xitij2000 force-pushed the kshitij/log_sync branch from da6dfe7 to d7c96e7 Compare April 2, 2018 14:18

Decopuple log AWS S3 and SWIFT syncing setup from the AWS and OpenSta…

892568e

…ck roles Allow enabling either/both/neither log syncing roles irrespective of deployment platform

xitij2000 force-pushed the kshitij/log_sync branch from d7c96e7 to 892568e Compare April 9, 2018 14:35

openedx-webhooks added engineering review and removed awaiting prioritization labels May 7, 2018

xitij2000 closed this Jul 19, 2018

Agrendalath mentioned this pull request Sep 20, 2018

Add optional aws role integration to edx_sandbox #4793

Merged

8 tasks

openedx-webhooks added rejected and removed engineering review labels Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple log syncing from AWS and OpenStack roles #4398

Decouple log syncing from AWS and OpenStack roles #4398

xitij2000 commented Mar 20, 2018 •

edited

openedx-webhooks commented Mar 20, 2018

nedbat commented Mar 21, 2018

pomegranited left a comment

pomegranited Mar 22, 2018

pomegranited Mar 22, 2018

xitij2000 Mar 22, 2018 •

edited

pomegranited Mar 22, 2018

pomegranited Mar 22, 2018

pomegranited Mar 22, 2018

xitij2000 Mar 22, 2018

pomegranited Mar 22, 2018

pomegranited Mar 22, 2018

pomegranited left a comment

xitij2000 commented Mar 23, 2018

nedbat commented Mar 23, 2018

xitij2000 commented Mar 23, 2018

mduboseedx commented May 4, 2018

feanil commented May 7, 2018

pomegranited commented May 7, 2018

xitij2000 commented May 7, 2018

xitij2000 commented Jun 7, 2018

nedbat commented Jun 13, 2018

nedbat commented Jun 14, 2018

nedbat commented Jun 19, 2018

coryleeio commented Jun 19, 2018

xitij2000 commented Jul 19, 2018

openedx-webhooks commented Jul 19, 2018

Decouple log syncing from AWS and OpenStack roles #4398

Decouple log syncing from AWS and OpenStack roles #4398

Conversation

xitij2000 commented Mar 20, 2018 • edited

openedx-webhooks commented Mar 20, 2018

nedbat commented Mar 21, 2018

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 Mar 22, 2018 • 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

pomegranited left a comment

Choose a reason for hiding this comment

xitij2000 commented Mar 23, 2018

nedbat commented Mar 23, 2018

xitij2000 commented Mar 23, 2018

mduboseedx commented May 4, 2018

feanil commented May 7, 2018

pomegranited commented May 7, 2018

xitij2000 commented May 7, 2018

xitij2000 commented Jun 7, 2018

nedbat commented Jun 13, 2018

nedbat commented Jun 14, 2018

nedbat commented Jun 19, 2018

coryleeio commented Jun 19, 2018

xitij2000 commented Jul 19, 2018

openedx-webhooks commented Jul 19, 2018

xitij2000 commented Mar 20, 2018 •

edited

xitij2000 Mar 22, 2018 •

edited