Skip to content
This repository has been archived by the owner on Oct 24, 2020. It is now read-only.

builder: check if patch was already applied before applying patch #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dongsupark
Copy link

Check if patch was already applied before applying patch. This will allow builder to work even if the kernel already includes the patch.

@alban
Copy link
Contributor

alban commented Jul 28, 2017

So you rely on the fact that set -e at the beginning of the script does not apply inside the function?

LGTM if CircleCI is green.

Check if patch was already applied before applying patch.
This will allow builder to work even if the kernel already includes
the patch.
@dongsupark dongsupark force-pushed the dongsu/check-patch-before-apply branch from 18ef9fa to fe7467d Compare July 28, 2017 15:36
@dongsupark
Copy link
Author

Ah, I missed something. Force-pushed with set +e and patch -f.

We need to have config for kernel 4.12 to be able to build kernel
on CircleCI.
@dongsupark
Copy link
Author

Added a kernel config 4.12 to make CI work again. Now the CI passed.

@alban
Copy link
Contributor

alban commented Jul 31, 2017

The PR itself is good, however I see that CircleCI (build 106) builds the following versions:

11825624 /tmp/circle-artifacts.jOtleV1/stage1-kvm-1.26.0-linux-4.12.4.aci
10565012 /tmp/circle-artifacts.jOtleV1/stage1-kvm-1.26.0-linux-4.1.38.aci
10938884 /tmp/circle-artifacts.jOtleV1/stage1-kvm-1.26.0-linux-4.4.45.aci
11063794 /tmp/circle-artifacts.jOtleV1/stage1-kvm-1.26.0-linux-4.8.13.aci
11285604 /tmp/circle-artifacts.jOtleV1/stage1-kvm-1.26.0-linux-4.9.9.aci

And the previous build generated the following artifacts:

$ curl -sSL 'https://circleci.com/api/v1.1/project/github/kinvolk/stage1-builder/latest/artifacts?branch=master&filter=successful' \
>   | jq -r .[].path \
>   | sed -e 's/.*\/\([^/]*\)\.aci$/\1/'
stage1-kvm-1.26.0-linux-4.1.38
stage1-kvm-1.26.0-linux-4.10.6
stage1-kvm-1.26.0-linux-4.11.5
stage1-kvm-1.26.0-linux-4.4.45
stage1-kvm-1.26.0-linux-4.8.13
stage1-kvm-1.26.0-linux-4.9.6
stage1-kvm-1.26.0-linux-4.9.9

So kernel 4.9.6 was previously built but not anymore. This is a problem because tcptracer-bpf and gobpf rely on it. See the following links:

So, merging this PR would break tcptracer-bpf and gobpf. @schu used to trigger parametric CircleCI builds manually to avoid this issue. But I think it is wrong that tcptracer-bpf and gobpf always use the last version of the stage1-builder builds. We should use something like this:

rkt image fetch --insecure-options=image kinvolk.io/aci/rkt/stage1-kvm:1.23.0,kernelversion=4.9.6,stage1builderversion=1.0

(specifying 3 versions instead of 2: the rkt version, the kernel version, and a version of the stage1builder, so downstream projects would not break on every stage1-builder PR)

And instead of picking the latest CircleCI artifacts, tcptracer-bpf and gobpf should stick to a specific CircleCI build.

@dongsupark
Copy link
Author

@alban Thanks for the explanation. I'm fine with not merging it until we had a new versioning policy.

builder Outdated
@@ -80,20 +80,36 @@ test "$(find "${kernel_source_dir}" -maxdepth 0 -type d -empty 2>/dev/null)" &&
# configure kernel
test -f "${kernel_source_dir}/.config" || sed -e "s/-rkt-v1/${kernel_version_suffix}/g" "${kernel_config}" >"${kernel_source_dir}/.config"

function patch_kernel() {
filename=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: we try to follow https://google.github.io/styleguide/shell.xml, i.e.

  • use local variables in functions
  • prefer [[ over [

Also, shellcheck complains about a couple of things (e.g. missing " quotes; not all from your change).

I'll try to get back to issue with build images / assets later this week.

@alban alban mentioned this pull request Aug 16, 2017
@dongsupark
Copy link
Author

@schu Pushed a new commit to fix shell style issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants