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.

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

feat(init): use dinit instead of supervisor #69

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Oct 18, 2022

Resolves #62.

Before this PR, there was a short delay (5s) when starting jmwalletd to wait for tor port bindings, as the previous service manager (supervisord) has no concept of dependencies. This is brittle and error-prone.

After this PR is applied, supervisord has been replaced by dinit which can build service hierarchies and making it possible for jmwalletd and ob-watcher to actively wait for tor to start successfully.

Also updates docker image versions:

  • build image: alpine from 3.15 to 3.16.2
  • runtime image debian from bullseye-20220801-slim to bullseye-20221004-slim

How to test?

Use the image published from this branch called dinit in your regtest environment by changing label master to dinit in dockerfile-deps/joinmarket/webui-standalone/Dockerfile and rebuild your setup with npm run regtest:build.
You should see instance 2 and 3 come up normally and access their UI on http://localhost:29080 and http://localhost:30080.
In the console you see smth like:

jm_regtest_joinmarket2          | [  OK  ] nginx
jm_regtest_joinmarket2          | [  OK  ] tor
jm_regtest_joinmarket2          | [  OK  ] jmwalletd
jm_regtest_joinmarket2          | [  OK  ] ob-watcher
jm_regtest_joinmarket2          | [  OK  ] boot

@theborakompanioni theborakompanioni marked this pull request as ready for review October 19, 2022 17:08
@dergigi
Copy link
Contributor

dergigi commented Oct 20, 2022

Good stuff! I'll do my best to test this properly today.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Oct 20, 2022

Good stuff! I'll do my best to test this properly today.

Thanks.
Please note that one feature got lost when switching to dinit: automatic log rotation.
Should this be reimplemented within this PR, or should a follow-up PR be created?

@dergigi
Copy link
Contributor

dergigi commented Oct 20, 2022

I'm usually in favor of doing separate, smaller PRs, but I don't have a strong preference here. You decide! 😁

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Nice! Code looks good to me, and I tested it successfully.

I didn't know about dinit. Looks solid enough, even though it is not super-widely used yet and still alpha (according to their releases). The service hierarchy is indeed very neat, though.

I don't think anything speaks against resolving the "wait for other service to start" issue in this way.

tACK ✅

standalone/Dockerfile Show resolved Hide resolved
@theborakompanioni
Copy link
Collaborator Author

Nice! Code looks good to me, and I tested it successfully.

💪

I didn't know about dinit. Looks solid enough, even though it is not super-widely used yet and still alpha (according to their releases). The service hierarchy is indeed very neat, though.

Yes, let's see if it fits the purpose. I too found no issues so far during testing, but will closely follow ongoing development and updates. Currently, it seems like a solid choice.

I don't think anything speaks against resolving the "wait for other service to start" issue in this way.

👍

tACK white_check_mark

Will merge, but keep on testing an report back in case anything comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sunset supervisord as service manager
2 participants