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

cache-invalidation based on image's contentDigest(a.k.a. imageId) #15678

Closed
wants to merge 3 commits into from

Conversation

x7upLime
Copy link
Contributor

In order to accomplish contentDigest-based cache-invalidation,
this proposes a very simple mechanism based on cache's filesystem:

KicDriver's stored images are checked against the cache based on contentDigest,
the contentDigest from the cached image is retrieved reading the manifest.json file inside the tarball,
the tarball is selected based on sanitized image name.

related to #15677
solves #currently_nothing.. but would facilitate the work in accomplishing #15491
and solving related issues..

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @x7upLime. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: x7upLime
Once this PR has been reviewed and has the lgtm label, please assign sharifelgamal for approval by writing /assign @sharifelgamal in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

pkg/drivers/kic/types.go Outdated Show resolved Hide resolved
pkg/drivers/kic/types.go Outdated Show resolved Hide resolved
x7upLime added a commit to x7upLime/minikube that referenced this pull request Jan 22, 2023
From pr kubernetes#15678 we're able to address content inside the kicDriver
based on contentDigest(a.k.a. imageID, a.k.a. ID). So that archived
images inside the kicBase cache, can be addressed and loaded to a more
generic kicDriver entity, rather than worrying about how each
container engine treats the distributionDigest/Tag/Name triple
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 24, 2023
Being pkg/drivers/kic/types.go the source of truth for the version of
the container we're using to instantiate our kübernetes cluster in,
the pr should start here..

Initially I thought about hardcoding the contentDigest(a.k.a. imageId)
here as well, to then use it to check against the images inside the
kicDriver.. It later took another turn(we're retrieving it from tar).

Plus a collaborator showed me that it was a bad idea.. maintaining it
here would bean bumping it as part of the image build process.

The idea is based on the following concepts:

.contentDigest is the most reliable way to address image content:
if the image is tampered with after push to a registry,
the contentDigest we'd see after pull,
would be different than the one hardcoded here.
It is also part of the image itself, i.e. part of the tar archive;
thus giving us a way to always know if the cache is up to date,
even offline.

.distributionDigest is the most reliable way to determine which
image we're looking to pull from a registry; a tag can be detached
from an image and recycled, referencing another one, with different
content.
It is not part of the image itself; it is computed on the image in
compressed state.. and since different engines/mechanisms could use
different types of compression, this digest is totally unreliable
as a way to address content.

[*] refs:
https://windsock.io/explaining-docker-image-ids/
google/go-containerregistry#895 (comment)
https://stackoverflow.com/questions/45533005/why-digests-are-different-depend-on-registry
https://blog.aquasec.com/docker-image-tags -- follow links
Ther are two versions of imagePathInCache:
one in pkg/minikube/download
one in pkg/minikube/image
They refer to two different caches..
I thought that we could use the one referring to the kicBase cache,
instead of rewriting the same thing inside pkg/minikube/node
The idea is that the distributionDigest was not
meant to be used as something content-addressable.
So instead of invalidating the kicBase cache based on the
distributionDigest present in the kicDriver's storage,
we could move to something like this:

.User expresses preferences in regard of kicBase image,
 by specifying the image by name:tag@digest, name:tag,... whatever
.The image's specified name is sanitized
 and used as filename for the tarbal in minikube's cache[*]
.Image stored inside kicDriver(docker, podman,...) is validated
 against the image's contentDigest(imageID), which we're retrieving
 directly from the .tar archive.. which we're selecting from the
 cache, by sanitized name

I initially tried implementing a mechanism based on a json file that
would contain entries in regard of cached images, plus other infos
that we would need to address content, like
distributionDigest(registry digest)...
Something like a repositories.json file.. in a docker fashion
(/var/lib/docker/image/overlay2/repositories.json)
That would lead to complications like
"what if user removes a .tar to save space?",
(basically.. how we keep the file up-to-date with cache content)
that would add extra complexity in solving.. like
some logic that would be called on startup, that would read all
cached files and reflect in the file.

As far as I can understand our usecase, which doesn't seem very
complex(I might be wrong..), some kind of simple/raw mechanism like
the one in this commit could suffice..
Like we don't have to maintain a lot of images..
The source of truth is hardcoded in the sources..
the image/file relation for the kicBase cache
is based on the sanitize(img) mechanism..
...

[+] refs..
https://windsock.io/explaining-docker-image-ids/
https://blog.aquasec.com/docker-image-tags
https://stackoverflow.com/questions/45533005/why-digests-are-different-depend-on-registry

[*] TODO: this could be one possible shortcoming..
  - what if user wants the image:tag to be always up-to-date with
    registry? perhaps some flag?
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 26, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 27, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 27, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants