Add cleanup before docker run in incremental loader

[clintharrison]

When #667 was merged, the assumption was that the cleanup handler does not do anything -- but we've since seen a lot of files left around in /tmp on our CI machines from this change.

Since we've already loaded the layers into the docker daemon before docker runing, we can run the cleanup before we exec docker.

As an aside...
I'm slightly hesitant that the cleanup step could take a while, which means we're making the race condition from docker tag and a much-delayed docker run worse.

Maybe the tag_reference in the built run_statements should use tag@sha256:digest format, since we do have the digest available?

[k8s-ci-robot]

Hi @clintharrison. Thanks for your PR.

I'm waiting for a bazelbuild 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.

Details

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.

[smukherj1]

Hi @clintharrison,

Thanks for your PR. The change LGTM. Could you explain what you mean by

I'm slightly hesitant that the cleanup step could take a while, which means we're making the race condition from docker tag and a much-delayed docker run worse.

Are you referring to the tag statements here? I thought these statements are executed synchronously. Why would they introduce a race condition?

[clintharrison]

@smukherj1 yes, exactly those: imagine the scenario

process 1 (bazel run)	process 2 (maybe another bazel run)
docker tag sha256:foo bazel/some:target
cleanup	docker tag sha256:bar bazel/some:target
(cleanup still deleting files)	...
docker run bazel/some:target	...

At the end of this, we'd end up running a different image than we originally tagged.

Since we have the digest, we could instead do docker run bazel/some@sha256:foo so this isn't an issue.
Happy to send out another PR for this and have the conversation there :)

[smukherj1]

@clintharrison Ok I see. I'm not sure if we actually support building the same target across different bazel invocations in the same WORKSPACE. @nlopezgi any idea what bazel does in this scenario?

Regardless, this change LGTM until we figure out if we need to fix the race condition you described

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clintharrison, smukherj1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [smukherj1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[phb]

This change is causing the cleanup to run twice in the cases where there are no run_statements defined. (second time will fail with an error since the files are already removed)

[clintharrison]

Ahh, I totally see how that happens :( Let me send a follow-up PR.