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

genpolicy: add missing cache improvements #181

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

Redent0r
Copy link

@Redent0r Redent0r commented Apr 22, 2024

Merge Checklist
  • Followed patch format from upstream recommendation: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
    • Included a single commit in a given PR - at least unless there are related commits and each makes sense as a change on its own.
  • Aware about the PR to be merged using "create a merge commit" rather than "squash and merge" (or similar)
  • genPolicy only: Ensured the tool still builds on Windows
  • genPolicy only: Updated sample YAMLs' policy annotations, if applicable
  • The upstream-missing label (or upstream-not-needed) has been set on the PR.
Summary

Add missing cache improvements specifically missing in containerd pull. I'm not sure how I missed this: It was already using layers-cache.json and it produces the exact same cache after using these changes.

These improvements are already included in kata-containers#9530 and based on #113

Test Methodology

https://dev.azure.com/mariner-org/mariner/_build/results?buildId=555763&view=results

Add missing cache improvements specifically missing in containerd pull

Signed-off-by: Saul Paredes <[email protected]>
@Redent0r Redent0r added the upstream/merged PRs that have been merged upstream label Apr 22, 2024
@@ -333,10 +321,10 @@ async fn get_verity_hash(
}

if !error {
match std::fs::read_to_string(&verity_path) {
match get_verity_hash_value(&decompressed_path) {
Copy link
Author

@Redent0r Redent0r Apr 22, 2024

Choose a reason for hiding this comment

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

@@ -317,14 +308,11 @@ async fn get_verity_hash(

if verity_hash.is_empty() {
// go find verity hash if not found in cache
if let Err(e) = create_verity_hash_file(
use_cached_files,
if let Err(e) = create_decompressed_layer_file(
Copy link
Author

Choose a reason for hiding this comment

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

@@ -359,74 +347,41 @@ async fn get_verity_hash(
Ok(verity_hash)
}

async fn create_verity_hash_file(
Copy link
Author

Choose a reason for hiding this comment

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

No longer need create_verity_hash_file after cache improvements. Now we call create_decompressed_layer_file directly https://github.com/microsoft/kata-containers/pull/113/files#diff-754926dd5feb413b73f033fd1484312752a69e1b24d71fc57e422316b90c587bL329

@@ -444,25 +399,3 @@ async fn create_decompressed_layer_file(

Ok(())
}

fn do_create_verity_hash_file(path: &Path, verity_path: &Path) -> Result<()> {
Copy link
Author

Choose a reason for hiding this comment

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

We no longer need this and instead reuse get_verity_hash_value from registry.rs

};
let req = with_namespace!(req, "k8s.io");
let mut c = client.content();
let resp = c.read(req).await?;
Copy link
Author

Choose a reason for hiding this comment

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

This is the reason we can't reuse create_decompressed_layer_file and other adjacent methods from registry.rs. Here we fetch the content in a different way (using containerd_client)

@Redent0r Redent0r marked this pull request as ready for review April 22, 2024 22:30
@Redent0r Redent0r requested review from a team as code owners April 22, 2024 22:30
Copy link
Collaborator

@SethHollandsworth SethHollandsworth left a comment

Choose a reason for hiding this comment

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

lgtm!

@Redent0r Redent0r merged commit b4c814c into msft-main Apr 23, 2024
182 of 250 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream/merged PRs that have been merged upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants