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

TCB - docker-in-docker build #202

Closed
wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 22, 2024

The existing github actions does not function for a docker-in-docker (dind) configuration. Due to the build script using directories with root:root ownership a rootless self-hosted runner can not complete this workflow.

The configuration below adds -wd working_directory argument to the tcb-env-setup.sh script to allow absolute path to be passed directly to the docker run torizon/torizoncore-builder. Due to a dind setup, the /workdir needs to be mounted with an absolute path. Github actions provides variables containing the absolute path for the runner directory, however, this is a well documented bug in the runner since 2022:

A workaround for this is to add a build step creating a file with this abs-path to be referenced laterecho "${{ github.workspace }}" >> abs-path . Any ENV var set in actions will be replaced at run-time by the runner replacing the ABS path with a relative sandboxed path for the runner. This is why a file is written, not an ENV.

The scripts/tasks.ps1 now has an additional ENV check to detect if it's in a Actions workflow, and set the abs-path accordingly. I did not handle errors for this abs-path file not existing.

❗ This is only tested for tcb-build stage, no subsequent steps - tcb-platform-push-ostree, or platform-update-fleet

REF: email
Signed-off-by: d.kelly@coligo.ai

echo " -wd: select working directory for docker mount to /workdir"
echo " Pass the directory explicitly for docker run torizon/torizoncore-builder"
echo " to mount as /workdir."
echo " It must be an absolute directory. If this is not set, \$(pwd) with be used"
Copy link
Collaborator

Choose a reason for hiding this comment

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

$(pwd) with be used

I think this was a typo with should be will, right?

Copy link
Author

Choose a reason for hiding this comment

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

yes ..will be used

sh get-docker.sh

wget -nv https://github.com/PowerShell/PowerShell/releases/download/v7.4.2/powershell_7.4.2-1.deb_amd64.deb
dpkg -i powershell_7.4.2-1.deb_amd64.deb
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, maybe this is needed on your end because you host your own runner. But, on Github Actions the default image already has these dependencies.

Would be nice to first identify whether the dependencies are already installed before installing it.

Copy link
Author

Choose a reason for hiding this comment

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

this is needed because I'm running the build inside a container, on the shared and self-hosted runners. Both will spin up a docker Ubuntu:23.04 container, mount the local filesystem, and run the build commands. It's good for reproducibility. If you used GitLab, running dind is standard.

@@ -6,13 +6,18 @@ jobs:
build-deploy:
runs-on: ubuntu-latest
name: Build & Deploy TorizonCore
container: ubuntu:23.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed because of the self-hosted runner?

Copy link
Author

Choose a reason for hiding this comment

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

@microhobby microhobby assigned ghost May 30, 2024
@microhobby
Copy link
Collaborator

@dk-coligo sorry for the delay, we are in the process of a new release, so, I will back to this after the release. We need to add it to our CI/CD integration tests to make sure that this will not break it the already existent experience.

@microhobby microhobby added this to the 2.6.0 milestone Jun 5, 2024
@ghost
Copy link
Author

ghost commented Jun 12, 2024

@microhobby we've had to make many other changes to the default scripts. For example, the platform-push stage for the tasks.json. In the developer documentation, there is a dependency on tcb unpack then tcb platform push. When I add this to the depends-on: parameter of the platform-push task in tasks.json, only the unpack command runs and then exits without running the platform-push. So, unpack needs explicit running in the build.yaml stage. I'll comment on the file inline separately so you can find it :)

@andreriesco
Copy link
Collaborator

LGTM. I will rebase it here and include it in the next 2.6 release. Thanks for the contribution

microhobby added a commit that referenced this pull request Aug 30, 2024
When running the tcb in a docker-in-docker environment, the workdir
mount point is not found, because the github actions runner sandbox
the runner by re-writing the absolute path to a different location.
So, first we need to get the absolute path of the workspace and then
store it in a file called abs-path, so we can use get the path from
the file and use it as the workdir mount point.

Reported-by: @dk-coligo
Related-to: #202
Signed-off-by: Matheus Castello <matheus.castello@toradex.com>
@microhobby
Copy link
Collaborator

@dk-coligo I reimplemented the stuff reported here, so instead of adding a -wd argument we are setting a HOST_GITHUB_WORKSPACE env variable to change the default behavior. This way it make easier on our side to maintain it for all the templates not only for TCB one.
Take a look on 5a6849f and if this does not fit your needs feel free to reopen this ticket.

@microhobby microhobby closed this Aug 30, 2024
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.

3 participants