switch to fixed directory for twoliter tools #102

bcressey · 2023-10-09T19:17:54Z

Issue #, if available:
N/A

Description of changes:
I've been experimenting with a tool to forward file descriptors into a build, which requires the ability to run something larger than a shell script during the Docker execution. That hits the limitation of secrets which can't be larger than 500 KiB.

Switching to a fixed install location under build/tools avoids this limitation and makes the code a bit more intuitive. It's also relatively easy to see what twoliter has installed.

Because the tools need to be passed in the build context, I've also imported the current .dockerignore file from the core repo and renamed it to Dockerfile.dockerignore, so that it will be used by the BuildKit backend.

Testing done:
Built some variants successfully. I also confirmed through various hacks that the .dockerignore file was being respected. For example, excluding /build/rpms/*.rpm caused the expected build failure.

Terms of contribution:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

webern · 2023-10-09T22:53:51Z

twoliter/src/cmd/make.rs

-        let tempdir = tools_tempdir()?;
-        install_tools(&tempdir).await?;
-        let makefile_path = tempdir.path().join("Makefile.toml");
+        let toolsdir = project.project_dir().join("build/tools");


Maybe the project object should provide such things (build_dir, tools_dir, etc)

Suggested change

let toolsdir = project.project_dir().join("build/tools");

let toolsdir = project.tools_dir();

webern · 2023-10-09T22:57:47Z

twoliter/src/cmd/make.rs

-        let makefile_path = tempdir.path().join("Makefile.toml");
+        let toolsdir = project.project_dir().join("build/tools");
+        let _ = std::fs::remove_dir_all(&toolsdir);
+        std::fs::create_dir_all(&toolsdir)?;


Use tokio. Also I don't think even tokio gives a helpful error message. I have planned to add this to common.rs for convenience. The idea would be to move error message creation somewhere else.

mod fs { pub(crate) async fn create_dir_all(path: impl AsRef<Path>) -> Result<()> { tokio::fs::create_dir_all(path.as_ref()).context(format!("Unable to create directory '{}'", path.as_ref().display()); } }

webern · 2023-10-09T22:58:56Z

twoliter/src/cmd/make.rs

@@ -34,9 +33,11 @@ pub(crate) struct Make {
 impl Make {
    pub(super) async fn run(&self) -> Result<()> {


@ecpullen these are changes to the code that you are moving.

webern · 2023-10-09T23:00:37Z

twoliter/src/tools.rs

    // Write out the embedded tools and scripts.
    unpack_tarball(&dir)
        .await
        .context("Unable to install tools")?;

+    // Pick one of the embedded files for use as the canonical mtime.


Why do we need a canonical mtime? Is this preventing docker rebuilds?

Why do we need a canonical mtime? Is this preventing docker rebuilds?

I'll add a comment to the code. It's a minor optimization to make the build context more efficient by not always copying the tools through.

For example on a local build with a 180 MiB file:

❯ docker build --progress=plain . ... #10 [internal] load build context #10 sha256:8fe1ebd2168d7202972f3fed513e650e6bc14c1252ef426d770005b1b769b70b #10 transferring context: 183.54MB 0.5s done #10 DONE 0.6s ❯ docker build --progress=plain . ... #10 [internal] load build context #10 sha256:942722aa20dadaa1f21a60c65ffa04d801e48ac344603e42b368fda45457366a #10 transferring context: 291B done #10 DONE 0.0s ❯ touch -r Dockerfile NVIDIA-Linux-aarch64-470.82.01.run ❯ docker build --progress=plain . ... #10 [internal] load build context #10 sha256:8fe1ebd2168d7202972f3fed513e650e6bc14c1252ef426d770005b1b769b70b #10 transferring context: 183.54MB 0.5s done #10 DONE 0.6s ❯ docker build --progress=plain . ... #10 [internal] load build context #10 sha256:ec46d1475e765e302cc924f8682c05c897c1a56df7a3b356bae7b1b866eb4c74 #10 transferring context: 291B done #10 DONE 0.0s

The build context is more intelligent than a Docker layer in the sense that modifying a single file results in only that file being transferred, not the entire build context. But it still seems worth avoiding unnecessary copies.

The direction I'm exploring with passing file descriptors could let us get rid of all the changes to the build context. /packages/*/*.tar.*, /build/rpms, /.gomodcache, and /.cargo are the current offenders. I'd like to avoid others.

ecpullen · 2023-10-10T17:21:40Z

twoliter/build.rs

