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

stamping results in different digest with remote cache #1451

Closed
mariusgrigoriu opened this issue Mar 4, 2020 · 9 comments · Fixed by #1878
Closed

stamping results in different digest with remote cache #1451

mariusgrigoriu opened this issue Mar 4, 2020 · 9 comments · Fixed by #1878

Comments

@mariusgrigoriu
Copy link

Stamping a container image results in a different sha256 on each build. That may not be surprising in a local build scenario, but I expect the remote cache to serve up the files necessary to NOT generate a different sha256.

Repro steps:

bazel build --remote_http_cache //:image
cat bazel-bin/push_image.digest
bazel clean
bazel build --remote_http_cache //:image
cat bazel-bin/push_image.digest

Note that the digest is different between builds. I would expect the remote cache results in the same build stamped as before.

BUILD file:

container_image(
    name = "image",
    base = ":app_layer",
    stamp = True,
)

container_push(
    name = "push_image",
    format = "Docker",
    image = ":image",
    registry = "{STABLE_REGISTRY}",
    repository = "{STABLE_REPOSITORY_PATH}",
    tag = "{STABLE_IMAGE_TAG}",
)
@smukherj1
Copy link
Collaborator

In theory yes I expect rebuilding the same image using a remote cache should result in the same digest. However, I think I'll need more information to figure out what's wrong here. Could you try debugging this with modifying the digester here to dump img.RawManifest() in addition to the digest. You could then run the pusher multiple times and try to compare where the manifest differs. The digest is simply a sha256 digest of the raw manifest.

@mariusgrigoriu
Copy link
Author

Printing img.RawManifest() as a string, I get this:

{
    "schemaVersion": 2,
    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
    "config": {
        "mediaType": "application/vnd.docker.container.image.v1+json",
        "size": 926,
        "digest": "sha256:685bdc90075e05a7128c43b47d9d7de310d1ff6467c3c778fa9615c747654d29"
    },
    "layers": [
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 654467,
            "digest": "sha256:e8d8785a314f385d3675a017f4e2df1707c528c06e7a7989663fdab4900bd8ff"
        },
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 12660526,
            "digest": "sha256:3baacbc6b624269c661e5dc83f0744c6230f3ec79b9e162a4797fae10dd7b57d"
        },
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 45,
            "digest": "sha256:85cea451eec057fa7e734548ca3ba6d779ed5836a3f9de14b8394575ef0d7d8e"
        }
    ]
}

After a bazel clean I get the following:


{
    "schemaVersion": 2,
    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
    "config": {
        "mediaType": "application/vnd.docker.container.image.v1+json",
        "size": 926,
        "digest": "sha256:7687ef4a60920e4af48dd2749ca516e8b0bb619ed40b1fa93c3de28063df7976"
    },
    "layers": [
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 654467,
            "digest": "sha256:e8d8785a314f385d3675a017f4e2df1707c528c06e7a7989663fdab4900bd8ff"
        },
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 12660526,
            "digest": "sha256:3baacbc6b624269c661e5dc83f0744c6230f3ec79b9e162a4797fae10dd7b57d"
        },
        {
            "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
            "size": 45,
            "digest": "sha256:85cea451eec057fa7e734548ca3ba6d779ed5836a3f9de14b8394575ef0d7d8e"
        }
    ]
}

Only config.digest differs between the two runs.

I noticed that two processes are running locally:

INFO: 445 processes: 443 remote cache hit, 2 darwin-sandbox.

@mariusgrigoriu
Copy link
Author

I'm thinking this issue might be caused by bazelbuild/bazel#10075

The scenario is certainly similar.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_docker!

@github-actions github-actions bot added the Can Close? Will close in 30 days unless there is a comment indicating why not label Mar 18, 2021
@github-actions
Copy link

This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

@jinhong-
Copy link

jinhong- commented Jun 2, 2021

It looks like i have been experiencing similar issues. However mine is due to the use of x_defs in go_library rule. It is mentioned in the documentation that stamping does not result in a recompilation, but a re-link. Could this be why?

@alexeagle alexeagle reopened this Jun 2, 2021
@alexeagle
Copy link
Collaborator

@jinhong- I think the problem here is actually that rules_docker unconditionally stamps (for container_image based on the stamp boolean attr, for container_push and container_bundle based on whether any { characters appear in a stampable attr) while rules_go bases the logic on whether the build is configured with --stamp (as a command-line argument to Bazel)

My understanding after spending a bunch of time getting this right in rules_nodejs is that we should be doing the same thing as rules_go. I'll work up a PR but it's a breaking change so I'll need to work with other maintainers to figure out how we'll roll it out.

alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 2, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute was removed from some providers
- the stamp attribute was deprecated on container_push and container_bundle, it is now removed
- the stamp attribute was removed from container_image, there is no
replacement

Fixes bazelbuild#1451
@github-actions github-actions bot removed the Can Close? Will close in 30 days unless there is a comment indicating why not label Jun 3, 2021
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute was removed from some providers
- the stamp attribute was deprecated on container_push and container_bundle, it is now removed
- the stamp attribute was removed from container_image, there is no
replacement

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute was removed from some providers
- the stamp attribute was deprecated on container_push and container_bundle, it is now removed
- the stamp attribute was removed from container_image, there is no
replacement

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute was removed from some providers
- the stamp attribute was deprecated on container_push and container_bundle, it is now removed
- the stamp attribute was removed from container_image, there is no
replacement

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute was already deprecated on container_push and container_bundle, it is now removed
- the stamp attribute is removed from container_image, there is no replacement

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute was already deprecated on container_push and container_bundle, it is now removed
- the stamp attribute is removed from container_image, there is no replacement

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Jun 4, 2021
…he rule

This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute was already deprecated on container_push and container_bundle, it is now removed
- the stamp attribute is removed from container_image, there is no replacement

Fixes bazelbuild#1451
@sluongng
Copy link
Contributor

I have read through #1878.

In addition to just solving the problem directly, I would suggest to add some documentation around rules_docker and remote caching/RBE best practices:

  • Link reference to Guide to troubleshoot remote cache problems
  • Document best practice in build stamping, what should a normal workspace status file should include
  • Best practice when using rules_docker with/without docker daemon and with/without RBE

Perhaps a dedicated Markdown file for FAQ and/or open up Github's Discussion feature? WDYT?

@jinhong-
Copy link

jinhong- commented Jul 6, 2021

@jinhong- I think the problem here is actually that rules_docker unconditionally stamps (for container_image based on the stamp boolean attr, for container_push and container_bundle based on whether any { characters appear in a stampable attr) while rules_go bases the logic on whether the build is configured with --stamp (as a command-line argument to Bazel)

My understanding after spending a bunch of time getting this right in rules_nodejs is that we should be doing the same thing as rules_go. I'll work up a PR but it's a breaking change so I'll need to work with other maintainers to figure out how we'll roll it out.

Not sure i quite understand this. Am i correct to say that for me to fix this now, I would add stamp = True to my container_image attr?

Also, I am not sure if I understand how stamping would result in different hashes

On a separate note, maybe I have a different issue after all. My issue is that the SHA256 generated into the .digest does not match the actual digest of the actual image that is being uploaded. And this issue seem to happen because of of x_defs option. Very recently I am having similar issues again over a different scenario (I have yet to narrow it down why)

alexeagle added a commit to alexeagle/rules_docker that referenced this issue Nov 25, 2021
This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute is now trinary

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Nov 25, 2021
This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute is now trinary

Fixes bazelbuild#1451
alexeagle added a commit to alexeagle/rules_docker that referenced this issue Nov 25, 2021
This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute is now trinary

Fixes bazelbuild#1451
alexeagle added a commit that referenced this issue Nov 26, 2021
This follows the example of rules_go and rules_nodejs.

It means we don't produce cache-busting outputs unless the user
explicitly requests non-deteriminism in the build with --stamp.

BREAKING CHANGE:
- To get stamped outputs, users must now call bazel with `--stamp`
- the stamp attribute is removed from some providers
- the stamp attribute is now trinary

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

Successfully merging a pull request may close this issue.

5 participants