-
Notifications
You must be signed in to change notification settings - Fork 969
feat: add playbooks for enterprise-access and enterprise-subsidy #7107
feat: add playbooks for enterprise-access and enterprise-subsidy #7107
Conversation
A new playbook and role have been added for deploying enterprise-access based on the existing enterprise_catalog playbook and role.
A new playbook and role have been added for deploying enterprise-subsidy based on the existing playbook and role for enterprise_catalog.
As enterprise-subsidy uses Django 4.2, it doesn't support the default MemchachedCache backend provided by edx_django_service. So, this sets the default memcached backend to PyMemcacheCache[1]. [1]: https://docs.djangoproject.com/en/4.2/topics/cache/#memcached
* Bump the Vagrant box from Xenial to Focal * Update the IP address of the Vagrant box based on the docs[1] > On Linux, macOS and Solaris Oracle VM VirtualBox will only allow IP addresses in 192.168.56.0/21 range to be assigned to host-only adapters. [1]: https://www.virtualbox.org/manual/ch06.html#network_hostonly
Thanks for the pull request, @tecoholic! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tecoholic, some of the comments apply to both roles. I didn't duplicate them for readability, though.
Also, we should update the CHANGELOG.md
file.
playbooks/enterprise_access.yml
Outdated
- role: insightvm_agent | ||
when: COMMON_ENABLE_INSIGHTVM_AGENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: why do we want this here and in the playbooks/enterprise_subsidy.yml
playbook? Shouldn't it be installed explicitly with the insightvm_agent
playbook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Agrendalath Since I created the playbooks and the roles using the enterprise-catalog
playbook and role as a reference, these were carried over. These can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these.
nginx_default_sites: | ||
- enterprise_subsidy | ||
- role: memcache | ||
when: ENTERPRISE_SUBSIDY_MEMCACHE_ENABLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be undefined. Besides, do we need to deploy Memcached here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker-compose.yml file of enterprise-subsidy does run memcache as a service. So, I added this flag to make it available as an option.
You are right, I missed properly mapping the CACHE config to this flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both the services use memcache in their setup, I have added it to both the playbooks and used the default localhost:11211
in the config. Also included a comment that if the flag is turned off, then the other related settings need to be changed as well.
So, this should allow for disabling memcache installation and setting the config to a totally different backend, or pointing to a remote memcahce.
It makes sense to me, but I am not sure if this is the best way to do it. Kindly change it, if I am doing something wrong here.
enterprise_access_environment: | ||
ENTERPRISE_ACCESS_CFG: '{{ COMMON_CFG_DIR }}/{{ enterprise_access_service_name }}.yml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be overridden to specify more env variables. Why don't we add ENTERPRISE_ACCESS_ENVIRONMENT_EXTRA: {}
and pass it like edx_django_service_environment_extra: '{{ enterprise_access_environment | combine(ENTERPRISE_ACCESS_ENVIRONMENT_EXTRA) }}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
MEDIA_ROOT: '{{ ENTERPRISE_ACCESS_MEDIA_ROOT }}' | ||
MEDIA_URL: '{{ ENTERPRISE_ACCESS_MEDIA_URL }}' | ||
|
||
# TODO: Let edx_django_service manage ENTERPRISE_ACCESS_STATIC_ROOT in phase 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I know it was copied from the license_manager
, but I have no idea what "phase 2" is. Instead of adding more TODOs, maybe we could extend the comment in the license_manager
to indicate that this also applies to two other roles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it from the enterprise_catalog
. So I have added reference to these 2 services in the enterprise_catalog's comment and removed them here.
edx_django_service_decrypt_config_enabled: '{{ ENTERPRISE_SUBSIDY_DECRYPT_CONFIG_ENABLED }}' | ||
edx_django_service_copy_config_enabled: '{{ ENTERPRISE_SUBSIDY_COPY_CONFIG_ENABLED }}' | ||
edx_django_service_migration_check_services: '{{ enterprise_subsidy_service_name }},{{ enterprise_subsidy_service_name }}_workers' | ||
edx_django_service_enable_celery_workers: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enterprise-subsidy
does not seem to be using Celery:
enterprise-subsidy on main via 🐍 v3.11.7 ➜ ag "celery"
docs/decisions/0003-fulfillment-and-corrective-policies.rst
52:as an asynchronous celery task that retries indefinitely (or up to some fairly high maximum). The fulfillment action
pylintrc
72:load-plugins = edx_lint.pylint,pylint_django,pylint_celery
requirements/dev.txt
16: # pylint-celery
435: # pylint-celery
438:pylint-celery==0.3
449: # pylint-celery
requirements/doc.txt
20: # pylint-celery
413: # pylint-celery
416:pylint-celery==0.3
427: # pylint-celery
requirements/quality.txt
16: # pylint-celery
387: # pylint-celery
390:pylint-celery==0.3
401: # pylint-celery
requirements/test.txt
15: # pylint-celery
317: # pylint-celery
320:pylint-celery==0.3
326: # pylint-celery
requirements/validation.txt
18: # pylint-celery
493: # pylint-celery
496:pylint-celery==0.3
510: # pylint-celery
enterprise-subsidy on main via 🐍 v3.11.7 ➜ fd tasks
enterprise-subsidy on main via 🐍 v3.11.7 ➜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. 👍 Removed all celery/worker related config and set the relevant flag edx_django_service_enable_celery_workers
to false
. I remember thinking one of the services doesn't have a background worker when starting to work on these playbooks, but got lazy and forgot midway.
- queue: '{{ enterprise_access_celery_default_queue }}' | ||
concurrency: 4 | ||
monitor: True | ||
enterprise_access_workers: "{{ ENTERPRISE_ACCESS_CELERY_WORKERS }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: why don't we pass ENTERPRISE_ACCESS_CELERY_WORKERS
directly to edx_django_service_workers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed the redundant variable.
# NOTE: These variables are only needed to create the demo site (e.g. for sandboxes) | ||
ENTERPRISE_ACCESS_LMS_URL_ROOT: !!null | ||
ENTERPRISE_ACCESS_DISCOVERY_API_URL: !!null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these variables be used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these.
ENTERPRISE_ACCESS_SOCIAL_AUTH_EDX_OAUTH2_SECRET: 'enterprise-access-sso-secret' | ||
ENTERPRISE_ACCESS_BACKEND_SERVICE_EDX_OAUTH2_KEY: 'enterprise-access-backend-service-key' | ||
ENTERPRISE_ACCESS_BACKEND_SERVICE_EDX_OAUTH2_SECRET: 'enterprise-access-backend-service-secret' | ||
ENTERPRISE_ACCESS_SOCIAL_AUTH_REDIRECT_IS_HTTPS: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one also seems to be unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
config.vm.box = "xenial64" | ||
config.vm.box_url = "http://files.vagrantup.com/xenial64.box" | ||
config.vm.box = "ubuntu/focal64" | ||
# config.vm.box_url = "http://files.vagrantup.com/xenial64.box" | ||
|
||
config.vm.network :private_network, ip: "192.168.33.20" | ||
config.vm.network :private_network, ip: "192.168.56.20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Agrendalath These were added for testing. Probably needed if you are running the Vagrant tests locally as VirtualBox only supports the 192.168.56.0/21
address space by default and Vagrant no longer seems to be using the files.vagrantup.com domain to serve the VM boxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are in a separate commit 5cfa290. If these are not required for testing, we can just rebase and drop that one commit. I needed them for testing, so I am leaving it for now, in case someone else faces issues with the testing instructions, running Vagrant.
Since enterprise_access's settings file doesn't load the URLs for other services like the LMS, discovery, enterprise-catalog..etc., from the environment variables, the extra variable introduced in this commit provides a way to set those values in the .yml config.
Setting the edx_django_service_cors_whitelist doesn't seem to be putting the values in the generated yml config file. So adding it to the config overrides of enterprise-access and enterprise-subsidy.
By default the edx_django_service puts only the user-profile URL in the EDX_DRF_EXTENSIONS value. This overrides the user attribute mapping for enterprise-access and enterprise-subsidy. This commit adds the user-attribute mappings to the config value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tecoholic, a quick reminder about updating the changelog.
For some reason, I'm unable to resolve conversations in this PR, so I left +1s on your comments.
👍
- I tested this: we have already used these playbooks on Stage
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation:
⚠️ - I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository: n/a
@@ -55,7 +57,10 @@ ENTERPRISE_ACCESS_MYSQL_HOST: 'localhost' | |||
ENTERPRISE_ACCESS_MYSQL_USER: 'entaccess001' | |||
ENTERPRISE_ACCESS_MYSQL_PASSWORD: 'password' | |||
|
|||
ENTERPRISE_ACCESS_MEMCACHE: [ 'memcache' ] | |||
ENTERPRISE_ACCESS_MEMCACHE: [ 'localhost:11211' ] | |||
# The memcache config of edx_django_service is overriden in defaults/main.yml to use PyMemcacheCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# The memcache config of edx_django_service is overriden in defaults/main.yml to use PyMemcacheCache | |
# The memcache config of edx_django_service is overridden in defaults/main.yml to use PyMemcacheCache |
Configuration Pull Request
Make sure that the following steps are done before merging:
Description
This PR adds new playbooks and roles for deploying enterprise-access and enterprise-subsidy natively. The roles were created using the
enterprise-catalog
role as a reference.Testing
The roles were tested following the Vagrant based testing instructions using VirtualBox as the backend on a Linux machine.
export VAGRANT_ANSIBLE_ROLE=enterprise_access
orexport VAGRANT_ANSIBLE_ROLE=enterprise_subsidy
export VAGRANT_ANSIBLE_VARS_FILE=/path/to/file-with-above-content
vagrant up
and verify that the role completes successfully.Verifying services are running correctly
vagrant ssh -- -NL 8270:localhost:8270
allows us to verify that the service is up. Visit http://localhost:8270/admin/ to get the login form. The static files won't load as they depend on the Nginx from the playbook.vagrant ssh -- -NL 8280:localhost:8280
the admin page refuses to render because it tries to load a Waffle Flag from the database which isn't deployed by this role.