@@ -20,12 +20,14 @@ fn main() {
    let paths = Paths::new();
    println!("cargo:rerun-if-changed={}", paths.data_input_dir.display());

+    let _ = fs::remove_dir_all(&paths.prep_dir);


If we can avoid updating build.rs, that would be great. The entire file will be changed in an upcoming pr.

If we can avoid updating build.rs, that would be great. The entire file will be changed in an upcoming pr.

The addition of a file appears to be necessary for this PR, that is Dockerfile.dockerignore, so at least one change in build.rs is not optional. Anyway, the changes here should be easy to replay. Delete the prep dir and add a file.

webern · 2023-12-07T20:08:31Z

/~https://github.com/bottlerocket-os/twoliter/compare/c05d9e1e24afb6b223b93234c68bda340400619d..c08da4cf3730fe54f616cee4b5bec52eef130fd2

I think this rebase is correct.

webern

I can follow up with some of the refactoring things I mentioned.

Secrets have a maximum size of 500 KiB, which is not sufficient to pass binaries to the build. This would prevent us from using native code rather than shell scripts. Install the tools into `build/tools` instead, taking care to clean up any previous installs and to reset the file modification times so as not to bust any caches or trigger unnecessary rebuilds. Signed-off-by: Ben Cressey <bcressey@amazon.com>

This leaves an empty build directory when `cargo make clean` runs. Signed-off-by: Ben Cressey <bcressey@amazon.com>

Signed-off-by: Ben Cressey <bcressey@amazon.com>

The build system has very specific requirements on what to include in the Docker build context in order to work as expected. Embedding the file avoids the need for a synchronized change in other projects when updating Twoliter. Signed-off-by: Ben Cressey <bcressey@amazon.com>

Otherwise, files that were present in an earlier build that have been renamed or removed might still be in the prep directory, where they will be added to the new archive. Signed-off-by: Ben Cressey <bcressey@amazon.com>

webern · 2023-12-19T20:43:10Z

Rebased (twice because of a mistake):

webern · 2023-12-19T20:44:21Z

twoliter/src/cmd/make.rs

@@ -1,9 +1,10 @@
 use crate::cargo_make::CargoMake;
-use crate::project;
-use crate::tools::{install_tools, tools_tempdir};
+use crate::project::{self};


Suggested change

use crate::project::{self};

use crate::project;

webern · 2023-12-22T23:11:24Z

I am merging this. I've been bitten for days with rpm2img changes not being reflected in the docker secret mount. It seems like docker is using a cached version of the file for the secret instead of the newly written-out changed version. I'm assuming this would fix the problem and we have two green check marks, so YOLO merging.

bcressey requested a review from webern October 9, 2023 19:18

webern reviewed Oct 9, 2023

View reviewed changes

ecpullen approved these changes Oct 10, 2023

View reviewed changes

bcressey mentioned this pull request Oct 12, 2023

twoliter: Create a test project #106

Merged

webern force-pushed the twoliter-tools branch from c05d9e1 to c08da4c Compare December 7, 2023 20:07

webern approved these changes Dec 7, 2023

View reviewed changes

webern force-pushed the twoliter-tools branch from c08da4c to c0ef5c4 Compare December 19, 2023 20:40

bcressey added 5 commits December 19, 2023 12:42

twoliter: add target to clean tools directory

cb1aa17

This leaves an empty build directory when `cargo make clean` runs. Signed-off-by: Ben Cressey <bcressey@amazon.com>

twoliter: fix whitespace in embedded Dockerfile

8419d5c

Signed-off-by: Ben Cressey <bcressey@amazon.com>

twoliter: clean up prep dir between builds

d7aba25

Otherwise, files that were present in an earlier build that have been renamed or removed might still be in the prep directory, where they will be added to the new archive. Signed-off-by: Ben Cressey <bcressey@amazon.com>

webern force-pushed the twoliter-tools branch from c0ef5c4 to d7aba25 Compare December 19, 2023 20:42

webern reviewed Dec 19, 2023

View reviewed changes

webern merged commit 3d7a207 into bottlerocket-os:develop Dec 22, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to fixed directory for twoliter tools #102

switch to fixed directory for twoliter tools #102

bcressey commented Oct 9, 2023

webern Oct 9, 2023

webern Oct 9, 2023

webern Oct 9, 2023

webern Oct 9, 2023

bcressey Oct 10, 2023

ecpullen Oct 10, 2023

webern Oct 10, 2023

webern commented Dec 7, 2023 •

edited

Loading

webern left a comment

webern commented Dec 19, 2023

webern Dec 19, 2023

webern commented Dec 22, 2023

	let toolsdir = project.project_dir().join("build/tools");
	let toolsdir = project.tools_dir();

		@@ -34,9 +33,11 @@ pub(crate) struct Make {
		impl Make {
		pub(super) async fn run(&self) -> Result<()> {

switch to fixed directory for twoliter tools #102

switch to fixed directory for twoliter tools #102

Conversation

bcressey commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webern commented Dec 7, 2023 • edited Loading

webern left a comment

Choose a reason for hiding this comment

webern commented Dec 19, 2023

Choose a reason for hiding this comment

webern commented Dec 22, 2023

webern commented Dec 7, 2023 •

edited

Loading