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: Make image table's project, registry_id column nullable #2888

Closed

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Oct 2, 2024

Follow-up of #1917.

  • Refactoring the image rescan command

    • Although the existing image rescan command works well, unfortunately, the code does not align with the original design intent. The existing rescan_images code is designed to work with multiple container registries, but because it is not project-aware, it only operates on the last record. This issue is addressed by iterating over all container registries in rescan_single_registry, but this is not the correct approach.
    • This code has been refactored to return the project iteration loop back to rescan_images.
    • Unfortunately, running multiple rescan_single_registry tasks concurrently has been found to trigger an abort. Therefore, I modified it to execute each iteration sequentially. While this results in a slight performance decrease, we can consider improving it to support parallel processing in the future.
  • More flexible support for local-type container registries

    • Previously, even local-type container registries were constrained by the project value and the name of a non-existent virtual container registry. In this PR, all unnecessary restrictions have been removed, allowing local images to be scanned as they are.
  • Make image table's project column nullable

    • If the "project" column is null, the string after the registry will be treated as the image name, following the existing logic.
  • Make container registry table's project column nullable

    • This is primarily intended to provide more flexible support for local-type container registries, but it may also be useful for other use cases.
  • Modified the upgrade() to automatically generate the corresponding container registry row instead of raising an error when no container registry exists for the image canonical name.

Thoughts

  • Local-type container registry records currently do not contain meaningful information. Additionally, having more than one local container registry type would be awkward.

  • Whether an image is local can be determined by checking if the container registry pointed to by image.registry_id is of the Local type, making the is_local column redundant.

Container registry row auto-generation example

🕙 18:58:36 ❯ ./py -m alembic upgrade +1
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 20218a73401b -> 1d42c726d8a3, Migrate container registry config storage from `Etcd` to `PostgreSQL`
INFO  [alembic.runtime.migration] Following container registry row auto-generated: "cr.backend.ai" with the project "multiarch".
INFO  [alembic.runtime.migration] If credential required for the auto-generated container registry rows, you should fill their credential columns manually.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@jopemachine jopemachine added type:bug Reports about that are not working require:db-migration Automatically set when alembic migrations are added or updated labels Oct 2, 2024
@jopemachine jopemachine added this to the 24.09 milestone Oct 2, 2024
@jopemachine jopemachine self-assigned this Oct 2, 2024
@jopemachine jopemachine added the skip:ci Make the action workflow to skip running lint, check, and test (use with caution!) label Oct 2, 2024
@jopemachine jopemachine marked this pull request as ready for review October 3, 2024 10:06
@jopemachine jopemachine force-pushed the fix/wrong-container-registries-migration-script branch from c420fd8 to 2274fc8 Compare October 3, 2024 10:06
@jopemachine jopemachine added the skip:changelog Make the action workflow to skip towncrier check label Oct 3, 2024
@jopemachine jopemachine changed the title fix: Wrong container registries migration script fix: Improve container registries migration script Oct 3, 2024
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Still it fails:

  File "/home/joongi/bai-edge/src/ai/backend/manager/models/alembic/versions/1d42c726d8a3_migrate_container_registries_from_etcd_to_psql.py", line 457, in upgrade
    insert_registry_id_to_images_with_black_registry_id()
  File "/home/joongi/bai-edge/src/ai/backend/manager/models/alembic/versions/1d42c726d8a3_migrate_container_registries_from_etcd_to_psql.py", line 381, in insert_registry_id_to_images_with_black_registry_id
    registry_info = dict(registry_info)
                    ^^^^^^^^^^^^^^^^^^^
TypeError: 'NoneType' object is not iterable

