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

models: link current variant more carefully #1329

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Feb 17, 2021

Description of changes:

We were still seeing some rare, random build failures after #1303, and tracked it down to the fact that cargo calls models' build.rs script multiple times: once for the build-dep in storewolf, once for the (regular) dep in storewolf, and once (much later) for the dep in the static build of apiclient. The first two have a chance of running at the same time and racing to delete/create the symlink, causing a failure. (I think it's correct for cargo to run it multiple times, since each of those is running in a different environment; build-dep vs. dep, for example, use the host build toolchain versus the target build toolchain.)

Example error from #1318: Failed to create symlink at 'src/variant/current' pointing to '../aws-ecs-1' - we need this to support different API models for different variants. Error: File exists (os error 17)"

This PR makes a few related changes to make this process safer. First, we only run the model build.rs once, during the "host" build for the build-dep in storewolf, by checking CARGO_CFG_TARGET_VENDOR. Second, in case they do happen to run at the same time for other unexpected reasons, it now uses an atomic link-swap rather than a remove/create so we can't race. Third, just to be tidy, it alerts cargo that it should rerun if either of our links change, though this should only happen if someone manually deletes a link or something.

    models: fix TOCTOU in build.rs by safely swapping links into place
    models: rerun if variant/mod links change
    
    We don't expect these links to change outside of this script, but if they do,
    we need to rerun this build.rs and reset the links properly.
    models: only run build.rs once, for host

Testing done:

I built and ran a k8s AMI OK, then an ECS AMI, then a k8s AMI again. (This is to ensure I didn't reintroduce the issue solved in #1319, and for general health.)

I checked the build log and saw that later build.rs runs exited quickly:

#17 1.117      Running `/home/builder/.cache/release/build/models-b586c88a1f2e4d4b/build-script-build`
#17 1.117      Running `/home/builder/.cache/release/build/models-b586c88a1f2e4d4b/build-script-build`
#17 1.122 warning: Already ran model build.rs for host, skipping for target
...
#17 115.4      Running `/home/builder/.cache/.static/release/build/models-b87a178a21838d64/build-script-build`
#17 115.4 warning: Already ran model build.rs for host, skipping for target

Also, local testing shows the symlink being swapped around OK. Starting with aws-k8s-1.17 linked, rebuilding keeps it:

> ls -ahlF --color=auto src/variant/current
lrwxrwxrwx. 1 tjk tjk 15 02-17 14:59 src/variant/current -> ../aws-k8s-1.17/
> cargo build
> ls -ahlF --color=auto src/variant/current
lrwxrwxrwx. 1 tjk tjk 15 02-17 15:42 src/variant/current -> ../aws-k8s-1.17/

Changing variant works:

> env VARIANT=aws-dev cargo build
> ls -ahlF --color=auto src/variant/current
lrwxrwxrwx. 1 tjk tjk 10 02-17 15:43 src/variant/current -> ../aws-dev/

Removing the files gets them recreated:

> rm src/variant/current
> cargo build
> ls -ahlF --color=auto src/variant/current
lrwxrwxrwx. 1 tjk tjk 15 02-17 15:43 src/variant/current -> ../aws-k8s-1.17/

Changing them does too:

> ln -snf bad src/variant/current
> cargo build
> ls -ahlF --color=auto src/variant/current
lrwxrwxrwx. 1 tjk tjk 15 02-17 15:45 src/variant/current -> ../aws-k8s-1.17/

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

We don't expect these links to change outside of this script, but if they do,
we need to rerun this build.rs and reset the links properly.
sources/models/build.rs Show resolved Hide resolved
sources/models/build.rs Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

💯

@tjkirch tjkirch merged commit b626497 into bottlerocket-os:develop Feb 18, 2021
@tjkirch tjkirch deleted the model-link branch February 18, 2021 18:44
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.

5 participants