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

Remove releaseId and imageId from container names #2136

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pipex
Copy link
Contributor

@pipex pipex commented Mar 2, 2023

The release id and image id are not really necessary for reporting the current state since v3 (it has been replaced by releaseUuid + imageName) and is only kept for backwards compatibility for some supervisor API endpoints.

Until now, both the releaseId and imageId were part of the container name, leading to longer names than desired. This change removes those values from the container name and modifies queries to look for the value in the image database.

This also removes imageId from the log stream, which was no longer used by the backend and should result in some bandwidth usage improvements

Change-type: minor
Relates-to: #2077

@pipex pipex marked this pull request as draft March 2, 2023 20:49
@pipex pipex force-pushed the remove-releaseId-imageId branch 4 times, most recently from e285017 to ea5fb85 Compare March 7, 2023 22:32
src/device-api/v2.ts Outdated Show resolved Hide resolved
src/device-api/actions.ts Outdated Show resolved Hide resolved
@pipex pipex force-pushed the remove-releaseId-imageId branch 3 times, most recently from 91949cf to bcc2617 Compare April 27, 2023 00:36
pipex added a commit to balena-io/balena-cli that referenced this pull request Apr 27, 2023
On local push, the CLI uses `localrelease` as the `commit` property for
the development application. This is not a valid uuid and will not be
read properly by the supervisor, as seen in

https://github.com/balena-os/balena-supervisor/blob/master/src/compose/service.ts#L652

While this is not a problem right now, the commit is becoming the main
way to identify a service release (replacing `releaseId` and `imageId`),
and the invalid release uuid could cause update issues when pushing a
local release on when using some API endpoints.

Change-type: patch
Relates-to: balena-os/balena-supervisor#2136
pipex added a commit to balena-io/balena-cli that referenced this pull request May 2, 2023
On local push, the CLI uses `localrelease` as the `commit` property for
the development application. This is not a valid uuid and will not be
read properly by the supervisor, as seen in

https://github.com/balena-os/balena-supervisor/blob/master/src/compose/service.ts#L652

While this is not a problem right now, the commit is becoming the main
way to identify a service release (replacing `releaseId` and `imageId`),
and the invalid release uuid could cause update issues when pushing a
local release on when using some API endpoints.

Change-type: patch
Relates-to: balena-os/balena-supervisor#2136
pipex added a commit to balena-io/balena-cli that referenced this pull request May 3, 2023
On local push, the CLI uses `localrelease` as the `commit` property for
the development application. This is not a valid uuid and will not be
read properly by the supervisor, as seen in

https://github.com/balena-os/balena-supervisor/blob/master/src/compose/service.ts#L652

While this is not a problem right now, the commit is becoming the main
way to identify a service release (replacing `releaseId` and `imageId`),
and the invalid release uuid could cause update issues when pushing a
local release on when using some API endpoints.

Change-type: patch
Relates-to: balena-os/balena-supervisor#2136
@pipex pipex force-pushed the remove-releaseId-imageId branch 2 times, most recently from a915937 to 74aa838 Compare June 19, 2023 22:00
@pipex pipex force-pushed the remove-releaseId-imageId branch from 74aa838 to 8a47e0f Compare June 28, 2023 16:04
@pipex pipex force-pushed the remove-releaseId-imageId branch from 8a47e0f to 4b772c4 Compare July 12, 2023 22:11
pipex added a commit to balena-io/balena-cli that referenced this pull request Jul 14, 2023
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
@pipex pipex force-pushed the remove-releaseId-imageId branch 3 times, most recently from fd6c890 to 6171d96 Compare November 16, 2023 20:50
@pipex pipex requested a review from cywang117 November 16, 2023 20:53
@pipex pipex marked this pull request as ready for review November 16, 2023 20:53
The release id is not really necessary for reporting the current state
since v3 (it has been replaced by `releaseUuid`) and is only kept for
backwards compatibility for some supervisor API endpoints.

Until now, the `releaseId` was part o the container name, leading to
longer names than desired. This change removes the value from the
container name and modifies queries to look for the value in the image
database.

Change-type: minor
Relates-to: #2077
@pipex pipex force-pushed the remove-releaseId-imageId branch from 6171d96 to fbdb2ca Compare November 23, 2023 22:16
@flowzone-app flowzone-app bot enabled auto-merge November 23, 2023 22:17
@pipex
Copy link
Contributor Author

pipex commented Nov 28, 2023

@cywang117 remember to take a look at this when you get a chance

Copy link
Contributor

@cywang117 cywang117 left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

Comment on lines 443 to 447
// QUESTION: what to do if we cannot find the image?
// if the image database got deleted
s.imageId = allImages.find(
(img) => img.commit === svc.commit && img.serviceName === s.serviceName,
)?.imageId;

return s;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to worry about this, if the image table got deleted unexpectedly, then the action would have thrown a lot earlier during images.getState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think images.getState would throw, but we recreate the image database when initializing application manager so if the data from getState should be correct, so you are right the comment is unnecessary

pipex added a commit to balena-io/balena-cli that referenced this pull request Dec 1, 2023
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
pipex added a commit to balena-io/balena-cli that referenced this pull request Dec 6, 2023
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
pipex added a commit to balena-io/balena-cli that referenced this pull request Dec 11, 2023
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
The image id is no longer necessary to report the current state since
moving to v3 of the state endpoint and it is only kept for backwards
compatibility for some supervisor API endpoings.

Until now, `imageId` was part of the container name leading to longer
names than desired. This change removes the value from the container
name an modifies queries to look for the value in the image database.

This also removes the imageId from the log stream which should result in
reduced bandwidth usage.

Change-type: minor
This allows to compare the current and target container names and
trigger an `updateMetadata` step if they are different. This allows us
to normalize the container name format with a new supervisor update.

Change-type: minor
@pipex pipex force-pushed the remove-releaseId-imageId branch from fbdb2ca to f7ed27f Compare December 11, 2023 20:23
pipex added a commit to balena-io/balena-cli that referenced this pull request Dec 12, 2023
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
@pipex
Copy link
Contributor Author

pipex commented Dec 14, 2023

Will need to refactor this, I had not considered that, because of the git workflow, in a multi-app world you could have two apps pointing to the same release commit. Will probably need to keep the releaseId

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