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

Rewrite asset list generation for cache service #2860

Merged
merged 1 commit into from Mar 25, 2020

Conversation

@mdoucha
Copy link
Contributor

mdoucha commented Mar 24, 2020

Extract all cacheable assets from job settings. Also cache generic assets defined using ASSET_* settings and ignore external assets (*_URL) which are handled elsewhere.

This will also help resolve issues with LTP runfile assets on O3 where one of the workers is losing the NFS mount for shared asset directory.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #2860 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2860      +/-   ##
==========================================
- Coverage   93.23%   93.23%   -0.01%     
==========================================
  Files         189      189              
  Lines       11882    11881       -1     
==========================================
- Hits        11078    11077       -1     
  Misses        804      804              
Impacted Files Coverage Δ
lib/OpenQA/Worker/Engines/isotovideo.pm 93.21% <100.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36618e3...94a41b5. Read the comment docs.

Copy link
Contributor

Martchus left a comment

Looks good, just some nitpicks.

lib/OpenQA/Worker/Engines/isotovideo.pm Outdated Show resolved Hide resolved
lib/OpenQA/Worker/Engines/isotovideo.pm Outdated Show resolved Hide resolved
lib/OpenQA/Worker/Engines/isotovideo.pm Show resolved Hide resolved
@mdoucha mdoucha force-pushed the mdoucha:asset-cache branch from 24cc719 to 1c110c1 Mar 24, 2020
@pevik
pevik approved these changes Mar 24, 2020
Copy link
Contributor

pevik left a comment

LGTM, thanks for implementing it.

Copy link
Contributor

Martchus left a comment

LGTM but I give other team members some time to review it before clicking the merge button.

@kraih
kraih approved these changes Mar 24, 2020
Copy link
Member

kraih left a comment

Code looks ok, but i don't know which assets should go through the cache service.

@pevik

This comment has been minimized.

Copy link
Contributor

pevik commented Mar 24, 2020

@okurz even if there is a ticket instead of TODO I consider good to note the problem or a ticket in commit message / code. It helps others to understand what was mentioned to be done but haven't and generally to understand complex code (tickets gets unimplemented as well and if you look into the code you don't also search in progress).

@mdoucha

This comment has been minimized.

Copy link
Contributor Author

mdoucha commented Mar 24, 2020

I don't care either way whether repo caching ever gets implemented. But if you want to get rid of the NFS workaround, it'll have to be implemented eventually or all our migration tests will break. https://openqa.suse.de/tests/4025892#settings

The TODO is there to provide context why repo assets are explicitly excluded from cached assets and what needs to be done so they can be properly cached. I don't know how else to provide that context clearly enough. Suggestions welcome.

my $type = asset_type_from_setting($key, $value);

# TODO: Allow repo caching when the cache service gets support
# for downloading entire directories.

This comment has been minimized.

Copy link
@kraih

kraih Mar 24, 2020

Member

Just make this a normal comment, explaining why repos are excluded without implying that the feature will be added in the future.

@okurz

This comment has been minimized.

Copy link
Member

okurz commented Mar 25, 2020

AFAIK the migration tests use http and not NFS. So please follow #2860 (comment)

@okurz

This comment has been minimized.

Copy link
Member

okurz commented Mar 25, 2020

@pevik the point is: I am not even sure if we ever want to sync repos this is why the TODO – if taken serious – means that something "should be done" but we didn't agree it should be done

@pevik

This comment has been minimized.

Copy link
Contributor

pevik commented Mar 25, 2020

@okurz ok. But no problem if the whole thing is about removing word TODO, but keeping the comment.

@mdoucha mdoucha force-pushed the mdoucha:asset-cache branch 2 times, most recently from 83a122b to ad137b7 Mar 25, 2020
@mdoucha

This comment has been minimized.

Copy link
Contributor Author

mdoucha commented Mar 25, 2020

TODO removed.

lib/OpenQA/Worker/Engines/isotovideo.pm Outdated Show resolved Hide resolved
Extract all cacheable assets from job settings. Also cache generic assets
defined using ASSET_* settings and ignore external assets (*_URL) which are
handled elsewhere.
@mdoucha mdoucha force-pushed the mdoucha:asset-cache branch from ad137b7 to 94a41b5 Mar 25, 2020
@okurz
okurz approved these changes Mar 25, 2020
@kraih
kraih approved these changes Mar 25, 2020
@okurz okurz merged commit 2e2f43f into os-autoinst:master Mar 25, 2020
14 checks passed
14 checks passed
OBS Package Build 8/8 processed
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: cache Your tests passed on CircleCI!
Details
ci/circleci: cache.fullstack Your tests passed on CircleCI!
Details
ci/circleci: checkstyle Your tests passed on CircleCI!
Details
ci/circleci: codecov Your tests passed on CircleCI!
Details
ci/circleci: developer Your tests passed on CircleCI!
Details
ci/circleci: fullstack Your tests passed on CircleCI!
Details
ci/circleci: scheduler Your tests passed on CircleCI!
Details
ci/circleci: t Your tests passed on CircleCI!
Details
ci/circleci: ui Your tests passed on CircleCI!
Details
codecov/patch 100.00% of diff hit (target 93.23%)
Details
codecov/project 93.23% (target 85.70%)
Details
@mdoucha mdoucha deleted the mdoucha:asset-cache branch Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.