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

Use new build URL #930

Closed
wants to merge 2 commits into from
Closed

Use new build URL #930

wants to merge 2 commits into from

Conversation

rail
Copy link
Contributor

@rail rail commented Jun 3, 2019

According to
https://github.com/mozilla-services/cloudops-infra-deploylib/blob/7d668384e41fbf936af3b706a9a3cce2d10499ff/deploylib/docker.py#L152
the digest of the docker image should be in the log. The digest printed
using the locally built image doesn't produce the same sha256 with the
one pushed to Docker Hub, because Taskcluster uses an older version of
docker.

According to
https://github.com/mozilla-services/cloudops-infra-deploylib/blob/7d668384e41fbf936af3b706a9a3cce2d10499ff/deploylib/docker.py#L152
the digest of the docker image should be in the log. The digest printed
using the locally built image doesn't produce the same sha256 with the
one pushed to Docker Hub, because Taskcluster uses an older version of
docker.
@rail rail requested a review from bhearsum June 3, 2019 15:27
@rail rail self-assigned this Jun 3, 2019
@@ -45,6 +45,9 @@ for tag in ${tags[*]}; do
docker tag buildtemp "mozilla/balrog:${tag}"
echo "Pushing Docker image tagged with ${tag}"
docker push mozilla/balrog:${tag}
# Pull the image to print its digest. cloudops-deploylib verifies the
# images comparing their digests to the digests in the logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revise this comment? I think the pull is necessary because the digest changes when you push it (?), but this comment doesn't make it clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does pulling defeat the purpose of printing the digest in the log? My assumption is that it is printed so we don't have to trust docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does pulling defeat the purpose of printing the digest in the log? My assumption is that it is printed so we don't have to trust docker.

It does. So far I haven't found any better way to find the digest using this old image format... :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some investigation it turns out that the digest is something registry specific, so you have to pull or push the image to get the digest. Since we push in the same task, there is no need to pull again, docker push prints the digest.

@bhearsum
Copy link
Contributor

bhearsum commented Jun 3, 2019

It's worthing noting the current state is that images are not verified, so this patch doesn't regress in any way. We should try to find a way to get it working though.

@rail
Copy link
Contributor Author

rail commented Jun 4, 2019

Let me investigate it more and keep this PR open as a reminder.

@rail rail requested a review from bhearsum June 4, 2019 03:25
@rail rail changed the title Print docker digest and change build URL Use new build URL Jun 4, 2019
@bhearsum
Copy link
Contributor

bhearsum commented Jun 4, 2019

Without any changes to push-dockerimage.sh, I'm not sure how this is going to fix validation? If docker push prints the digest already, image verification should already work?

@rail
Copy link
Contributor Author

rail commented Jun 4, 2019

Yes, image validation should work, just need to fix the URL to the new format, it's used in parsing the task ID.

@bhearsum
Copy link
Contributor

bhearsum commented Jun 4, 2019

Yes, image validation should work, just need to fix the URL to the new format, it's used in parsing the task ID.

The old URL still works though AFAICT, so how will this patch help? Deploylib pulls the task id out with:

job_key = build_url.split("/")[-1][1:]

Which seems to pull the taskId out OK:

$ python -c "print('https://tools.taskcluster.net/task-inspector/#GB3uq1QpQiSaOkUdFq779A'.split('/')[-1][1:])"
GB3uq1QpQiSaOkUdFq779A

And then it generates the log with:

log_url = "https://taskcluster-artifacts.net/{}/0/public/logs/live_backing.log".format(job_key)

Which works fine:

$ curl -vL https://taskcluster-artifacts.net/eKZSvGRER-q5wo1LJg4hHg/0/public/logs/live_backing.log
< HTTP/2 200

@rail
Copy link
Contributor Author

rail commented Jun 4, 2019

Bah, I misread the code and missed the [1:] part. Hmm, this makes me think that the new style won't work. Let me create a PR against deploylib.

@rail rail closed this Jun 4, 2019
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 this pull request may close these issues.

None yet

3 participants