Factor out incremental build prep into an action #143

theihor · 2024-09-12T18:58:47Z

Partially resolves #142

@chantra I've been testing this change in a simplified workflow on my fork.

Any suggestions how to verify kernel-patches/bpf CI would work with the change? I guess we could just push and see, but maybe there is a less disruptive way.

chantra · 2024-09-13T06:19:03Z

Thanks @theihor

I have not fully reviewed the whole PR yet, but from quick pass, it seems fine so far.

Any suggestions how to verify kernel-patches/bpf CI would work with the change? I guess we could just push and see, but maybe there is a less disruptive way.

Yes, you should create a PR against kernel-patches/vmtest, apply the changes would expect to do when migrating to using the new action, but until this is landed, you will need to use an action path pointing to your repo/branch, e.g

theihor/libbpf-ci/prepare-incremental-build@prepare-incremental-build-action
instead of libbpf-ci/{action}@main.

chantra

I think there is potentially 1 flaw that we were lucky to not hit so far. e.g if we were to run the action outside of the github repo.

For the case of kernel-patches/bpf... the workspace just happens to be a linux repo. For other projects, we do download the sources in the step above and move it in place, so we end up being lucky and having the workspace being a linux repo too.

I believe, if we had the action working properly, we should be able to skip the "move linux source in place" step. I think this is out of scope for this task, but somethign to keep in mind for later to make the action self-contained.

Finally, lets take the opportunity when working on actions to write a README.md in the action directory to document how to use it. Similarly to how actions in the marketplace do: https://github.com/actions/checkout/blob/main/README.md for instance.

chantra · 2024-09-13T06:29:17Z

prepare-incremental-build/action.yml

+  repo-root:
+    description: "Root of the kernel repository"
+    required: true
+    default: ${{ github.workspace }}


in my experience, this does not work. We have it in a few places, but I don't think it works.

It may be worth testing separately through a simple action.

Based on https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#context-availability , it is not mentioned that any contexts are available in the "inputs" part of an action. (It would work for a reusable workflow though).

replying to myself... on the other hand, there is things like https://github.com/actions/checkout/blob/6d193bf28034eafb982f37bd894289fe649468fc/action.yml#L6C5-L6C38 which suggests it should work, so mayke I did something wrong when testing.

Yes, it's not clear from the docs. I can double check, although in this case I think we can just remove the default and require this input to be specified.

requiring would be fine too and probably preferred. We can do this here because it is a brand new action, it is more tricky when the action is already used.

chantra · 2024-09-13T06:35:19Z

prepare-incremental-build/action.yml

+      run: ${GITHUB_ACTION_PATH}/get-commit-metadata.sh ${{ inputs.base-branch }}
+
+    - name: Pull recent KBUILD_OUTPUT contents
+      uses: actions/cache@v4


I am pretty sure we should set the working-directory here too.
I believe we are lucky it works because the caller sets repo-root to github.workspace, which happens to be the default working directory.

My understading is that this particular step invoking actinons/cache@v4 does not have to be executed from the repository directory. The path to store the result of cache lookup (if it's a hit) is passed via path parameter:

- name: Pull recent KBUILD_OUTPUT contents uses: actions/cache@v4 with: path: ${{ inputs.kbuild-output }} # <-- THIS key: ...

And we pass it through as an action parameter, although my impression is it never changes for our workflows.

And then the next step (that runs prepare-incremental-builds.sh) expects to be executed in the repository root, but also expects KBUILD_OUTPUT env variable to point to the cached build.

- name: Prepare incremental build shell: bash env: KBUILD_OUTPUT: ${{ inputs.kbuild-output }} working-directory: ${{ inputs.repo-root }} # and so here we do have to set path to the repository

I guess it is confusing that we do not set working-directory in cache step, but do set it for other steps, but the action should work either way.

I am not convinced path work as you are explaining it. Based on https://github.com/actions/cache?tab=readme-ov-file#inputs

my understanding is that it will extract the "kbuild-output" directory from the cache archive.

side note... we are not caching as part of libbpf/ci (e.g https://github.com/libbpf/ci/actions/caches shows no cache, we may want to also exercise this in the future.).

I am not convinced path work as you are explaining it. Based on https://github.com/actions/cache?tab=readme-ov-file#inputs

my understanding is that it will extract the "kbuild-output" directory from the cache archive.

Maybe I explained it badly, but I think we both understand it correctly.

The cache action is designed to store things like gradle caches (docs), so I am pretty sure its path input is used both for uploading to the "cloud" after job has finished, and then restoring to the same path later, if lookup key matches.

Although actually, if the path is relative, then working directory might be important. So to be on the safe side, I'll set working-directory to the kernel repo root.

The cache action is designed to store things like gradle caches (docs), so I am pretty sure its path input is used both for uploading to the "cloud" after job has finished, and then restoring to the same path later, if lookup key matches.

agreed

Although actually, if the path is relative, then working directory might be important. So to be on the safe side, I'll set working-directory to the kernel repo root.

I think this would make the composite action work as expected if the kernel-root does not happen to be the github.workspace.

On the other hand, kbuild-output could be anywhere on the filesystem and we should be able to build by providing the absolute path. If we remove the need for kbuild-output to be in kernel-root, we could leverage the cache for workflows outside of kernel-patches/bpf .But that should probably go in a separate PR.

It turns out that it is not possible to specify working-directory in a step that uses an action (as opposed to running a script). If you try, job fails on execution. So, in our case, passing an absolute path to actions/cache is the only good option really.

theihor · 2024-09-13T16:24:29Z

@chantra

For the case of kernel-patches/bpf... the workspace just happens to be a linux repo. For other projects, we do download the sources in the step above and move it in place, so we end up being lucky and having the workspace being a linux repo too.

I believe, if we had the action working properly, we should be able to skip the "move linux source in place" step. I think this is out of scope for this task, but somethign to keep in mind for later to make the action self-contained.

In the simplified workflow where I was testing this action, I deliberately checked out kernel-patches/bpf into a non-workspace directory, and then passed a path to that directory as action input (see here).

So, assuming I haven't missed anything, the action doesn't care how the linux source tree directory was created: via checkout or via "download linux source" steps. As long as the correct path to the git repo is passed, the action will try finding the cached build output.

Finally, lets take the opportunity when working on actions to write a README.md in the action directory to document how to use it. Similarly to how actions in the marketplace do: https://github.com/actions/checkout/blob/main/README.md for instance.

Will do.

prepare-incremental-build/get-commit-metadata.sh

chantra · 2024-09-13T22:37:28Z

In the simplified workflow where I was testing this action, I deliberately checked out kernel-patches/bpf into a non-workspace directory, and then passed a path to that directory as action input (see here).

So, assuming I haven't missed anything, the action doesn't care how the linux source tree directory was created: via checkout or via "download linux source" steps. As long as the correct path to the git repo is passed, the action will try finding the cached build output.

yeah, I think we are missing something but it silently fails.
In your case, no cache was available: https://github.com/theihor/libbpf-ci/actions/runs/10836721377/job/30071091347#step:4:63
What I am afraid is that at best we would download the cache but in an directory which is not accessible when we do the build.
It is probably easier to test/validate with a actions/cache basic workflow.

theihor · 2024-09-14T00:42:55Z

yeah, I think we are missing something but it silently fails.
In your case, no cache was available: https://github.com/theihor/libbpf-ci/actions/runs/10836721377/job/30071091347#step:4:63
What I am afraid is that at best we would download the cache but in an directory which is not accessible when we do the build.
It is probably easier to test/validate with a actions/cache basic workflow.

In that workflow there was nothing to cache actually... I wrote another one, and there you can see the cache was

created by https://github.com/theihor/libbpf-ci/actions/runs/10857913112/job/30135143759
and then restored in subsequent run: https://github.com/theihor/libbpf-ci/actions/runs/10857937779/job/30135210069

Btw, do we want workflows like that (created specifically to test a custom action) in libbpf/ci? I am not sure.

chantra · 2024-09-14T01:13:47Z

Btw, do we want workflows like that (created specifically to test a custom action) in libbpf/ci? I am not sure.

Maybe not using the key used by the kernel. But given the hard time we have to validate it works as expected, I think it could be useful to have some integration workflows to avoid regression in the future.

Make `kbuild-output` a required input. Pass an absolute path to `kbuild-output` from kernel-build reusable workflow. Introduce optional `cache-key-prefix` input. Remove branch name parsing logic from `get-commit-metadata.sh`. Accept the branch that is a target for caching as argument.

theihor · 2024-09-17T23:55:31Z

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf.

I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66

Do you know if there is a reason for having two copies?

There are two distinct KBUILD_OUTPUTs across libbpf/ci: - $KBUILD_OUTPUT used by actions/cache (in prepare-incremental-build) - $REPO_ROOT/kbuild-output used by ./run-vmtest So far this difference wasn't important, because $KBUILD_OUTPUT always pointed to $(pwd)/kbuild-output. And $(pwd) happened to be a root of the linux source tree, also equal to github.workspace However we don't want prepare-incremental-build action to put any requirements on $KBUILD_OUTPUT path to allow for a convenient action API. With this, the difference becomes important, because only selected files are put from $KBUILD_OUTPUT into kernel-build output artifacts, while entire $KBUILD_OUTPUT has to be cached by prepare-incremental-build.

chantra · 2024-09-19T21:34:29Z

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf.

I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66

Do you know if there is a reason for having two copies?

No, I don't think so. The artifact is downloadable from GH (when going to the summary of the run, there is a link to the artifacts). The content of vmlinux-x86_64-gcc is https://gist.github.com/chantra/0acb0ec04a58db3adea2debd47e73fee

I think it could be moved in place.
I know that we keep the "Makefile"s so later we can run make -s image_name to discover where the vmlinux file is. The rest, I am not too sure. Even in the selftests/bpf directory we may not need to keep everything that is compiled. At least not for the "artifact" that we re-use during the test run. We do need to keep it for when we update the cache though.

chantra

LGTM. Before this lands, do you mind testing on the kernel-patches/vmtest repo by using this action, just to make sure we did not missed anything.

It looks like the tar logic as a whole needs to be re-assessed, but this is a separate story/change that can be followed-up later.

chantra · 2024-09-19T21:39:39Z

.github/scripts/tar-artifact.sh

-    "tools/testing/selftests/bpf/" 
-    "tools/include/" 
+    "scripts/"
+    "tools/testing/selftests/bpf/"


maybe this one is not needed given that we add the "copied" selftest/bpf on line 97.

Yeah, I thought about it, but didn't want to risk breaking something, before I'm sure this is unnecessary.

I agree with your suggestion to re-assess build artifacts in a separate change, as this one is quite big already.

chantra · 2024-09-19T21:42:23Z

.github/scripts/tar-artifact.sh

+  mkdir -p "$(dirname "${local_kbuild_output}/${file}")"
+  cp -a "${stashed_kbuild_output}/${file}" "${local_kbuild_output}/${file}"
+done
+
 additional_file_list=()
 if [ $archive_make_helpers -ne 0 ]; then
  # Package up a bunch of additional infrastructure to support running
  # 'make kernelrelease' and bpf tool checks later on.
  mapfile -t additional_file_list < <(find . -iname Makefile)
  additional_file_list+=(


I suppose the end of the if block is here.

Do you mean that only the makefiles should be included conditionally? I simply left the original logic.

chantra · 2024-09-19T21:54:31Z

.github/scripts/tar-artifact.sh

+    --exclude '*.cmd'                  \
+    --exclude '*.d'                    \
+    --exclude '*.h'                    \
+    --exclude '*.output'               \


weird, at least in the paste, there were files ending with .d and .o: https://gist.github.com/chantra/0acb0ec04a58db3adea2debd47e73fee#file-gistfile0-txt-L3600

There may be something wrong with the pattern matching?

I'll have to double check this, but IIRC --exclude applies to the files after it is specified.

So tar -cf - foo --exclude="*.x" bar will filter out *.x only from bar.

In our case it means that the filters don't apply to additional_file_list. Whether this was intentional, I don't know. Probably not.

theihor · 2024-09-20T15:44:32Z

@chantra I noticed that artifacts produced by tar-artifact.sh contain two copies of selftests/bpf: tools/testing/selftests/bpf and selftests/bpf.
I found that one is copied into another here: https://github.com/libbpf/ci/blob/main/build-selftests/build_selftests.sh#L63-L66
Do you know if there is a reason for having two copies?

No, I don't think so. The artifact is downloadable from GH (when going to the summary of the run, there is a link to the artifacts). The content of vmlinux-x86_64-gcc is https://gist.github.com/chantra/0acb0ec04a58db3adea2debd47e73fee

I think it could be moved in place. I know that we keep the "Makefile"s so later we can run make -s image_name to discover where the vmlinux file is. The rest, I am not too sure. Even in the selftests/bpf directory we may not need to keep everything that is compiled. At least not for the "artifact" that we re-use during the test run. We do need to keep it for when we update the cache though.

I believe on kernel-patches/bpf CI only one of those two selftests/bpf copies is present. For example build job artifacts from this bpf-ci run contain only this inside:

$ tree -L 2
.
├── kbuild-output
│   ├── arch
│   ├── include
│   └── vmlinux
└── selftests
    └── bpf

And the flag controlling what's included in the tarball depends on the repository name.

So the question is why additional files are needed on kernel-patches/vmtest, but not on kernel-patches/bpf. I'll try to figure that out. If it's only for make image_name it seems silly, as we should be able to pass that name around as an input or env variable.

LGTM. Before this lands, do you mind testing on the kernel-patches/vmtest repo by using this action, just to make sure we did not missed anything.

I already did: kernel-patches/vmtest#287

I tagged you there, but wasn't able to add you as a reviewer.

chantra · 2024-09-23T21:31:44Z

So the question is why additional files are needed on kernel-patches/vmtest, but not on kernel-patches/bpf. I'll try to figure that out. If it's only for make image_name it seems silly, as we should be able to pass that name around as an input or env variable.

make image_name only needs the Makefiles, and I don't think it needs anything related to selftests. So I think this is probably unrelated and just ended up there as an accident. As much as this PR is concerned, I think we should leave it like this, and follow-up in a separate PR.

I already did: kernel-patches/vmtest#287

I tagged you there, but wasn't able to add you as a reviewer.

Ha great. Thanks. If this works, then that's a high signal that everything should be fine :D

chantra · 2024-09-23T21:32:47Z

Feel free to merge this whenever you want, we may want to keep an eye on both kernel-patches/bpf and libbpf/libbpf after it landed just to be safe.

theihor · 2024-09-23T21:58:03Z

Feel free to merge this whenever you want, we may want to keep an eye on both kernel-patches/bpf and libbpf/libbpf after it landed just to be safe.

Ok. Thank you for the review.

I'd like BPF CI to become stable before merging this.

Factor out incremental build prep into an action

eb423f1

chantra closed this Sep 13, 2024

chantra reopened this Sep 13, 2024

chantra reviewed Sep 13, 2024

View reviewed changes

prepare-incremental-build/get-commit-metadata.sh Outdated Show resolved Hide resolved

theihor marked this pull request as draft September 14, 2024 00:43

theihor added 3 commits September 16, 2024 18:01

prepare-incremental-build: add testing workflow and readme

79e134c

kernel-build: remove _base suffix from default value of base branch

ac65463

theihor marked this pull request as ready for review September 17, 2024 01:15

theihor mentioned this pull request Sep 17, 2024

Use libbpf/ci/prepare-incremental-build action kernel-patches/vmtest#287

Merged

theihor added 2 commits September 17, 2024 09:24

Fix KBUILD_OUTPUT value in kernel-test workflow

e3e093f

Pass kbuild-output to the run-vmtest action

d949503

theihor marked this pull request as draft September 17, 2024 17:40

tar-artifacts.sh: don't rely on kbuild-output to be in kernel repo

33aca10

theihor force-pushed the prepare-incremental-build-action branch from 545e6fe to c646399 Compare September 18, 2024 17:02

theihor force-pushed the prepare-incremental-build-action branch from c646399 to db66d59 Compare September 18, 2024 17:27

run-vmtest: Fix path to test_bpftool_sinctypes.py

1ea87ab

theihor marked this pull request as ready for review September 18, 2024 19:27

kernel-build: Add ARCHIVE_MAKE_HELPERS flag based on repo name

370d242

theihor force-pushed the prepare-incremental-build-action branch from a8bcdf6 to 370d242 Compare September 18, 2024 20:49

chantra approved these changes Sep 19, 2024

View reviewed changes

theihor merged commit 939bc7d into libbpf:main Sep 26, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out incremental build prep into an action #143

Factor out incremental build prep into an action #143

theihor commented Sep 12, 2024 •

edited

Loading

chantra commented Sep 13, 2024

chantra left a comment

chantra Sep 13, 2024

chantra Sep 13, 2024

theihor Sep 13, 2024

chantra Sep 13, 2024

theihor Sep 17, 2024

chantra Sep 13, 2024

theihor Sep 13, 2024 •

edited

Loading

chantra Sep 13, 2024

theihor Sep 14, 2024

chantra Sep 14, 2024

theihor Sep 16, 2024

theihor commented Sep 13, 2024 •

edited

Loading

chantra commented Sep 13, 2024

theihor commented Sep 14, 2024

chantra commented Sep 14, 2024

theihor commented Sep 17, 2024 •

edited

Loading

chantra commented Sep 19, 2024

chantra left a comment

chantra Sep 19, 2024

theihor Sep 20, 2024

chantra Sep 19, 2024

theihor Sep 20, 2024

chantra Sep 19, 2024

theihor Sep 20, 2024

theihor commented Sep 20, 2024

chantra commented Sep 23, 2024

chantra commented Sep 23, 2024

theihor commented Sep 23, 2024

Factor out incremental build prep into an action #143

Factor out incremental build prep into an action #143

Conversation

theihor commented Sep 12, 2024 • edited Loading

chantra commented Sep 13, 2024

chantra left a 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

Choose a reason for hiding this comment

theihor Sep 13, 2024 • edited Loading

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

theihor commented Sep 13, 2024 • edited Loading

chantra commented Sep 13, 2024

theihor commented Sep 14, 2024

chantra commented Sep 14, 2024

theihor commented Sep 17, 2024 • edited Loading

chantra commented Sep 19, 2024

chantra left a 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

Choose a reason for hiding this comment

theihor commented Sep 20, 2024

chantra commented Sep 23, 2024

chantra commented Sep 23, 2024

theihor commented Sep 23, 2024

theihor commented Sep 12, 2024 •

edited

Loading

theihor Sep 13, 2024 •

edited

Loading

theihor commented Sep 13, 2024 •

edited

Loading

theihor commented Sep 17, 2024 •

edited

Loading