Only rely on shasum for dependency cache hit #233

c0d1ngm0nk3y · 2023-04-14T09:03:09Z

fixes #167

Summary

When checking if some dependency can be reused, it should be enough to check the shasum. There is no need to verify that the metadata is the same (the binary is).

Use Cases

This caused cache misses when the metadata was different (e.g. DeprecationDate).

Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
I have added an integration test, if necessary.
I have reviewed the styleguide for guidance on my code quality.
I'm happy with the commit history on this PR (I have rebased/squashed as needed).

dmikusa

I agree with the premise here. The primary purpose is to cache the dependency that was downloaded. The shasum of the dependency should be sufficient to determine if there is a need to fetch the resource. That said, since this is a very critical piece of code, this review will take a bit longer. I need to do a lot of testing on this to be satisfied this isn't going to break something.

A couple of minor suggestions included for now.

dmikusa · 2023-04-24T17:37:13Z

dependency_cache.go

@@ -131,29 +129,27 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque
 	}

 	file = filepath.Join(d.CachePath, fmt.Sprintf("%s.toml", dependency.SHA256))
-	b, err := ioutil.ReadFile(file)
+	_, err := ioutil.ReadFile(file)


It looks like we are just checking if the file exists, so we can use https://github.com/paketo-buildpacks/libpak/blob/main/sherpa/exists.go

dmikusa · 2023-04-24T17:37:25Z

dependency_cache.go

 		d.Logger.Bodyf("%s cached download from buildpack", color.GreenString("Reusing"))
 		return os.Open(filepath.Join(d.CachePath, dependency.SHA256, filepath.Base(uri)))
 	}

 	file = filepath.Join(d.DownloadPath, fmt.Sprintf("%s.toml", dependency.SHA256))
-	b, err = ioutil.ReadFile(file)
+	_, err = ioutil.ReadFile(file)


Same comment as above, https://github.com/paketo-buildpacks/libpak/blob/main/sherpa/exists.go.

c0d1ngm0nk3y · 2023-04-26T16:07:39Z

That said, since this is a very critical piece of code, this review will take a bit longer. I need to do a lot of testing on this to be satisfied this isn't going to break something.

Thanks. Much appreciated. If tests show that there is some reason for this (which I doubt), we could probably try to add some unit test to document this.

dmikusa · 2023-04-29T20:27:01Z

Looking at this some more. There is definitely an issue with the deprecation date. I'm seeing this trigger a reload of the layer at times when it should not. It looks like the issue is when it's comparing the expected and actual metadata for the layer, the deprecation date has a timezone in one case and doesn't have a timezone in the other case (I have a suspicion this is because there are two different toml libraries being used and each is handling the date slight different). This causes the deep equals comparison to fail and the layer metadata doesn't match. I'd expect something similar can happen with the artifact downloading as it's doing something similar.

My concern with this proposal is this. There is a slight difference in the behavior. If you were to make a metadata-only change to a dependency, the way it is previously implemented would detect that change and redownload the binary. This proposal would not. Since this has been the behavior of the library forever, it's really hard for us to tell if someone might be depending on the way this works in their buildpacks.

I've been trying to think about cases where one might make a metadata-only change. The cases that come to mind are when something gets published with the wrong metadata, or when developing and you're making changes to the dependency metadata.

At any rate, what I'd like to do is this:

Propose that we make this change in the next major branch. I think that is safer.
I think we should evaluate making the same shasum-only comparison for layer caching with the dependency layer contributor. Right now, it looks at the full dependency object. It could probably also compare just the checksum. I think we should evaluate this for consistency of behavior.
For the v1 branch, fix the comparison problem with the deprecation date. I think making it a pointer so it can be nil when not set might correct the problem.

c0d1ngm0nk3y · 2023-05-03T06:44:27Z

My concern with this proposal is this. There is a slight difference in the behavior.

But isn't that normal for a bugfix?

If you were to make a metadata-only change to a dependency, the way it is previously implemented would detect that change and redownload the binary.

Yes, the very same binary with the exact same shasum. What would be gained?

