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: Migrate container registries from etcd to postgreSQL #1917

Open
wants to merge 76 commits into
base: main
Choose a base branch
from

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Feb 19, 2024

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation

Resolves #1907.

Changes

  • Migrate container registry schema and data from etcd to PostgreSQL
  • Update ContainerRegistry graphql API argument from hostname to id (UUID)
  • hostname is changed into registry_name
  • Add is_global column to the ContainerRegistry table

@github-actions github-actions bot added comp:manager Related to Manager component size:XL 500~ LoC labels Feb 19, 2024
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from 51b64c8 to 497bc32 Compare February 19, 2024 02:01
@jopemachine jopemachine changed the title feat: Migrate container registry info from etcd to postgresql Migrate container registry schema from etcd to postgreSQL Feb 19, 2024
@jopemachine jopemachine linked an issue Feb 19, 2024 that may be closed by this pull request
@jopemachine jopemachine added this to the 24.03 milestone Feb 19, 2024
@jopemachine jopemachine changed the title Migrate container registry schema from etcd to postgreSQL feat: Migrate container registry schema from etcd to postgreSQL Feb 19, 2024
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch 8 times, most recently from ecff04f to d8a29c4 Compare February 21, 2024 04:50
@jopemachine jopemachine marked this pull request as ready for review February 21, 2024 06:06
@jopemachine jopemachine added the require:db-migration Automatically set when alembic migrations are added or updated label Feb 22, 2024
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch 4 times, most recently from e6d9ef0 to 127221c Compare February 29, 2024 07:10
Copy link
Member

@kyujin-cho kyujin-cho left a comment

Choose a reason for hiding this comment

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

Current implementation does not have a flag field at container_registry table which should indicate if this registry is available for every users. Please update the branch accordingly.

src/ai/backend/manager/models/base.py Outdated Show resolved Hide resolved
@jopemachine jopemachine marked this pull request as draft March 6, 2024 04:42
@jopemachine jopemachine marked this pull request as ready for review March 7, 2024 05:33

maybe_registries = queue.sync_q.get()

if isinstance(maybe_registries, Exception):
Copy link
Member

@achimnol achimnol Mar 7, 2024

Choose a reason for hiding this comment

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

Is any unhandled exception inside asyncio.run() stored as a queue item?

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception is stored in the queue only when the backup method fails currently.
Should we catch all exceptions occurring in the thread and put them into the queue?

tests/manager/conftest.py Outdated Show resolved Hide resolved
@fregataa fregataa self-requested a review March 8, 2024 05:12
Copy link
Contributor

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

I left some comments about typing

src/ai/backend/manager/models/image.py Outdated Show resolved Hide resolved
src/ai/backend/manager/container_registry/harbor.py Outdated Show resolved Hide resolved
src/ai/backend/manager/container_registry/harbor.py Outdated Show resolved Hide resolved
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from 9891e05 to 9fd2e2a Compare March 8, 2024 06:48
Copy link
Contributor

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

Just a little suggestion about type hint in is_known_registry(). How about accepting only dict (or Mapping) type as known_registries parameter?
Please enlighten me how the known_registries looks like in list type and dict type

src/ai/backend/common/docker.py Outdated Show resolved Hide resolved
src/ai/backend/common/docker.py Outdated Show resolved Hide resolved
src/ai/backend/common/docker.py Outdated Show resolved Hide resolved
src/ai/backend/manager/registry.py Outdated Show resolved Hide resolved
src/ai/backend/common/docker.py Outdated Show resolved Hide resolved
src/ai/backend/common/docker.py Outdated Show resolved Hide resolved
@jopemachine
Copy link
Member Author

jopemachine commented Mar 17, 2024

Just a little suggestion about type hint in is_known_registry(). How about accepting only dict (or Mapping) type as known_registries parameter? Please enlighten me how the known_registries looks like in list type and dict type

dict example:

{'stable': {'cr.backend.ai': URL('https://cr.backend.ai')}, 'community': {'cr.backend.ai': URL('https://cr.backend.ai')}, 'multiarch': {'cr.backend.ai': URL('https://cr.backend.ai')}}

list example:

 ['cr.backend.ai']

Note: list is not aware of project unlike dict type here as you see above example.

Copy link
Contributor

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

good!
in the future, we can apply more strict typing about container image data such as ImageRef, ImageRegistry or ContainerRegistry etc, and revamp codes

@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from 20be322 to ea2ea64 Compare March 22, 2024 01:20
@github-actions github-actions bot added the comp:client Related to Client component label Mar 22, 2024
@jopemachine jopemachine added the comp:installer Related to Installer label Mar 22, 2024
@jopemachine jopemachine changed the title feat: Migrate container registry schema from etcd to postgreSQL feat: Migrate container registries from etcd to postgreSQL Mar 22, 2024
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from 560c2e7 to 9cbb0c3 Compare March 22, 2024 23:45
@kyujin-cho
Copy link
Member

kyujin-cho commented Apr 8, 2024

I am postponing milestone to 24.09. We managed to meet our internal deadline for per-user image customization feature (#1973). From now on, at this PR, let's focus on forming concrete definitions and stabilizing things rather than rushing for the merge approval.

@kyujin-cho kyujin-cho modified the milestones: 24.03, 24.09 Apr 8, 2024
@github-actions github-actions bot modified the milestones: 24.09, 24.03 Apr 9, 2024
@jopemachine jopemachine modified the milestones: 24.03, 24.09 Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:client Related to Client component comp:installer Related to Installer comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rebase container registry config storage to relational database
5 participants