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

Add CacheKey config to override cache_tree_paths #2862

Closed
wants to merge 1 commit into from

Conversation

dzil123
Copy link

@dzil123 dzil123 commented Jul 11, 2024

My usecase doesn't fit in mkosi.images.

Currently, I'm managing my image variants with include and image_id. If I instead put my image variants in mkosi.images, each variant tries to build the same default-initrd image and invalidate the cache each time.

This doesn't work for me because each image variant should have a separate cache for itself and its unique default-initrd image.

@DaanDeMeyer
Copy link
Contributor

The thing is, you're introducing the assumption here that a different image ID equals a different set of packages that get installed, which is not always going to be true. With this change you force everyone that's using different image IDs without changing the packages that get installed per image ID to have a duplicate cache image per image ID.

I think introducing a CacheKey= setting might be a better approach. You could then set CacheKey=%d-%r-%a-%i whereas for everyone else it would still default to CacheKey=%d-%r-%a.

@dzil123 dzil123 force-pushed the imageid branch 2 times, most recently from cc68306 to ecf2da7 Compare July 11, 2024 23:46
@dzil123 dzil123 changed the title Add image_id to cache tree key Add CacheKey config to override cache_tree_paths Jul 12, 2024
@@ -1776,6 +1776,7 @@ def finalize_default_initrd(
*(["--output-dir", str(output_dir)] if output_dir else []),
*(["--workspace-dir", str(config.workspace_dir)] if config.workspace_dir else []),
*(["--cache-dir", str(config.cache_dir)] if config.cache_dir else []),
*(["--cache-key", config.cache_key]),
Copy link
Author

Choose a reason for hiding this comment

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

This passes the finalized value with the specifiers replaced to the CLI call instead of the original template, but I'm not sure how else to implement this.

@dzil123
Copy link
Author

dzil123 commented Jul 12, 2024

After further investigation, I'm not a fan of this solution because it evaluates the cache_key template at the moment that config is parsed, not at build time. This means that any config changes in mkosi.profiles, mkosi.images, etc will not affect cache_key and potentially break caching if the additional configs change the set of packages.

@dzil123 dzil123 force-pushed the imageid branch 3 times, most recently from c413a54 to 9f63d8e Compare July 12, 2024 08:28
@DaanDeMeyer
Copy link
Contributor

@dzil123 We should do this via a second layer of specifiers, similar to what we do for UnifiedKernelmageFormat=, We can use & as the specifier character and only interpret the specifiers in the cache_tree_paths() function.

@dzil123
Copy link
Author

dzil123 commented Jul 18, 2024

Thank you for the feedback. Unfortunately I'm out of time to work on this. I hope someone else will pick this up.

@DaanDeMeyer
Copy link
Contributor

Let's close this for now then, I filed #2883 to track the feature request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants