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

layers: add new TOCDigest attribute #1719

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Sep 21, 2023

introduce the TOCDigest field for a layer. TOCDigest is designed to store the digest of the Table of Contents (TOC) of the blob.

It is useful when the UncompressedDigest cannot be validated during a partial image pull, but the TOC itself is validated

@giuseppe
Copy link
Member Author

needed by containers/image#1980

Should not be merged until we agree on the design in the c/image PR

layers.go Outdated
if layer.Flags == nil {
layer.Flags = make(map[string]interface{})
}
layer.Flags[ExpectedLayerDiffIDFlag] = expectedDiffID
Copy link
Member

Choose a reason for hiding this comment

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

If diffOutput.UncompressedDigest isn't empty, its value is already being saved in the layer's UncompressedDigest field. How does storing a duplicate of it as a flag change anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

ApplyDiffFromStagingDirectory replaces diffOutput.UncompressedDigest with the TOC Digest.

And we are abusing UncompressedDigest since the TOC Digest was validated, while the layer uncompressed digest was not as we have not pulled the whole layer

@rhatdan
Copy link
Member

rhatdan commented Oct 26, 2023

@giuseppe @nalind what is going on with this PR?

@giuseppe
Copy link
Member Author

@giuseppe @nalind what is going on with this PR?

this is a requirement for containers/image#1980. It depends on the other PR whether this is the right approach

@giuseppe giuseppe force-pushed the partial-images-store-expected-diffid branch from 4a45ae2 to 328a826 Compare November 30, 2023 13:54
@giuseppe giuseppe changed the title layers: store the expected diff id for the layer layers: add new TOCDigest attribute Nov 30, 2023
@giuseppe giuseppe marked this pull request as ready for review November 30, 2023 20:25
@giuseppe
Copy link
Member Author

@mitr I've added a new field TOCDigest to the layer and updated the c/image PR to use it

@rhatdan
Copy link
Member

rhatdan commented Nov 30, 2023

LGTM
@mtrmac @nalind PTAL

@giuseppe giuseppe force-pushed the partial-images-store-expected-diffid branch 4 times, most recently from 582430d to 06c5afd Compare December 1, 2023 21:31
Comment on lines +318 to +321
ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error)

// ApplyDiffFromStagingDirectory uses stagingDirectory to create the diff.
ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffOpts) error
ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are the ~only users of ApplyDiffOpts. Is there a reason to introduce a new type instead of extending the existing one?


The new Flags field is not really used in this PR anyway — are there going to be users?

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason why I've added a new type is that the existing struct is used as the output, and I wanted to make clear the data that is passed to the function and the data that is returned.

The user is containers/image@cbea9be#diff-2d4a826a6986cd3a8f73b9fb0e68b27e87caaefb0703a5b1695255e4d22a7d5aR673-R678

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m sorry, ApplyDiffOpts is actually used in non-TOC ApplyDiff; I confused this value with DriverWithDifferOutput somehow.


Thanks for the pointer,

If the new Flags are only set, and only read, when calling ApplyDiffFromStagingDirectory, shouldn’t they only supplied to that function, and not also added to ApplyDiffWithDiffer?

And, at least right now (?)ApplyDiffFromStagingDirectory doesn’t seem to use the existing ApplyDiffOpts for anything, so maybe that can be replaced by a new struct which only carries the Flags, without having to nest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can surely simplify the call to ApplyDiffFromStagingDirectory with a simpler struct, but would it be worth though? In this way we can extend the ApplyDiffWithDifferOpts struct without having to change the API and the caller needs to fill just one struct to reuse for both calls.

But if you have a strong opinion about it, I'll split ApplyDiffWithDifferOpts in two separate structs

Copy link
Collaborator

@mtrmac mtrmac Dec 5, 2023

Choose a reason for hiding this comment

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

I don’t have a good intuition about it. My default is that each API call should have its own, specific and individually documented, …Opts struct unless there is a specific reason to share.

Maybe there is a good reason to share? At least looking at the one existing implementation so far, I can’t see it; but I have no idea how this might change e.g. with more native composefs features.

@giuseppe giuseppe force-pushed the partial-images-store-expected-diffid branch from 06c5afd to 58cf12f Compare December 4, 2023 15:25
@giuseppe
Copy link
Member Author

giuseppe commented Dec 4, 2023

pushed a new version with the comments addressed, except the one about applyDiffWithOptions

introduce the TOCDigest field for a layer. TOCDigest is designed to
store the digest of the Table of Contents (TOC) of the blob.

It is useful when the UncompressedDigest cannot be validated during a
partial image pull, but the TOC itself is validated.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The UncompressedDigest has not been validated by the driver so leave
it unset.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the partial-images-store-expected-diffid branch from 58cf12f to 15ac716 Compare December 4, 2023 15:58
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Still #1719 (comment); I don’t see any record of why that is needed (perhaps it is a historical artifact of the original ExpectedLayerDiffIDFlag?)

Other than that, implementation mechanism LGTM.

@rhatdan
Copy link
Member

rhatdan commented Dec 5, 2023

Merging we can continue working on this .

/approve
/lgtm

Copy link
Contributor

openshift-ci bot commented Dec 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

Successfully merging this pull request may close these issues.

4 participants