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.

Sign up for GitHub

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

Jump to bottom

fix(oci): push archive layers if layer count exceeds max #1847

Merged
merged 1 commit into from
Oct 11, 2023
Merged

fix(oci): push archive layers if layer count exceeds max #1847

merged 1 commit into from
Oct 11, 2023

Conversation

vdice
Copy link
Member

@vdice vdice commented Oct 4, 2023

Closes #1787

@vdice vdice force-pushed the fix/oci-combine-layers-tar-gz branch 4 times, most recently from 7ad9b72 to 9bd91d9 Compare October 6, 2023 15:31
@vdice vdice marked this pull request as ready for review October 6, 2023 15:31
lann
lann reviewed Oct 6, 2023
View reviewed changes
crates/app/src/locked.rs Outdated Show resolved Hide resolved
@vdice
Copy link
Member Author

vdice commented Oct 10, 2023

Ok @lann, ready for another review. Per our sync, b9ce749 brings in the following changes:

  • in push, add component.file entry for every file to be archived but only push the archive layer to the oci registry
  • in pull, when we have an archive layer, unarchive and write files to expected locations in cache (by digest)
    • this logic wasn't quite as clean as I had hoped -- any pointers to simplify?
  • revert changes in loader.rs

lann
lann reviewed Oct 10, 2023
View reviewed changes
crates/loader/src/cache.rs Outdated Show resolved Hide resolved
crates/oci/src/client.rs Outdated Show resolved Hide resolved
crates/oci/src/client.rs
}
}
}
}
_ => {
let _ = this.cache.write_data(&bytes, &layer.digest).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

...this one too

Suggested change
let _ = this.cache.write_data(&bytes, &layer.digest).await;
this.cache.write_data(&bytes, &layer.digest).await?;

crates/oci/src/client.rs Outdated Show resolved Hide resolved
crates/oci/src/utils.rs Outdated Show resolved Hide resolved
@vdice vdice force-pushed the fix/oci-combine-layers-tar-gz branch from 71640b3 to c4c59de Compare October 10, 2023 21:53
@vdice
Copy link
Member Author

vdice commented Oct 11, 2023

35dbd84 brings a slight improvement: pairs archive and unarchive logic into the one utils file, for (hopefully) easier refactoring (eg async -> sync, diff compression algorithm...)

lann
lann approved these changes Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

I think Client generally needs some TLC but that's not this PR's fault.

@vdice
Copy link
Member Author

vdice commented Oct 11, 2023

Thanks @lann. 3f7d2ce just moves the debug log.

Going to squash/clean up commits next...

@vdice
fix(oci): bundle component files into archive layer per source entry
be87f0c 
          if number of layers otherwise exceeds max

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>
@vdice vdice force-pushed the fix/oci-combine-layers-tar-gz branch from 3f7d2ce to be87f0c Compare October 11, 2023 16:09
@vdice vdice merged commit c2bde92 into fermyon:main Oct 11, 2023
10 checks passed
@vdice vdice deleted the fix/oci-combine-layers-tar-gz branch October 11, 2023 18:36
This was referenced Oct 11, 2023
Merged
Merged
@itowlson itowlson mentioned this pull request Sep 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lann lann lann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OCI: Max layer count per app
2 participants
@vdice @lann