Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

Remove 'runserver' in favor of 'start' #26

Closed
kdmccormick opened this issue Jan 28, 2022 · 17 comments
Closed

Remove 'runserver' in favor of 'start' #26

kdmccormick opened this issue Jan 28, 2022 · 17 comments
Assignees

Comments

@kdmccormick
Copy link
Collaborator

kdmccormick commented Jan 28, 2022

Context

There are two ways to bring up services in dev mode:

  1. tutor dev runserver myservice
  2. tutor dev start [myservice1 myservice2 ...]

Command 1 has the advantage of accepting volume mounts and allowing debugger attachment.

Command 2 has the advantage of:

  • accepting multiple services, or
  • accepting no services, in which case all enabled services are started. This is helpful in that it doesn't "leave out" any important services, such as the MFE container. Relevant forum thread.

Are there other substantive differences? Could they be reconciled into a single command that supports all features?

Update

We have already:

  • added the ability for tutor [dev|local] start to mount volumes using the new --mount command
  • allowed debugger attachment for containers started by tutor dev start
  • marked tutor dev runserver as deprecated, since it no longer has any advantages over tutor dev start. This was done in: Deprecate 'runserver' in favor of 'start' #61

Acceptance Criteria

  • In order to give folks time to stop using runserver, wait until Tutor v14 is released.
  • Go through official plugins. For each plugin:
    • Ensure that the plugin is ported to the V1 API
    • Ensure that the plugin hooks into the COMPOSE_MOUNTS filter in order to automatically bind-mount folders as appropriate.
    • Replace any references to runserver with start
    • Replace any references to the --volume option with an equivalent usage of the --mount option.
  • Remove the implementation of tutor dev runserver.
@regisb
Copy link

regisb commented Feb 4, 2022

Are there other substantive differences?

  • runserver allocates a tty, which makes it possible to breakpoint() python code.
  • containers run as runserver are no longer reachable by containers run as start -- and this is a problem. For instance, with tutor dev runserver lms and tutor dev start -d cms then we can no longer login to the Studio, because the CMS makes an internal call to the LMS.

We can certainly do a better job at better documenting these commands, but I'm afraid that we need both.

@kdmccormick
Copy link
Collaborator Author

@regisb Is there any reason we wouldn't want start to allocate a tty like devstack's dev.up does?

@kdmccormick kdmccormick moved this from Ungroomed (Kyle) to Ungroomed (Régis) in Tutor DevEnv Adoption (OLD BOARD) Feb 15, 2022
@regisb
Copy link

regisb commented Feb 15, 2022

This is a very good point! I attempted this some time ago and it didn't work, though I can't remember why. I tried again just now and I now realise that I can docker attach to any container that was started with tty/stdin_open: true, thus allowing me to troubleshoot any container without the use of docker-compose run.
Thus, yes we can get rid of docker-compose run for long-running services, such as runserver. This is fantastic news, as it allows us to greatly simplify the CLI 🥳

EDIT: I just remembered that docker-compose run also allows bind-mounting volumes from the host. We would have to simplify the bind-mounting of folders and repositories (see #43) before we can really get rid of docker-compose run. (ping @kdmccormick)

@kdmccormick
Copy link
Collaborator Author

@regisb Awesome!

I'm excited that we can drop tutor dev runserver. Would you still want tutor dev run <service> <cmd> to exist for running one-off commands? For example: tests, migrations, management commands, and so on.

Or should those workflows be replaced with tutor dev start <service> && tutor dev exec <service> <cmd>?

@kdmccormick kdmccormick moved this from Ungroomed (Kyle) to Ungroomed (Régis) in Tutor DevEnv Adoption (OLD BOARD) Feb 15, 2022
@regisb
Copy link

regisb commented Feb 21, 2022

Would you still want tutor dev run to exist for running one-off commands? For example: tests, migrations, management commands, and so on.

Yes, dev run commands would still be useful. For instance I use the following commands very frequently:

  • tutor dev run lms bash
  • tutor dev run --no-deps lms ./manage.py lms shell
  • tutor local run ecommerce ./manage.py shell -c "from django.contrib.auth import get_user_model; get_user_model().objects.filter(email='[email protected]').update(is_staff=True, is_superuser=True)"

For one-off tasks, exec is still inferior to run because it ignores the environment variables defined in the docker-compose*.yml files.

@kdmccormick kdmccormick moved this from Ungroomed (Régis) to Ungroomed (Kyle) in Tutor DevEnv Adoption (OLD BOARD) Feb 22, 2022
@kdmccormick
Copy link
Collaborator Author

Yes, dev run commands would still be useful.

I agree. Since switching from devstack, I do appreciate how Tutor can run commands with spinning up a persistent container.

exec is still inferior to run because it ignores the environment variables defined in the docker-compose*.yml files.

I have not been able to recreate this. I tried adding an environment variable to docker-compose.yml:

$ vi $(tutor config printroot)/env/local/docker-compose.yml
    ...
    lms:
      image: docker.io/overhangio/openedx:13.1.5-nightly
      environment:
        SERVICE_VARIANT: lms
        UWSGI_WORKERS: 2
        SETTINGS: ${TUTOR_EDX_PLATFORM_SETTINGS:-tutor.production}
        MY_VAR: my_val  # <- added this line
    ...
$ tutor dev run lms printenv MY_VAR
my_val
$ tutor dev start -d lms
$ tutor exec lms bash
app@f4960a487696:~/edx-platform$ echo $MY_VAR
my_val
app@f4960a487696:~/edx-platform$

and found that both exec and run pick up on the new environment variable.

I'll update the acceptance criteria of this issue to keep run in the CLI because I think it's useful, but I'm still curious to understand what's going on with environment variables.

@kdmccormick kdmccormick changed the title As a developer, I want it to be clear how to start services Remove 'tutor dev runserver' in favor of 'tutor dev start' Feb 22, 2022
@kdmccormick kdmccormick moved this from Ungroomed (Kyle) to Groomed in Tutor DevEnv Adoption (OLD BOARD) Feb 22, 2022
@kdmccormick kdmccormick added enhancement Relates to new features or improvements to existing features refactor and removed for:2u-pilot labels Feb 22, 2022
@kdmccormick kdmccormick moved this from Groomed to Next Up in Tutor DevEnv Adoption (OLD BOARD) Mar 1, 2022
@kdmccormick kdmccormick moved this from Next Up to Groomed in Tutor DevEnv Adoption (OLD BOARD) Mar 2, 2022
@regisb
Copy link

regisb commented Mar 10, 2022

and found that both exec and run pick up on the new environment variable.

Right. I guess I was wrong about that. Now that you mention it, the actual problem that I faced in the past is that exec bypasses the Docker entrypoint. Sorry about the confusion. Tutor used to run images with heavy entrypoint scripts. This is mostly no longer the case, but it might still cause surprises from time to time.

@kdmccormick
Copy link
Collaborator Author

the actual problem that I faced in the past is that exec bypasses the Docker entrypoint.

Whew, that is a very confusing Docker nuance 😛

That sheds light on one weird behavior I've noticed: using tutor [local|k8s] exec, Django management commands almost work perfectly, except that DJANGO_SETTINGS_MODULE isn't set. After I run export DJANGO_SETTINGS_MODULE=[lms|cms].envs.tutor.production, everything works fine.

I think that it's because the Dockerfile sets DJANGO_SETTINGS_MODULE via an entrypoint script instead of setting it directly as using ENV. It seems like DJANGO_SETTINGS_MODULE is the only environment variable set this way.

Do you know of any reason we couldn't pull DJANGO_SETTINGS_MODULE into the Dockerfile?

@kdmccormick kdmccormick changed the title Remove 'tutor dev runserver' in favor of 'tutor dev start' Remove 'runserver' in favor of 'start' Mar 24, 2022
@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Apr 9, 2022

Work-in-progress PR: kdmccormick/tutor#7

This is effectively blocked by #43 , because we can't remove runserver until we have a replacement strategy for mounting local code, so I'm putting down that PR for the time being.

@kdmccormick
Copy link
Collaborator Author

@regisb I updated the acceptance criteria of this ticket, can you take a look to see if you agree?

@kdmccormick kdmccormick moved this from Backlog to Next Up in Tutor DevEnv Adoption Jun 23, 2022
@regisb
Copy link

regisb commented Jul 4, 2022

I updated the acceptance criteria of this ticket, can you take a look to see if you agree?

I do! Sorry about the delay :-/

@kdmccormick
Copy link
Collaborator Author

No worries, thanks!

@Carlos-Muniz If/when you need a new ticket, this one is ready to go.

@Carlos-Muniz
Copy link

@kdmccormick I'm going to tackle this ticket if you want to assign it to me.

@Carlos-Muniz
Copy link

Of the official Tutor Plugins:

@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Jul 19, 2022

I think a few of these services should add callbacks to the COMPOSE_MOUNTS filter in order to enable local repo auto-mounting:

  • tutor-xqueue: --mount xqueue should auto-mount to /openedx/xqueue in the xqueue and xqueue-job services.
  • tutor-forum: --mount cs_comments_service should auto-mount to /app/cs_comments_service in the forum and forum-job services.
  • tutor-notes: --mount edx-notes-api should auto-mount /app/edx-notes-api in the notes and notes-job services.

I agree about the rest of the services.

@Carlos-Muniz
Copy link

Local repo auto-mounting has been enabled for the following repos as requested:

  • tutor-forum
  • tutor-notes
  • tutor-xqueue

@kdmccormick
Copy link
Collaborator Author

New removal PR, on top of nightly instead of master: overhangio/tutor#718

Repository owner moved this from Groomed to Closed in Tutor DevEnv Adoption (OLD BOARD) Sep 6, 2022
Repository owner moved this from In Review to Closed in Tutor DevEnv Adoption Sep 6, 2022
@jmakowski1123 jmakowski1123 moved this to Done - To ship in future Named Release in Open edX Roadmap Oct 6, 2022
@jmakowski1123 jmakowski1123 moved this from Done - To ship in future Named Release to To ship in Olive in Open edX Roadmap Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants