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.

Sign up for GitHub

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

Jump to bottom

feat(docker): Update docker-compose to use lily worker pattern #1044

Merged
merged 3 commits into from
Aug 29, 2022
Merged

feat(docker): Update docker-compose to use lily worker pattern #1044

merged 3 commits into from
Aug 29, 2022

Conversation

placer14
Copy link
Contributor

@placer14 placer14 commented Aug 23, 2022 •
edited
Loading

Closes #1034

  • remove daemon and replace w notifier and worker containers
  • notifier and worker each have it's own envvars and config
  • redis instance is brought up first and both notifier and worker successfully register their queues
  • prometheus should be target scraping both though I haven't tested it (or jaeger/tasks as I don't have a repo handy)

@placer14 placer14 self-assigned this Aug 23, 2022
@placer14
Copy link
Contributor Author

@frrist This has the basis for what you asked, but we can't use (docker-compose) replicas for the worker. (We can't assign their ports (API and prom) uniquely and only the first binding succeeds.) The next best thing we can do is make additional boilerplate for worker-2 and worker-3 that is commented out but can be enabled and works. What do you think?

Could you also test this out with a repo and see if anything here doesn't make sense?

placer14
placer14 commented Aug 23, 2022
View reviewed changes
docker-compose.yml
lily_notifier_data: {}
lily_worker_data: {}
lily_notifier_tmp: {}
lily_worker_tmp: {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lily_*_tmp volumes will persist the params so you don't have to keep downloading them. <3

Copy link
Member

Choose a reason for hiding this comment

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

If it's not already, could the daemon's repo also be persisted? (so we don't redownload the snapshots)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both worker and notifier have their repo's persisted.

frrist
frrist reviewed Aug 24, 2022
View reviewed changes
build/lily/docker_init.sh
@@ -22,6 +22,6 @@ else
lily init
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on the line, but above on line 11, could we add an elif with something like:

elif [[ ! -z "${LILY_DOCKER_INIT_IMPORT_SNAPSHOT_PATH}" ]]; then
    echo "Importing snapshot from ${LILY_DOCKER_INIT_IMPORT_SNAPSHOT_PATH}"
    lily init --import-snapshot=${LILY_DOCKER_INIT_IMPORT_SNAPSHOT_PATH}

then we could optionally specify locally mounted snapshots in the compose file: https://github.com/filecoin-project/lily/pull/1044/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the envvar get a default assignment if found to be empty.

frrist
frrist reviewed Aug 24, 2022
View reviewed changes
build/lily/notifier.env Outdated Show resolved Hide resolved
build/lily/notifier.env Outdated

# Enable IMPORT_SNAPSHOT below to use snapshot on lily startup
#_LILY_DOCKER_INIT_IMPORT_MAINNET_SNAPSHOT=true

# Debugging options
#LILY_TRACING=true
LILY_PROMETHEUS_PORT="prometheus:9090"
LILY_PROMETHEUS_PORT="localhost:9090"
Copy link
Member

Choose a reason for hiding this comment

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

im not sure this works, I wasn't able to scrap any metrics from the nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a change if you want to try this again?

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022 •
edited
Loading

Codecov Report

Merging #1044 (194d34e) into master (2dac569) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1044   +/-   ##
======================================
  Coverage    34.4%   34.4%           
======================================
  Files          44      44           
  Lines        2925    2925           
======================================
  Hits         1008    1008           
  Misses       1821    1821           
  Partials       96      96

@placer14 placer14 force-pushed the mg/docker-compose-w-distributed-lily branch from 2ce0084 to 194d34e Compare August 24, 2022 23:07
@placer14 placer14 marked this pull request as ready for review August 25, 2022 02:22
@placer14
Copy link
Contributor Author

I confirmed getting metrics from worker and notifier into prom and seeing them in grafana. 🤝

@placer14 placer14 requested a review from frrist August 25, 2022 02:23
@placer14 placer14 changed the title (WIP) feat(docker): Update docker-compose to use lily worker pattern feat(docker): Update docker-compose to use lily worker pattern Aug 25, 2022
frrist
frrist approved these changes Aug 25, 2022
View reviewed changes
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

LOVE IT!! Works great, good to merge after last few comments addressed

build/lily/notifier.env Outdated
@@ -5,16 +5,20 @@ LILY_REPO=/var/lib/lily
#LILY_CONFIG=/var/lib/lily/config.toml

# Postgres URL may be overridden
#LILY_STORAGE_POSTGRESQL_DB_URL=postgres://user:pass@host:port/database?options
LILY_STORAGE_POSTGRESQL_DB_URL=postgres://postgres@timescaledb:5432/postgres?sslmode=disable
Copy link
Member

Choose a reason for hiding this comment

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

need the password field: postgres:password@timescaledb:5432/postgres?sslmode=disable

build/lily/notifier_config.toml
@@ -0,0 +1,14 @@
[API]
Copy link
Member

Choose a reason for hiding this comment

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

needs a storage config

@placer14 placer14 merged commit da510c3 into master Aug 29, 2022
@placer14 placer14 deleted the mg/docker-compose-w-distributed-lily branch August 29, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frrist frrist frrist approved these changes

Assignees

@placer14 placer14

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create functional docker-compose for running lily with distributed indexing
3 participants
@placer14 @codecov-commenter @frrist