This proposal would not. Since this has been the behavior of the library forever, it's really hard for us to tell if someone might be depending on the way this works in their buildpacks.

What would be a use case?

I've been trying to think about cases where one might make a metadata-only change. The cases that come to mind are when something gets published with the wrong metadata, or when developing and you're making changes to the dependency metadata.

Deprecation dates might change, right?

At any rate, what I'd like to do is this:

Propose that we make this change in the next major branch. I think that is safer.

tbh, I don't see the point since this is rather a bug to me. It screwed up my offline buildpack and judging on the linked issue, I am not the only one coming across this strange behavior. But it is of course your call.

I think we should evaluate making the same shasum-only comparison for layer caching with the dependency layer contributor. Right now, it looks at the full dependency object. It could probably also compare just the checksum. I think we should evaluate this for consistency of behavior.

Sounds good, but independent to me.

For the v1 branch, fix the comparison problem with the deprecation date. I think making it a pointer so it can be nil when not set might correct the problem.

But fiddling around with the metadata instead of fixing the root problem, sounds strange to be tbh. I would still like to understand why the metadata was compared in the first place.

loewenstein · 2023-05-10T06:24:22Z

Since this has been the behavior of the library forever, it's really hard for us to tell if someone might be depending on the way this works in their buildpacks.

I have a hard time thinking about any way someone could depend on this. Is there any way to perceive the change at all?

It's about getting a binary, isn't it? And, besides this change in the implementation, at the end you have a binary with a specific sha256. I cannot come up with any potential dependency on whether this binary was taken from the cache or downloaded.

c0d1ngm0nk3y · 2023-05-10T06:35:04Z

Propose that we make this change in the next major branch.

A v2 branch does not exist yet, right? Or are there already plans for a v2?

dmikusa · 2023-05-10T12:07:53Z

A v2 branch does not exist yet, right? Or are there already plans for a v2?

Not created yet. What we'll probably do is branch for 1.x maintenance and make main the v2 branch.

dmikusa · 2023-05-10T12:20:38Z

It's about getting a binary, isn't it?

In a perfect world, I'd say yes. The implementation of how that happens shouldn't matter. If it's comparing all the metadata or just the shasum at the end of the day you should get the same binary. I'll admit I'm looking at this in a pessimistic manner. Abstractions can be leaky though, and this wouldn't be the first time where we've tried to change something only to find that the abstraction wasn't good enough and people were depending on the behavior of the implementation. Given the age of v1 and the fact that we're planning v2 where we can easily make this change without disrupting folks, I'd prefer to go that route and not take the risk of changing the implementation.

In addition, you said: This caused cache misses when the metadata was different (e.g. DeprecationDate). so for v1 I'd prefer to focus on why the metadata is different and correcting that, instead of changing the implementation. If we can fix your cache misses this way, isn't that good enough?

In regards to the cache misses, can you elaborate on when you're seeing them Is this happening just because of the deprecation date? Or other metadata? What buildpacks and dependencies does it impact? Are these cache misses reproducible? If so, what are some steps I could take to look more closely at it?

Thanks

loewenstein · 2023-05-11T09:29:11Z

In addition, you said: This caused cache misses when the metadata was different (e.g. DeprecationDate). so for v1 I'd prefer to focus on why the metadata is different and correcting that, instead of changing the implementation. If we can fix your cache misses this way, isn't that good enough?

Sure, solving the concrete case for v1 would help. I was just really curious and had (and still have) my doubts that the additional effort (to fix the root cause in v2 and additionally band aid in v1) is justified. But it's your call of course.

c0d1ngm0nk3y · 2023-05-12T14:05:49Z

3. For the v1 branch, fix the comparison problem with the deprecation date. I think making it a pointer so it can be nil when not set might correct the problem.

That was not as straight forward as I expected due to the combination of using toml and reflect. See #240

dmikusa · 2023-05-18T04:10:08Z

@c0d1ngm0nk3y Thank you so much for putting that together. Working with reflect.DeepEqual is a pain.

