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

Add multiarch support #2301

Merged
2 commits merged into from
Sep 23, 2021
Merged

Add multiarch support #2301

2 commits merged into from
Sep 23, 2021

Conversation

toochevere
Copy link
Contributor

Bump version of balena-multibuild to the one that supports multiarch
Remove previous hack to avoid sending platform information to multibuild

Change-type: minor
Signed-off-by: Paul Jonathan [email protected]
See: #1508
Resolves: #1508

@toochevere toochevere added the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Jul 28, 2021
@toochevere toochevere requested a review from pdcastro July 28, 2021 19:15
@toochevere toochevere self-assigned this Jul 28, 2021
Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Looks like some test cases are failing, perhaps build.spec.ts or deploy.spec.ts or push.spec.ts:

https://ci.balena-dev.com/builds/1285659

Click here to expand the build error
  balena build

[debug] original argv0="node" argv=[C:\Program Files\nodejs\node.exe,C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\bin\balena,build,C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\tests\test-data\projects\no-docker-compose\basic,--deviceType,nuc,--arch,amd64,-g] length=9
[debug] new argv=[C:\Program Files\nodejs\node.exe,C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\bin\balena,build,C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\tests\test-data\projects\no-docker-compose\basic,--deviceType,nuc,--arch,amd64,-g] length=9
[Debug]   Parsing input...
[Debug]   Loading project...
[Debug]   Resolving project...
[Debug]   Creating project...
[Info]    No "docker-compose.yml" file found at "C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\tests\test-data\projects\no-docker-compose\basic"
[Info]    Creating default composition with source: "C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\tests\test-data\projects\no-docker-compose\basic"
[Info]    Building for amd64/nuc
[Build]   Building services...
[Build]   main          Preparing...
[Info]    Docker Desktop detected (daemon architecture: "x86_64")
[Info]      Docker itself will determine and enable architecture emulation if required,
[Info]      without balena-cli intervention and regardless of the --emulated option.
[Info]    Converting line endings CRLF -> LF for file: C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\tests\test-data\projects\no-docker-compose\basic\src\windows-crlf.sh
[Debug]   Found build tasks:
[Debug]       main: build [.]
[Debug]   Resolving services with [nuc|amd64]
[Debug]   Found project types:
[Debug]       main: Standard Dockerfile
[Debug]   Prepared tasks; building...
NockMock: Unexpected HTTP request: GET http//localhost:80/version
[Build]   Built 1 service in 0 seconds
[Error]   Build failed.

Nock: No match for request {
  "method": "GET",
  "url": "http://localhost/version",
  "headers": {}
}

BuildProcessError: Nock: No match for request {
  "method": "GET",
  "url": "http://localhost/version",
  "headers": {}
}
    at performSingleBuild (C:\var\lib\concourse\worker\volumes\live\81a3f164-2cef-4f9c-4c81-42c6156b20bd\volume\node_modules\resin-multibuild\lib\index.ts:359:9)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

For further help or support, visit:
https://www.balena.io/docs/reference/balena-cli/#support-faq-and-troubleshooting


NockMock: Unexpected HTTP request: POST https://sentry.io:443/api/149239/store/
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test:source: `cross-env BALENA_CLI_TEST_TYPE=source mocha`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test:source script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\balena\AppData\Roaming\npm-cache\_logs\2021-07-28T19_22_18_407Z-debug.log
npm ERR! Test failed.  See above for more details.

The test cases watch or intercept HTTP requests made by the CLI using nock, failing on unexpected requests -- a story of love and hate. :-) They may be failing because of the new HTTP requests introduced by balena-multibuild. Then, to make your life harder, the error may be further masked by Sentry:

NockMock: Unexpected HTTP request: POST https://sentry.io:443/api/149239/store/

Sentry.io is a service we use to capture errors produced by the CLI. You may be able to sign in to Sentry.io with GitHub, and could search for more details (traceback) for that error there. But I think the easiest / best option is to run the CLI tests on your laptop, disabling Sentry with an env var so that it does not mask the original error:

export BALENARC_NO_SENTRY=1
cd balena-cli
npm test

To run only a subset of tests, you can edit the .mocharc.js file, and/or use it.only(...) strategically. I think there are some auth-related tests that must run for authentication to succeed in other tests though, which is a bit of a pain when attempting to run only a subset of tests. You're welcome to create additional PRs to make improvements to CLI testing and to the CONTRIBUTING.md file when you come across things that could be described as "I wish this was better documented and easier to do" :-)

@toochevere toochevere force-pushed the add-multiarch-handling branch 2 times, most recently from 66c29f6 to 17e5b8d Compare September 16, 2021 21:00
@toochevere toochevere removed the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Sep 16, 2021
@toochevere toochevere marked this pull request as ready for review September 16, 2021 21:06
@toochevere toochevere changed the title WIP - Add multiarch support Add multiarch support Sep 16, 2021
Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

The last time I wrote a PR to add multiarch support to the CLI (last year), Marios tested it during PR review against the balena-node-hello-world repo and got the following error, which derailed everything:

# deleted all containers and images, to be sure
$ docker rm $(docker ps -aq)
$ docker rmi -f $(docker image ls -q)

$ ./bin/balena build ~/balena/balena-io-examples/balena-node-hello-world -A armv7hf -d raspberrypi3 --nocache
[Info]    No "docker-compose.yml" file found at "/Users/paulo/balena/balena-io-examples/balena-node-hello-world"
[Info]    Creating default composition with source: "/Users/paulo/balena/balena-io-examples/balena-node-hello-world"
[Build]   Building services...
[Build]   main Preparing...
[Info]    Building for armv7hf/raspberrypi3
[Info]    Docker Desktop detected (daemon architecture: "x86_64")
[Info]      Docker itself will determine and enable architecture emulation if required,
[Info]      without balena-cli intervention and regardless of the --emulated option.
[Build]   main Step 1/7 : FROM balenalib/raspberrypi3-node:14-buster-run
[Build]   main  ---> e074e38a89ba
[Build]   main Step 2/7 : WORKDIR /usr/src/app
[Build]   Built 1 service in 0:24
[Error]   Build failed.
failed to get destination image "sha256:a420a59549a9771c38e751ef29dfa6d213bf7e108b85ceb3592c6b07c9dbfe57": 
image with reference sha256:a420a59549a9771c38e751ef29dfa6d213bf7e108b85ceb3592c6b07c9dbfe57 
was found but does not match the specified platform: wanted linux/arm/v7, actual: linux/amd64

It seems that balena-node-hello-world remains true to form, as I just got the same error when testing this PR. The error was discussed towards the beginning of The Mother of All Threads. It may be a case where the new multibuild logic should not be forwarding the platform option to Docker, and actually is. Or not. I have not dug to find out.

It is also possible that I did something wrong at my end, while testing. Let me know if you can reproduce the error with balena-node-hello-world.

@toochevere
Copy link
Contributor Author

toochevere commented Sep 17, 2021

It is also possible that I did something wrong at my end, while testing. Let me know if you can reproduce the error with balena-node-hello-world.

This was a bug in multibuild. Fixed. It now selects the correct image on my machine, but unfortunately I have a bin_fmt problem that stops the build from going all the way. Please check again, it should work on the sample app with a warning that platform support has been disabled.

The base image does not in fact have a distribution manifest, so it doesn't have platform support. I am checking with @nghiant2710 in FD.

@toochevere toochevere marked this pull request as draft September 17, 2021 00:31
@toochevere toochevere force-pushed the add-multiarch-handling branch from 17e5b8d to d869e28 Compare September 17, 2021 01:36
@toochevere toochevere marked this pull request as ready for review September 17, 2021 01:41
@toochevere toochevere requested a review from pdcastro September 17, 2021 13:17
Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

I may have been the one who suggested adding the following warning, but now that I see how the CLI is producing it with balenalib base images, I think it feels like a step backwards in terms of UX, adding rather than removing friction to end users:

$ ./bin/balena build ~/balena/balena-io-examples/balena-node-hello-world -A armv7hf -d raspberrypi3
...
[Info]    Building for armv7hf/raspberrypi3
[Info]    Docker Desktop detected (daemon architecture: "x86_64")
[Info]      Docker itself will determine and enable architecture emulation if required,
[Info]      without balena-cli intervention and regardless of the --emulated option.
Warning: Dockerfile contains a images do not support platform selection.
-balenalib/raspberrypi3-node:14-buster-run

To avoid a possible build error, the CLI has disabled platform selection. As a result, the architecture of the machine where the Docker Engine is running will be used to select the platform when pulling upstream images which may result in "exec format error" at runtime. To avoid this warning, update or replace the images that do not support platform selection.
[Build]   main Step 1/7 : FROM balenalib/raspberrypi3-node:14-buster-run
[Build]   main  ---> e074e38a89ba

Before this PR, no warning was printed and the CLI "just did the right thing" (which actually meant "doing nothing" in relation to platform selection). From users' point of view, this is a case of a single-arch balenalib base image named "raspberrypi3-node", so to warn that "the CLI has disabled platform selection" will sound strange to them. Picture how Alex would friendly trash this warning in a "Alex uses balena" kind of call, if he was trying to use balena build. :-) What would be helpful in this context is #673, but it is outside the scope of this PR.

I have put together a draft multibuild PR: https://github.com/balena-io-modules/balena-multibuild/pull/96/files

That draft PR also introduces a logger to replace "console.log", which would make the warnings look better. The CLI output shown above looks visually bad, missing the [Build] or other prefixes that a logger inserts.

package.json Outdated
@@ -268,7 +268,7 @@
"resin-cli-visuals": "^1.8.0",
"resin-compose-parse": "^2.1.3",
"resin-doodles": "^0.1.1",
"resin-multibuild": "^4.12.1",
"resin-multibuild": "4.12.2-logger-for-multiarch-warnings-277a26c5e7ea11c2c4bba007263f4c168232682d",
Copy link
Contributor

@pdcastro pdcastro Sep 17, 2021

Choose a reason for hiding this comment

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

I've taken the liberty of adding a commit to this PR and editing the commit message of the previous commit.

This temporary resin-multibuild version refers to a draft multibuild PR: balena-io-modules/balena-multibuild#96

I have tested:

  • balena build with single and multi-container projects (including balena-node-hello-world) :-)
  • Single-stage and multi-stage Dockerfiles
  • Using a few FROM --platform=linux/arm/v7 lines
  • The latest Docker Desktop on macOS (Docker Desktop v4.0.1, Docker Engine v20.10.8, with binfmt_misc support)
  • balenaEngine on balenaOS (no binfmt_misc support)

Things I have not tested:

  • balena deploy
  • balena push

Copy link
Contributor Author

@toochevere toochevere Sep 20, 2021

Choose a reason for hiding this comment

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

thanks for your help @pdcastro ... 🙏🏻 this is definitely a team effort. One thing that I still need to test (just realized) is a third party registry.

Sanity checks

  • balena build with single and multi-container projects (including balena-node-hello-world) :-)
  • Single-stage and multi-stage Dockerfiles
  • Using a few FROM --platform=linux/arm/v7 lines
  • The latest Docker Desktop on macOS (Docker Desktop v4.0.1, Docker Engine v20.10.8, with binfmt_misc support)
  • balenaEngine on balenaOS (no binfmt_misc support)
  • balen deploy
  • balena push
  • third party registries

Putting this back in draft until we cover those.

Copy link
Contributor Author

@toochevere toochevere Sep 23, 2021

Choose a reason for hiding this comment

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

So, I don't know if I should be shocked, scared, or both. 😱 But the latest changes are passing all manual tests.

  • Build with multi arch v2 manfiest Pi and Nuc
  • Build with single arch v2 manifest Pi and Nuc
  • Build with single arch V1 manifest Pi and Nuc
  • Build with third party registry

@pdcastro pdcastro force-pushed the add-multiarch-handling branch from 7af4c7f to 1892067 Compare September 18, 2021 02:07
@toochevere toochevere marked this pull request as draft September 20, 2021 10:38
@toochevere
Copy link
Contributor Author

I may have been the one who suggested adding the following warning, but now that I see how the CLI is producing it with balenalib base images, I think it feels like a step backwards in terms of UX, adding rather than removing friction to end users:

$ ./bin/balena build ~/balena/balena-io-examples/balena-node-hello-world -A armv7hf -d raspberrypi3
...
[Info]    Building for armv7hf/raspberrypi3
[Info]    Docker Desktop detected (daemon architecture: "x86_64")
[Info]      Docker itself will determine and enable architecture emulation if required,
[Info]      without balena-cli intervention and regardless of the --emulated option.
Warning: Dockerfile contains a images do not support platform selection.
-balenalib/raspberrypi3-node:14-buster-run

To avoid a possible build error, the CLI has disabled platform selection. As a result, the architecture of the machine where the Docker Engine is running will be used to select the platform when pulling upstream images which may result in "exec format error" at runtime. To avoid this warning, update or replace the images that do not support platform selection.
[Build]   main Step 1/7 : FROM balenalib/raspberrypi3-node:14-buster-run
[Build]   main  ---> e074e38a89ba

Before this PR, no warning was printed and the CLI "just did the right thing" (which actually meant "doing nothing" in relation to platform selection). From users' point of view, this is a case of a single-arch balenalib base image named "raspberrypi3-node", so to warn that "the CLI has disabled platform selection" will sound strange to them. Picture how Alex would friendly trash this warning in a "Alex uses balena" kind of call, if he was trying to use balena build. :-) What would be helpful in this context is #673, but it is outside the scope of this PR.

I have put together a draft multibuild PR: https://github.com/balena-io-modules/balena-multibuild/pull/96/files

That draft PR also introduces a logger to replace "console.log", which would make the warnings look better. The CLI output shown above looks visually bad, missing the [Build] or other prefixes that a logger inserts.

@pdcastro I am still waiting to hear (haven't gone through my FD entirely yet) about what the story is regarding those balenalib images. I'll update when I find out.

@toochevere toochevere marked this pull request as ready for review September 23, 2021 16:31
toochevere and others added 2 commits September 23, 2021 16:37
Bump version of balena-multibuild to the one that supports multiarch
Remove previous hack to avoid sending platform information to multibuild

Change-type: minor
Signed-off-by: Paul Jonathan <[email protected]>
See: #1508
@toochevere toochevere force-pushed the add-multiarch-handling branch from 1892067 to 8bb211e Compare September 23, 2021 16:44
@toochevere toochevere requested a review from pdcastro September 23, 2021 16:44
@pdcastro
Copy link
Contributor

node-cli build was stuck at event duplicated-asset balena-cli-v12.49.0-windows-x64-installer.exe - https://ci.balena-dev.com/builds/1405302
When this happens, I learned it requires manually deleting the asset from the draft release in GitHub, which I just did. I think this is an issue with resinci-deploy that needs investigation at some point. For now, I'll trigger a retest.

@pdcastro
Copy link
Contributor

@balena-ci retest

package.json Outdated Show resolved Hide resolved
@@ -18527,6 +18539,11 @@
}
}
},
"vscode-languageserver-types": {
Copy link
Contributor

Choose a reason for hiding this comment

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

vscode-languageserver-types as a production dependency? Seems to be a dependency of the newly added dockerfile-ast module. Only 300KB though so never mind, we can't fix the world... :-)

Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Hurray! Multiarch support across balena build/deploy/push at last. Well done @toochevere and everybody else involved!

@@ -268,7 +268,7 @@
"resin-cli-visuals": "^1.8.0",
"resin-compose-parse": "^2.1.3",
"resin-doodles": "^0.1.1",
"resin-multibuild": "^4.12.1",
"resin-multibuild": "4.12.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"resin-multibuild": "4.12.2",
"resin-multibuild": "^4.12.2",

In this case it does not make a difference though.

@ghost ghost merged commit b42af74 into master Sep 23, 2021
@ghost ghost deleted the add-multiarch-handling branch September 23, 2021 20:54
This pull request was closed.
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.

balena build / deploy --build selects base image of wrong architecture (multiarch image manifest)
2 participants