-
Notifications
You must be signed in to change notification settings - Fork 10
Set up docker with dispatcherd for local development #18
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
base: main
Are you sure you want to change the base?
Conversation
19a6fc5
to
ad9b029
Compare
Hi @abikouo, I couldn't get the migrate command to work with manage.py inside src/pattern_service/. I tested moving manage.py to the src/ directory and adjusting its references, which fixed all the builds (docker build and docker compose) and migration. Is there a step I'm overlooking, or should manage.py be located in the src/ directory? |
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.
In addition to the inline comments, I would suggest we remove the following from this PR:
- Movement of code to the
src
directory. There are ongoing conversations org-wide about how we want to standardize platform service repository structure; let's hold off on making big changes in this regard until we know what those decisions are. - Dependency management with Poetry. This isn't standard for platform services either; let's stick with pyproject.toml for now.
- Bash files to run tests and mypy, this shouldn't be necessary.
- Addition of copyright statements to individual files. We may want to do this; however I would suggest we do it in a single PR to all existing files because adding it in this PR to only some files is introducing inconsistency across the repo. I would also like to verify what the org-wide guideline is for this.
tools/docker/README.md
Outdated
@@ -0,0 +1,51 @@ | |||
# Docker Compose for Development |
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.
There is a push to move towards podman over docker within Red Hat generally; let's try to refer either to podman or just containers generally instead of specifying docker.
@hakbailey I have updated the PR based on what you did on #25. #25 has to be merged first. |
tools/docker/Dockerfile
Outdated
|
||
ENV PATTERN_SERVICE_MODE=development | ||
|
||
RUN python3.11 manage.py migrate |
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 suggest you run the migration when the container starts because this adds a read-only layer.
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 was able to create a read-write file for the db, what is the error on your side?
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 wasn't able to add anything else to the db because the migration was run in a read only layer. When creating a pattern I was getting something like "read-only database".
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.
django.db.utils.OperationalError: attempt to write a readonly database
|
e14894d
to
b45ca24
Compare
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.
It would be good to have the test
endpoint submit a task to the dispatcher worker so we can verify this functionality; something simple like this would be fine.
I think it would also be valuable to update the logging section of the development settings to include dispatcherd DEBUG logs.
pattern_service/settings/defaults.py
Outdated
DISPATCHER_CONFIG = { | ||
"version": 2, | ||
"service": { | ||
"pool_kwargs": {"min_workers": 2, "max_workers": 12, "scaledown_wait": 15}, |
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'm not sure we know yet what reasonable values will be for any of this. I would vote that we leave the default values for everything that we don't have to explicitly define here and then update them as needed. The settings used for EDA may not be relevant to our service, they're doing different kinds of async tasks.
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 have updated the configuration accordingly
|
||
|
||
def override_dispatcher_settings(loaded_settings: Dynaconf) -> None: | ||
feature_dispatcherd = convert_to_bool(loaded_settings.get("FEATURE_DISPATCHERD")) |
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'm not sure why this conditional is here either, FEATURE_DISPATCHERD
isn't a setting in our app which means this function doesn't currently do anything. The way our settings are written write now, I'm not sure this override function is even needed. We will need to add it when we start pulling some of these values from env variables or files, but at the moment it doesn't seem necessary.
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 override here is to switch between SQLite DB and PostgreSQL DB while running with the dispatcher. We can remove that if we assume that the default behavior is with the dispatcher.
This will also break the current deployment on aap-dev.
tools/docker/docker-compose.yaml
Outdated
@@ -0,0 +1,84 @@ | |||
x-environment: &common-env | |||
PATTERN_SERVICE_DB_HOST: postgres |
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 env variables used for anything?
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 environment variables will be part of the settings without the PATTERN_SERVICE
prefix. They are used to configure the DB.
tools/docker/docker-compose.yaml
Outdated
image: localhost/ansible-pattern-service-worker | ||
environment: | ||
<< : *common-env | ||
PATTERN_SERVICE_DISPATCHER_NODE_ID: pattern-service-worker |
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.
Does this env variable get used for anything?
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 is the node ID that will be set in the dispatcher config.
Refer to AAP-47113
Note: #25 has to be merged first