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

fix: add sidkiq&nginx docker image on eb #235

Merged
merged 12 commits into from
Jun 7, 2021

Conversation

komtaki
Copy link
Contributor

@komtaki komtaki commented May 26, 2021

🎩 What? Why?

本体をdocker化した時に、sidkiqが漏れたので、その対応。

これまでデプロイ後に、ebの設定で同サーバー内で自動でプロセスが起動するようになっていた。しかしDockerプラットフォームでそれは出来ない。そのため、同じコンテナイメージを使用してマルチコンテナ構成で起動するようにした。

その場合、nginxプロキシを自分で設定する必要がある。そのため、nginxコンテナも追加した。

https://docs.aws.amazon.com/ja_jp/elasticbeanstalk/latest/dg/create_deploy_docker.container.console.html

sidekiq

  1. こちらの警告が毎回出ていたので設定追加しました。
    https://qiita.com/shimadama/items/3dee21f179b8100d3209

  2. stagingでsidekiqを追加した場合、メモリ使用量が増加してwarning状態になりました。そのためインスタンスタイプを上げました。

  3. rails 本体もですが、送付に失敗したhtmlメール本文&ヘッダー全部とかがログに出力されてました。結果、パフォーマンスに影響していそうに見えたので、ログレベルをあげました。

所感

sidekiqはメモリ使用量が多く、webサーバーのスケールとスケールのタイミングが異なる。アクセスが増えたからと言って、キューの処理量もそれに応じて、増えるわけではない。このレベルのサービスなら1プロセスで十分。

そのため、WEBサーバーと同じパッケージに入れるのは非効率的だと感じた。パフォーマンスを追求するならfargateとかに移行するべきだと思う。

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • If there's a new public field, add it to GraphQL API
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

📷 Screenshots (optional)

Description

@komtaki komtaki force-pushed the hotfix/sidkiq-docker branch 3 times, most recently from 8acf1d3 to 2a5f739 Compare May 27, 2021 13:16
@komtaki komtaki changed the title fix: add sidkiq docker image on eb fix: add sidkiq&nginx docker image on eb May 27, 2021
@komtaki komtaki force-pushed the hotfix/sidkiq-docker branch from 2a5f739 to f93dbac Compare May 27, 2021 13:22
@komtaki komtaki marked this pull request as ready for review May 27, 2021 13:48
@komtaki komtaki force-pushed the hotfix/sidkiq-docker branch 9 times, most recently from 098765c to 61aca77 Compare May 30, 2021 14:23
@komtaki komtaki requested a review from ayuki-joto June 4, 2021 12:47
@komtaki komtaki force-pushed the hotfix/sidkiq-docker branch 2 times, most recently from 2380dc4 to 23f8653 Compare June 4, 2021 14:41
@komtaki komtaki force-pushed the hotfix/sidkiq-docker branch from 23f8653 to ce5441b Compare June 4, 2021 14:44
@ayuki-joto
Copy link
Collaborator

ちなみにですが、一つのコンテナで起動時にsidekiqも動かしてしまう(以下のリンクのようにsuperviserdなどで管理するもしくは複数コマンドを実行する)でも解決できる気がするのですが、こちらを採用しない理由としては、やはり1コンテナ1プロセスを保ちたいからでしょうか?
この方法の方が、現状起こっている問題ももしかしたら解決するかもと思ったりもしました!
https://docs.docker.jp/config/container/multi-service_container.html
CMD ["./bin/rails", "s", "-b", "0.0.0.0"] -> sh -c “./bin/rails s -b 0.0.0.0 && bundle exec sidekiq -C config/sidekiq.yml -e production"

@komtaki
Copy link
Contributor Author

komtaki commented Jun 6, 2021

レビューありがとうございます。:bow:

ちなみにですが、一つのコンテナで起動時にsidekiqも動かしてしまう(以下のリンクのようにsuperviserdなどで管理するもしくは複数コマンドを実行する)でも解決できる気がするのですが、こちらを採用しない理由としては、やはり1コンテナ1プロセスを保ちたいからでしょうか?
この方法の方が、現状起こっている問題ももしかしたら解決するかもと思ったりもしました!

https://github.com/komtaki/decidim-cfj/tree/hotfix/sidkiq-docker-supervisor

同じことを考えこのブランチでsupervisorも試しましたが、症状が全く同じでした。プロセスの起動順が影響しているのかといろいろ試したのですが何も変わらず。:cry:

なのでプロセスをわけても分けなくても同じなら、Dockerの原則にしたがって1コンテナ1プロセスのMRを作った次第です。

@komtaki
Copy link
Contributor Author

komtaki commented Jun 6, 2021

動作確認が取れたので、試行錯誤の過程で入れた不要なbashを除去しました。04e9f10

動かなかった原因は、データの問題。
#56 (comment)

@komtaki komtaki requested a review from ayuki-joto June 6, 2021 07:52
@komtaki komtaki requested a review from ayuki-joto June 7, 2021 09:06
@ayuki-joto ayuki-joto merged commit 88370bd into codeforjapan:develop Jun 7, 2021
@komtaki komtaki deleted the hotfix/sidkiq-docker branch June 24, 2021 09:56
@ayuki-joto ayuki-joto mentioned this pull request Aug 6, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants