Set asset size on job creation

[Vogtinator]

Assets without a size are treated like they don't exist. With POST isos, referenced assets don't directly get a size and are thus not shown as downloadable. Only the next limit_assets task run changes this by running refresh_assets. Improve this by setting the size for assets on job creation.

• Is this the right place? What about e.g. ISO_1_URL?
• Is it cheap enough to do refresh_size here or should this end up as ensure_size instead?

[Martchus]

I vaguely remember we once had something like this but removed the code again because it could lead to deadlocks between database transactions. I'll have a closer look at this tomorrow.

[Vogtinator]

I vaguely remember we once had something like this but removed the code again because it could lead to deadlocks between database transactions. I'll have a closer look at this tomorrow.

The code path does an insert already, so a further update shouldn't hurt...

[Martchus]

Good, then this was probably a different place.

[Vogtinator]

Good, then this was probably a different place.

Maybe? 8334b47 I don't really know anything about this deadlock bug and whether that's already addressed.

[Martchus]

The change I was thinking of was 79ba646 and its follow-up 00fe4e5.

Sorry for accidentally closing. I accidentally hit tab while typing and the next white-space then closed the PR.

[Vogtinator]

The change I was thinking of was 79ba646 and its follow-up 00fe4e5.

Ok, so this might possibly introduce the deadlock again though I'm not sure how this can actually deadlock.

[Martchus]

Ok, so this might possibly introduce the deadlock again though I'm not sure how this can actually deadlock.

My change was actually about register_assets_from_settings which is mainly called in create_from_settings which runs in a big transaction when scheduling an ISO. Your change runs before that big transaction so I guess it is less likely to cause any problems - especially since we're already acquiring a write lock at this place anyways.

Is this the right place? What about e.g. ISO_1_URL?

The loop your code is part of runs on the return value of parse_assets_from_settings which doesn't include …_URL settings. So the code will not fail on those settings but of course also won't be able actually set the size because the asset hasn't been downloaded yet at this point.

Is it cheap enough to do refresh_size here or should this end up as ensure_size instead?

Not sure. But this is about ensuring assets have a size at all, right? I would say if ensure_size is good enough (which sounds like it would be the case) than just use that cheaper function.

[Vogtinator]

Ok, so this might possibly introduce the deadlock again though I'm not sure how this can actually deadlock.

My change was actually about register_assets_from_settings which is mainly called in create_from_settings which runs in a big transaction when scheduling an ISO. Your change runs before that big transaction so I guess it is less likely to cause any problems - especially since we're already acquiring a write lock at this place anyways.

Is this the right place? What about e.g. ISO_1_URL?

The loop your code is part of runs on the return value of parse_assets_from_settings which doesn't include …_URL settings. So the code will not fail on those settings but of course also won't be able actually set the size because the asset hasn't been downloaded yet at this point.

Is that already handled in the download code or does that need to be added as well?

Is it cheap enough to do refresh_size here or should this end up as ensure_size instead?

Not sure. But this is about ensuring assets have a size at all, right? I would say if ensure_size is good enough (which sounds like it would be the case) than just use that cheaper function.

Yep, but FWICT this is only run once for all explicitly posted assets, so probably not worth the extra code.

[Martchus]

Is that already handled in the download code or does that need to be added as well?

Good question. It doesn't look like it is handled by that code. The relevant file would be lib/OpenQA/Task/Asset/Download.pm and it doesn't update the asset table after a successful download. So I guess it would make sense to refresh the asset size there.

Yep, but FWICT this is only run once for all explicitly posted assets, so probably not worth the extra code.

Ok, although there's already a ensure_size function so no extra code would be required.

[Vogtinator]

Is that already handled in the download code or does that need to be added as well?

Good question. It doesn't look like it is handled by that code. The relevant file would be lib/OpenQA/Task/Asset/Download.pm and it doesn't update the asset table after a successful download. So I guess it would make sense to refresh the asset size there.

Looks like that needs the type which I'm not sure how to get at that point easily...

Yep, but FWICT this is only run once for all explicitly posted assets, so probably not worth the extra code.

Ok, although there's already a ensure_size function so no extra code would be required.

register would need a line to handle that like refresh_size

[Martchus]

Looks like that needs the type which I'm not sure how to get at that point easily...

You would have to do another database query like this:

my $asset = $app->schema->resultset('Assets')->find({name => $asset_name, type => $asset_type});

The only problem is that even $asset_name and $asset_type are not available anymore in that context but they could likely be supplied easily as additional parameters. The only annoyance is that these additional parameters would need to be added on various function calls as name and type are already lost in create_downloads_list.

register would need a line to handle that like refresh_size

Right.

[Vogtinator]

Looks like that needs the type which I'm not sure how to get at that point easily...

You would have to do another database query like this:

my $asset = $app->schema->resultset('Assets')->find({name => $asset_name, type => $asset_type});

The only problem is that even $asset_name and $asset_type are not available anymore in that context but they could likely be supplied easily as additional parameters. The only annoyance is that these additional parameters would need to be added on various function calls as name and type are already lost in create_downloads_list.

Yeah... Fortunately this isn't that important for _URL assets as those can be downloaded from the job settings page directly, so something for a later PR?

register would need a line to handle that like refresh_size

Right.

[Martchus]

Yes, we can save this for later as all of this is basically an optional improvement anyways.

It would be good if your change would extend the tests, though.

[Vogtinator]

Yes, we can save this for later as all of this is basically an optional improvement anyways.

Well, without this PR it's currently not possible to download isos from o3.

It would be good if your change would extend the tests, though.

Done.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34068e3) 98.35% compared to head (22f8b9b) 98.36%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5369   +/-   ##
=======================================
  Coverage   98.35%   98.36%           
=======================================
  Files         389      389           
  Lines       37432    37441    +9     
=======================================
+ Hits        36818    36827    +9     
  Misses        614      614           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Martchus]

Looks good, I only have one small suggestion.