Skip to content

lifecycle: Add depends_on for worker and server container#5634

Merged
BeryJu merged 1 commit intogoauthentik:mainfrom
Bojan023:docker_compose_sequence
May 29, 2023
Merged

lifecycle: Add depends_on for worker and server container#5634
BeryJu merged 1 commit intogoauthentik:mainfrom
Bojan023:docker_compose_sequence

Conversation

@Bojan023
Copy link
Contributor

@Bojan023 Bojan023 commented May 16, 2023 •
edited
Loading

Details

It is possible that the server or worker container start prior to redis or postgresql, rendering their health status as unhealthy. It makes sense to define an explicit start-up sequence.

  • Does this resolve an issue?
    No

Changes

  • Declaration of the stack start-up sequence:
    1. postgresql and redis container start in parallel, after both containers run:
    2. the server and worker start in parallel

New Features

Breaking Changes

Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

@Bojan023 Bojan023 requested a review from a team as a code owner May 16, 2023 15:03
@codecov
Copy link

codecov bot commented May 16, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (a419249) 92.63% compared to head (1ec3e04) 92.59%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5634      +/-   ##
==========================================
- Coverage   92.63%   92.59%   -0.04%     
==========================================
  Files         546      546              
  Lines       26184    26184              
==========================================
- Hits        24254    24242      -12     
- Misses       1930     1942      +12
Flag Coverage Δ
e2e 51.82% <ø> (-0.12%) ⬇️
integration 26.39% <ø> (ø)
unit 89.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BeryJu
Copy link
Member

BeryJu commented May 16, 2023 •
edited
Loading

I think there was a reason why we don't do this, let me have a quick look in the compose file

I couldn't find anything, I think the main reason we don't use depends_on in compose is because the authentik containers do their own healthcheck on startup, and there's no equivalent to depends_on in kubernetes, so we rather have the same logic in both places?

@Bojan023
Copy link
Contributor Author

Bojan023 commented May 16, 2023 •
edited
Loading

I follow your reasoning and agree upon the build-in health checks preventing serious issues. However, to me it is not elegant that my stack sometimes starts unhealthy unnecessarily. But that does not take into account other use cases like k8s. In that perspective I understand consistency should prevail. Your call!

@BeryJu BeryJu changed the title Add depend_on for worker and server container lifecycle: Add depend_on for worker and server container May 29, 2023
@BeryJu BeryJu changed the title lifecycle: Add depend_on for worker and server container lifecycle: Add depends_on for worker and server container May 29, 2023
@BeryJu BeryJu merged commit 4a41811 into goauthentik:main May 29, 2023
kensternberg-authentik added a commit that referenced this pull request May 30, 2023
@kensternberg-authentik
Merge branch 'main' into lit-localization
dc8181a 
* main: (58 commits)
  web: bump @babel/preset-env from 7.22.2 to 7.22.4 in /web (#5808)
  web: bump @typescript-eslint/eslint-plugin from 5.59.7 to 5.59.8 in /web (#5805)
  web: bump @babel/plugin-transform-runtime from 7.22.2 to 7.22.4 in /web (#5806)
  web: bump @typescript-eslint/parser from 5.59.7 to 5.59.8 in /web (#5807)
  stages/deny: fix typos (#5800)
  core: bump coverage from 7.2.6 to 7.2.7 (#5809)
  blueprints: allow setting user's passwords from blueprints (#5797)
  providers/proxy: add support for traefik.io API and CRD (#5801)
  web: fix build (#5798)
  web/flows: fix RedirectStage not detecting absolute URLs correctly (#5781)
  lifecycle: Add depends_on for worker and server container (#5634)
  web: bump @babel/preset-env from 7.21.5 to 7.22.2 in /web (#5791)
  web: bump @babel/plugin-proposal-decorators from 7.21.0 to 7.22.3 in /web (#5787)
  web: bump @lingui/core from 4.1.2 to 4.2.0 in /web (#5782)
  website: bump postcss from 8.4.23 to 8.4.24 in /website (#5783)
  web: bump @babel/plugin-transform-runtime from 7.21.4 to 7.22.2 in /web (#5789)
  core: bump goauthentik.io/api/v3 from 3.2023051.3 to 3.2023052.1 (#5788)
  web: bump @babel/core from 7.21.8 to 7.22.1 in /web (#5790)
  web: bump yaml from 2.3.0 to 2.3.1 in /web (#5792)
  core: bump django-otp from 1.2.0 to 1.2.1 (#5794)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Bojan023 @BeryJu