I 100% want to move to just comparing the sha hash, but in v2. In v1, I'm optimistic we can make this work with a bit less risk of changing behavior.

c0d1ngm0nk3y · 2023-07-07T10:15:05Z

At any rate, what I'd like to do is this:

Propose that we make this change in the next major branch. I think that is safer.

I created #266 for this.

I think we should evaluate making the same shasum-only comparison for layer caching with the dependency layer contributor. Right now, it looks at the full dependency object. It could probably also compare just the checksum. I think we should evaluate this for consistency of behavior.

I had a look and I will create a dedicated pr for release-2.x for that.

For the v1 branch, fix the comparison problem with the deprecation date. I think making it a pointer so it can be nil when not set might correct the problem.

Fixed with #240

c0d1ngm0nk3y · 2023-07-11T12:33:38Z

Propose that we make this change in the next major branch. I think that is safer.

see #266

c0d1ngm0nk3y requested a review from a team as a code owner April 14, 2023 09:03

c0d1ngm0nk3y mentioned this pull request Apr 14, 2023

libpak fails to use cached dependencies when no CPEs are specified. #167

Closed

dmikusa requested changes Apr 24, 2023

View reviewed changes

dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Apr 24, 2023

Only rely on shasum for dependency cache hit

2d7fb31

c0d1ngm0nk3y force-pushed the shasum-caching branch from e9a1b28 to 2d7fb31 Compare April 26, 2023 16:05

c0d1ngm0nk3y mentioned this pull request May 12, 2023

Do not check DeprecationDate when comparing dependencies #240

Merged

5 tasks

Merge branch 'main' into shasum-caching

743314a

Merge branch 'main' into shasum-caching

af4fdf9

c0d1ngm0nk3y mentioned this pull request Jul 7, 2023

Only rely on shasum for dependency cache hit #266

Merged

5 tasks

c0d1ngm0nk3y mentioned this pull request Jul 7, 2023

Introduce struct with metadata to identify a dependency #267

Closed

5 tasks

c0d1ngm0nk3y closed this Jul 11, 2023

c0d1ngm0nk3y mentioned this pull request Aug 18, 2023

Introduce struct with metadata to identify a dependency #283

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only rely on shasum for dependency cache hit #233

Only rely on shasum for dependency cache hit #233

c0d1ngm0nk3y commented Apr 14, 2023 •

edited

Loading

dmikusa left a comment

dmikusa Apr 24, 2023

dmikusa Apr 24, 2023

c0d1ngm0nk3y commented Apr 26, 2023

dmikusa commented Apr 29, 2023 •

edited

Loading

c0d1ngm0nk3y commented May 3, 2023

loewenstein commented May 10, 2023

c0d1ngm0nk3y commented May 10, 2023

dmikusa commented May 10, 2023

dmikusa commented May 10, 2023

loewenstein commented May 11, 2023

c0d1ngm0nk3y commented May 12, 2023

dmikusa commented May 18, 2023

c0d1ngm0nk3y commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 11, 2023

Only rely on shasum for dependency cache hit #233

Only rely on shasum for dependency cache hit #233

Conversation

c0d1ngm0nk3y commented Apr 14, 2023 • edited Loading

Summary

Use Cases

Checklist

dmikusa left a comment

Choose a reason for hiding this comment

dmikusa Apr 24, 2023

Choose a reason for hiding this comment

dmikusa Apr 24, 2023

Choose a reason for hiding this comment

c0d1ngm0nk3y commented Apr 26, 2023

dmikusa commented Apr 29, 2023 • edited Loading

c0d1ngm0nk3y commented May 3, 2023

loewenstein commented May 10, 2023

c0d1ngm0nk3y commented May 10, 2023

dmikusa commented May 10, 2023

dmikusa commented May 10, 2023

loewenstein commented May 11, 2023

c0d1ngm0nk3y commented May 12, 2023

dmikusa commented May 18, 2023

c0d1ngm0nk3y commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 11, 2023

c0d1ngm0nk3y commented Apr 14, 2023 •

edited

Loading

dmikusa commented Apr 29, 2023 •

edited

Loading