kernel-patches/vmtest

Change condition on `kernel-patches/bpf` check to detect if Linux sources should be downloaded

Closed this issue · 7 comments

Context: I'm reusing some of the CI infra for a Linux fork on my own GitHub account.

Following commit fa07d13, the description of the GitHub workflow has the following snippet to allow the CI to run on the vmtest repository.

      - name: Get bpf-next source
        if: ${{ github.repository != 'kernel-patches/bpf' }}
        uses: actions/checkout@v2
        with:
          repository: kernel-patches/bpf
          ref: 'bpf-next'
          path: 'linux'
      - name: Move linux source in place
        if: ${{ github.repository != 'kernel-patches/bpf' }}
        run: rsync --exclude .git -av linux/. . && rm -rf linux

This makes the Action download kernel sources before compiling and running the checks, if github.repository != 'kernel-patches/bpf'.

It is rather easy to copy all files from vmtest to another Linux repository and to run the same CI checks from another GitHub account by creating a dedicated PR (e.g. qmonnet/linux#4, the branch was manually created, without the Patchwork parts of the CI), except that the above check will overwrite the Linux sources (and the modifications to test). It's not hard to remove this check before copying the file and running the action, but it makes the operation less straightforward.

Would it be possible to adjust this check? One solution could be to have if: ${{ github.repository == 'kernel-patches/vmtest' }}, although the same issue would arise again if someone tried to run the checks on e.g. a fork of vmtest. Alternatively, would it be possible to check whether the repository seems to contain the Linux sources? E.g. by looking at a few known files maybe?

I'm trying to understand what the actual problem is? You have some local (not committed) changes to the test that get overwritten by that rsync command? Or what exactly is the issue?

So the CI is presently made to run on two repos:

  1. On kernel-patches/bpf, by copying all CI files as an additional commit. In that case, the CI does not download Linux sources: it runs on the code committed in the PR.
  2. On kernel-paches/vmtest, to check that the CI still works as expected on modifications of the CI infra. For that case, we don't copy the CI files, but instead we pull the Linux sources to run on it the new CI files modified in the PR.

The distinction is made with that check on github.repository != 'kernel-patches/bpf'. If the repo is not kernel-patches/bpf, download the sources.

Now if I want to run the CI on my repo, I want to run them on my code, that is, on the commits on my PR. But given that my repo is obviously not kernel-patches/bpf, I hit case 2., so the Linux sources will be downloaded when the GitHub Action starts. The sources are downloaded and extracted at the root of the repo, and the result is that they overwrite all the Linux sources that were in the repository before (including my own commits). Kernel builds and selftests are run on the downloaded sources and not on the code I want to test.

What I want is case 1.: I have the Linux code (with my own commits on top) in my repo already, I copy the CI files, and I don't want to overwrite the content of my repo with upstream sources (or to wait until the useless download is finished, for that matter). So far I'm just removing the check in test.yml after I copy it locally, as a workaround. If it's really to change the test.yml, I can live with that, to be honest. I was just thinking it would feel more reusable/straightforward without that condition on the repo name.

I hope this makes things clearer?

@qmonnet what you are doing is very similar to what kernel-patches-daemon bot is doing on kernel-tests/bpf actually. These two steps was added by me to help tests PR in vmtests , I think the best fix is actually make a copy of tests.yml and add a top level "if: ${{ github.repository == "kernel-tests/vmtests" }} that has these two extra steps , and delete them on the main tests.yml .

I'm not sure I understand. So we would have two yml files in this repo: the current one, modified to never download the Linux sources, and a new one, which would download them but would be run only on kernel-patches/vmtest? Isn't it the same thing as reversing the check in the current file to if: ${{ github.repository == 'kernel-patches/vmtest' }}? Or am I confused?

Reversing the check one way or another to download the Linux sources just for kernel-patches/vmtest would work for my use case, but it would no longer allow developers to run the CI on PRs on forks of vmtest for example (although it would still work if they create PRs against kernel-patches/vmtest. I don't know if this could be an issue.

(Thanks a lot to both of you for looking into this!)

@qmonnet because the PR opened on kernel-patches/bpf is a actually a merge of kernel-tests/vmtest and linux source code / patches. this means test.yml currently serves two purpose: acting as CI on PRs on kernel-test/bpf and acting as CI for kernel-tests/vmtests (through those optional steps), so I think it make sense to separate this two purpose into two files.

Ok, presented that way, it sounds like a correct solution.

If two yml files are not a burden maintenance-wise, then sure, let's do that.

But regardless of that (now that I understand the issue), I do think that those conditions should be reverted. kernel-patches/vmtest is an exception here, not the kernel-patches/bpf. Think about when we'll have other kernel trees (e.g., net/net-next), they would fall into the (default) category of having Linux sources and needing CI files applied on top, not the other way. So I think inverting the condition as a first step makes total sense. @qmonnet, feel free to send the PR with the inverted condition.