-
Notifications
You must be signed in to change notification settings - Fork 0
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
When creating volume caches, incorporate "pack volume key" to avoid name collisions #48
Conversation
Signed-off-by: Natalie Arellano <narellano@vmware.com>
bab1110
to
1cc1cc8
Compare
Signed-off-by: Natalie Arellano <narellano@vmware.com>
pkg/cache/volume_cache.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
sum := sha256.Sum256([]byte(imageRef.Name() + volumeKey)) // TODO: investigate if there are better ways to do this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate some feedback on this aspect of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume using an md5sum
of the actual volume is too expensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and the value would also change over time as stuff gets added and removed from the volume. We need something that is deterministic with the image name + key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good
|
||
// first, look for key in env | ||
|
||
foundKey = os.Getenv(EnvVolumeKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be one of the pack
client options? Then the cobra side of the processing can find the env var or default it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? But, we don't have a corresponding flag for it - it's all env var based. I would say this is pretty similar to how we have DefaultRegistry()
and DefaultConfigPath()
sprinkled throughout the code. I didn't want to have to pass the value all the way from commands
-> client.BuildOptions
-> down through build.LifecycleOptions
when we could just read it at the point where it's needed.
pkg/cache/volume_cache.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
sum := sha256.Sum256([]byte(imageRef.Name() + volumeKey)) // TODO: investigate if there are better ways to do this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume using an md5sum
of the actual volume is too expensive?
@@ -24,7 +27,7 @@ func TestVolumeCache(t *testing.T) { | |||
color.Disable(true) | |||
defer color.Disable(false) | |||
|
|||
spec.Run(t, "VolumeCache", testCache, spec.Parallel(), spec.Report(report.Terminal{})) | |||
spec.Run(t, "VolumeCache", testCache, spec.Sequential(), spec.Report(report.Terminal{})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is required because we are writing to disk the volume-keys.toml
, I am ok with it because I think these tests are not going to increase the CI too much but if it is possible to keep it as parallel it will be awesome
I am ok with this change, my only doubt was if we are doing this:
From the solution proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left a few comments but I am ok with it
…ontainer Signed-off-by: Natalie Arellano <narellano@vmware.com>
Great catch @jjbustamante! I added the warning in 331ac6d |
Fixes buildpacks/pack#2224
(still has some TODOs, but there is enough to get the direction)