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

var-update-trigger: Update only devices that should be running the release that the image env var is part of #1796

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thgreasi
Copy link
Member

@thgreasi thgreasi commented Oct 17, 2024

The device.should_be_running__release path is optimized w/ the /canAccess() permissions hence this produces a smaller query, which has faster planning & execution time.

It looks like that the previous query was trying to only update devices that already have an image_install, but in the form it was, it was it was also including devices with image_installs that were marked as deleted. So the new approach might actually be saving work.

Didn't add further nested filters on image_installs, in an attempt to keep the query smaller, but I don't think that they are necessary, since it's correct to give trigger an update on a device that has just started downloading a release, but didn't have yet the time to do a state PATCH to create the image_installs.
Moreover, we don't really expect any POSTs or UPDATEs of device service env vars after a release successfully builds anyway.

Change-type: patch

@thgreasi thgreasi force-pushed the v2-image-env-var-device-update-trigger branch from cfd717d to c691ec8 Compare October 17, 2024 14:45
@thgreasi thgreasi requested review from Page- and a team October 17, 2024 15:02
@thgreasi thgreasi marked this pull request as ready for review October 17, 2024 15:11
@thgreasi thgreasi marked this pull request as draft October 17, 2024 15:21
@thgreasi thgreasi force-pushed the v2-image-env-var-device-update-trigger branch from c691ec8 to 8134c40 Compare October 17, 2024 15:48
$expr: {
installs__image: {
status: 'success',
release_image: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use contains_image to make this match the state get endpoint, and add a comment to the top of this file that the having the queries match the path the state get endpoint uses to get env vars for a device is the way to go. Essentially this captures if a subset of the state-get query will change so the correct thing is to follow the same path (and also means it'll have the same performance characteristics, which we take real care to optimize for in the state-get)

Copy link
Member Author

@thgreasi thgreasi Oct 18, 2024

Choose a reason for hiding this comment

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

Updated the state GET endoints to use the release_image term form
See: #1800
See: https://github.com/balena-io/balena-api/pull/5381
and added the comment that this query should match the state GET paths as discussed.

@thgreasi thgreasi force-pushed the v2-image-env-var-device-update-trigger branch 2 times, most recently from ee70693 to e7838a9 Compare October 21, 2024 13:53
…lease that the image env var is part of

Change-type: patch
@thgreasi thgreasi force-pushed the v2-image-env-var-device-update-trigger branch from e7838a9 to cf9c776 Compare October 21, 2024 16:06
@thgreasi thgreasi marked this pull request as ready for review October 21, 2024 17:14
@thgreasi thgreasi requested a review from Page- October 21, 2024 17:14
@thgreasi thgreasi marked this pull request as draft October 22, 2024 14:24
@thgreasi
Copy link
Member Author

Recheked this query and it seems that this way it becomes as slow as the service_environment_variables one (3+ seconds)...
The mistake was that I used a release id in place of a release_image id in my profiled queries...

@thgreasi
Copy link
Member Author

Marking as draft for now since we shouldn't get it in given that it regresses performance.
Should we reconsider #1794 ?

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