@jopemachine jopemachine changed the title fix: Improve container registries migration script fix: Make image table's project, registry_id column nullable Oct 6, 2024
@jopemachine jopemachine added comp:manager Related to Manager component and removed skip:changelog Make the action workflow to skip towncrier check skip:ci Make the action workflow to skip running lint, check, and test (use with caution!) labels Oct 6, 2024
@jopemachine jopemachine force-pushed the fix/wrong-container-registries-migration-script branch from 2059891 to 03ecb01 Compare October 6, 2024 05:17
@jopemachine jopemachine added the type:refactor Refactor codes or add tests. label Oct 6, 2024
lizable pushed a commit to lablup/backend.ai-webui that referenced this pull request Oct 21, 2024
- related PR: lablup/backend.ai#1917, lablup/backend.ai#2888

**TR;DR**
New GraphQL queries and mutations have been added related to the Registry since 24.09. Internally, the management from etcd has been changed to DB table. Therefore, version management is as follows:
- 24.09.0 and above: Use `ContainerRegistryList.tsx`, `ContainerRegistryEditorModal.tsx`
- 23.09.2 and above but below 24.09.0: Use `ContainerRegistryListBefore2409.tsx`, `ContainerRegistryEditorModalBefore2409.tsx`
- Below 23.09.2: Use `backend-ai-registry-list.ts`

**What's changed**
- `ContainerRegistryList.tsx` and `ContainerRegistryEditorModal.tsx` have been renamed to `ContainerRegistryListBefore2409.tsx`, `ContainerRegistryEditorModalBefore2409.tsx`.
- From 24.09.0, `ContainerRegistryList.tsx` and `ContainerRegistryEditorModal.tsx` use `container_registry_nodes` related query and mutation.
- Pagination and sorting (only for registry name) are supported.
- Updated schema

**How to test**
1. Change https://github.com/lablup/backend.ai-webui/blob/7163812c4b3a70679b0efb347e11b52c3f9948cb/react/src/pages/EnvironmentPage.tsx#L65-L73 to like this.
```ts
isSupportContainerRegistryGraphQL ? (
    <ContainerRegistryList />
) : (
````
2. Change this line https://github.com/lablup/backend.ai-webui/blob/7163812c4b3a70679b0efb347e11b52c3f9948cb/react/src/components/ContainerRegistryList.tsx#L87 to like this. (since 24.09.0 is not released yet).
```ts
          ) @SInCE(version: "24.09.0b1") {
```
3. Go to Environments - Registries page.

**Checklist for reviewers**
- [ ] CRUD registries.
- [ ] The project is set to 'library' when the type is 'docker' and the input field is disabled.
- [ ] Registry name filtering and sorting work well.
- [ ] Pagination works well.
  - For testing this, you can add '1' option to this line. https://github.com/lablup/backend.ai-webui/blob/a7ab1ed6fe540070010f87a437f52c1c42b30abc/react/src/components/ContainerRegistryList.tsx#L347

**Checklist:** (if applicable)

- [x] Mention to the original issue
- [ ] Documentation
- [x] Minium required manager version
- [x] Specific setting for review (eg., KB link, endpoint or how to setup)
- [x] Minimum requirements to check during review
- [ ] Test case(s) to demonstrate the difference of before/after
@jopemachine jopemachine force-pushed the fix/wrong-container-registries-migration-script branch from 872fb3b to b51f89d Compare October 23, 2024 13:22
@jopemachine jopemachine marked this pull request as draft October 23, 2024 13:22
@jopemachine jopemachine marked this pull request as ready for review October 23, 2024 13:25
@jopemachine jopemachine marked this pull request as draft October 25, 2024 02:18
@jopemachine jopemachine force-pushed the fix/wrong-container-registries-migration-script branch from 5613c3f to b4c8741 Compare October 25, 2024 05:56
@jopemachine
Copy link
Member Author

jopemachine commented Oct 25, 2024

The current PR has become larger than initially intended, making it difficult to review. So, I will close this PR and recreate it as a PR stack in Graphite.

See #2978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated type:bug Reports about that are not working type:refactor Refactor codes or add tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants