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

chore(server): Move library watcher to microservices #7533

Merged
merged 36 commits into from Mar 7, 2024

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Feb 29, 2024

This moves the library watcher to be running in the microservices.

Inspiration is from @jrasm91 who suggested this as a better option to #7509.

Here, if the watcher fails and stalls (as in #6833) we can stop the microservices container and disable the library watcher, unbricking the Immich install in a clean way

Added lock so that only one microservice takes care of the watcher in the case of multiple micros

Tests performed:

  • App init works
  • Library watcher automatically imports changes
  • This workflow works: Disable microservices in docker compose, start app, disable library watcher
  • Started multiple microservices and only one started the watcher

Copy link

cloudflare-pages bot commented Feb 29, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8b9b5bb
Status: ✅  Deploy successful!
Preview URL: https://12442231.immich.pages.dev
Branch Preview URL: https://chore-library-init-micro.immich.pages.dev

View logs

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This looks good so far, but you need to prevent library scanning from running on installs which run multiple instances of microservices. I think you can accomplish this by asking for a postgres lock, although you might need to get more info from @zackpollard or @mertalev on that.

@etnoy
Copy link
Contributor Author

etnoy commented Feb 29, 2024

My test shows the current lock implementation did not work as expected, will have to do some debugging later:

immich_microservices-dev-2 | [Nest] 20  - 02/29/2024, 7:24:09 PM     LOG [SystemConfigService] LogLevel=verbose (set via LOG_LEVEL)
immich_microservices-dev-2 | [Nest] 20  - 02/29/2024, 7:24:09 PM     LOG [LibraryService] Starting to watch library df9f43b9-bf13-4b1a-a469-fbde3bda1f3f with import path(s) /data/import/bilder/2023/
immich_microservices-dev   | [Nest] 20  - 02/29/2024, 7:24:09 PM     LOG [SystemConfigService] LogLevel=verbose (set via LOG_LEVEL)
immich_microservices-dev   | [Nest] 20  - 02/29/2024, 7:24:09 PM     LOG [LibraryService] Starting to watch library df9f43b9-bf13-4b1a-a469-fbde3bda1f3f with import path(s) /data/import/bilder/2023/

only one microservice should have initiated the watch

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 29, 2024

The return type is an array:

[ { pg_try_advisory_lock: true } ]

@etnoy
Copy link
Contributor Author

etnoy commented Feb 29, 2024

The return type is an array:

[ { pg_try_advisory_lock: true } ]

Yeah, I have to get used to ts not doing runtime type checking. Thanks :)

This reverts commit 84ab5ab.
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM!

server/src/domain/library/library.service.ts Outdated Show resolved Hide resolved
server/src/domain/library/library.service.ts Outdated Show resolved Hide resolved
@alextran1502 alextran1502 merged commit 4cb0f37 into main Mar 7, 2024
25 checks passed
@alextran1502 alextran1502 deleted the chore/library-init-micro branch March 7, 2024 17:36
aviv926 pushed a commit to aviv926/immich that referenced this pull request Mar 7, 2024
* move watcher init to micro

* document watcher recovery

* chore: fix lint

* add try lock

* use global library watch lock

* fix: ensure lock stays on

* fix: mocks

* unit test for library watch lock

* move statement to correct test

* fix: correct return type of try lock

* fix: tests

* add library teardown

* add chokidar error handler

* make event strings an enum

* wait for event refactor

* refactor event type mocks

* expect correct error

* don't release lock in teardown

* chore: lint

* use enum

* fix mock

* fix lint

* fix watcher await

* remove await

* simplify typing

* remove async

* Revert "remove async"

This reverts commit 84ab5ab.

* can now change watch settings at runtime

* fix lint

* only watch libraries if enabled

---------

Co-authored-by: mertalev <101130780+mertalev@users.noreply.github.com>
Co-authored-by: Alex Tran <alex.tran1502@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants