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

bpf: split complexity configurations into separate files #26925

Merged
merged 1 commit into from Jul 25, 2023

Conversation

lmb
Copy link
Contributor

@lmb lmb commented Jul 19, 2023

The complexity testsuite currently loads configuration from a single file for each BPF object and kernel version. In the file each line is a distinct configuration for that BPF object. For example:

-DFOO=1
-DBAR=1

would compile twice. Once with FOO defined, once with BAR. In practice we have about 30 to 40 defines per each line, which makes editing, diffing, etc. a nightmare.

Switch the format from one line per configration to one file per configuration. This makes it easier to diff and also to resolve merge conflicts.

The existing configurations were converted to invidiual files using the following script:

#!/bin/bash

set -euo pipefail

for dir in bpf/complexity-tests/*; do
    test -d "$dir" || continue
    pushd "$dir" || exit 2
    for f in *.txt; do
        i=0
        while read -r line; do
            test -z "$line" && continue
            i=$((i + 1))
            target="$(basename "$f" .txt)"
            mkdir -p "$target"
            echo "${line// /$'\n'}" > "$target/$i.txt"
        done < "$f"
    done
    popd || exit 2
done

@lmb lmb added sig/loader Impacts the loading of BPF programs into the kernel. release-note/misc This PR makes changes that have no direct user impact. area/modularization labels Jul 19, 2023
@lmb
Copy link
Contributor Author

lmb commented Jul 19, 2023

/ci-verifier

@lmb
Copy link
Contributor Author

lmb commented Jul 19, 2023

/ci-verifier

@lmb
Copy link
Contributor Author

lmb commented Jul 19, 2023

/test

@lmb
Copy link
Contributor Author

lmb commented Jul 19, 2023

This PR is probably best reviewed with Hide whitespace changes enabled.

@lmb lmb marked this pull request as ready for review July 19, 2023 14:40
@lmb lmb requested review from a team as code owners July 19, 2023 14:40
@lmb lmb requested review from ti-mo and christarazi July 19, 2023 14:40
@lmb
Copy link
Contributor Author

lmb commented Jul 20, 2023

/test

@lmb lmb self-assigned this Jul 20, 2023
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

LGTM!

test/verifier/verifier_test.go Outdated Show resolved Hide resolved
The complexity testsuite currently loads configuration from
a single file for each BPF object and kernel version. In the
file each line is a distinct configuration for that BPF object.
For example:

    -DFOO=1
    -DBAR=1

would compile twice. Once with FOO defined, once with BAR.
In practice we have about 30 to 40 defines per each line,
which makes editing, diffing, etc. a nightmare.

Switch the format from one line per configuration to one
file per configuration. This makes it easier to diff and
also to resolve merge conflicts.

The existing configurations were converted to invidiual
files using the following script:

    #!/bin/bash

    set -euo pipefail

    for dir in bpf/complexity-tests/*; do
        test -d "$dir" || continue
        pushd "$dir" || exit 2
        for f in *.txt; do
            i=0
            while read -r line; do
                test -z "$line" && continue
                i=$((i + 1))
                target="$(basename "$f" .txt)"
                mkdir -p "$target"
                echo "${line// /$'\n'}" > "$target/$i.txt"
            done < "$f"
        done
        popd || exit 2
    done

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb
Copy link
Contributor Author

lmb commented Jul 24, 2023

/test

@lmb
Copy link
Contributor Author

lmb commented Jul 24, 2023

Travis failure is due to #26617

@lmb lmb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 24, 2023
@lmb
Copy link
Contributor Author

lmb commented Jul 24, 2023

Timo is on PTO, so no review from him. Dylan and Robin reviewed instead.

@youngnick youngnick removed the request for review from ti-mo July 25, 2023 07:08
@youngnick youngnick merged commit 02a1045 into cilium:main Jul 25, 2023
64 of 65 checks passed
@jibi jibi added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Aug 25, 2023
@jibi jibi mentioned this pull request Aug 25, 2023
3 tasks
@jibi jibi added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants