| Message ID | 20220415214245.18948-7-gvrose8192@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Remove OVS kernel driver | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 4/15/22 23:42, Greg Rose wrote: > Remove kernel based github workflows since the OVS kernel driver is > no longer supported since Release 2.17 > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > --- Hi Greg, > .github/workflows/build-and-test.yml | 35 ---------------------------- > 1 file changed, 35 deletions(-) > > diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml > index eac3504e4..cf483dc1e 100644 > --- a/.github/workflows/build-and-test.yml > +++ b/.github/workflows/build-and-test.yml > @@ -9,16 +9,9 @@ jobs: > automake libtool gcc bc libjemalloc1 libjemalloc-dev \ > libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev \ > ninja-build selinux-policy-dev > - deb_dependencies: | > - linux-headers-$(uname -r) build-essential fakeroot devscripts equivs > - AFXDP: ${{ matrix.afxdp }} > - ASAN: ${{ matrix.asan }} > CC: ${{ matrix.compiler }} > - DEB_PACKAGE: ${{ matrix.deb_package }} > DPDK: ${{ matrix.dpdk }} > DPDK_SHARED: ${{ matrix.dpdk_shared }} > - KERNEL: ${{ matrix.kernel }} > - KERNEL_LIST: ${{ matrix.kernel_list }} > LIBS: ${{ matrix.libs }} > M32: ${{ matrix.m32 }} > OPTS: ${{ matrix.opts }} > @@ -37,14 +30,6 @@ jobs: > - compiler: clang > opts: --disable-ssl > > - - compiler: gcc > - testsuite: test > - kernel: 3.16 > - - compiler: clang > - testsuite: test > - kernel: 3.16 > - asan: asan > - Maybe it's enough to just remove the 'kernel' argument here? It's useful to have address sanitizer enabled test jobs too. Arguably, when we added it we should've maybe not link it to a given kernel but, in any case, I think we shouldn't remove it completely now. What do you think of keeping it as: - compiler: clang testsuite: test asan: asan I might be wrong but it seems to me we need something similar for afxdp too. Regards, Dumitru > - compiler: gcc > testsuite: test > opts: --enable-shared > @@ -66,23 +51,6 @@ jobs: > testsuite: test > libs: -ljemalloc > > - - compiler: gcc > - kernel_list: 5.8 5.5 5.4 4.19 > - - compiler: clang > - kernel_list: 5.8 5.5 5.4 4.19 > - > - - compiler: gcc > - kernel_list: 4.14 4.9 4.4 3.16 > - - compiler: clang > - kernel_list: 4.14 4.9 4.4 3.16 > - > - - compiler: gcc > - afxdp: afxdp > - kernel: 5.3 > - - compiler: clang > - afxdp: afxdp > - kernel: 5.3 > - > - compiler: gcc > dpdk: dpdk > opts: --enable-shared > @@ -106,9 +74,6 @@ jobs: > m32: m32 > opts: --disable-ssl > > - - compiler: gcc > - deb_package: deb > - > steps: > - name: checkout > uses: actions/checkout@v2
On Fri, Apr 22, 2022 at 9:24 AM Dumitru Ceara <dceara@redhat.com> wrote: > > @@ -37,14 +30,6 @@ jobs: > > - compiler: clang > > opts: --disable-ssl > > > > - - compiler: gcc > > - testsuite: test > > - kernel: 3.16 > > - - compiler: clang > > - testsuite: test > > - kernel: 3.16 > > - asan: asan > > - > > Maybe it's enough to just remove the 'kernel' argument here? It's > useful to have address sanitizer enabled test jobs too. Arguably, when > we added it we should've maybe not link it to a given kernel but, in any > case, I think we shouldn't remove it completely now. > > What do you think of keeping it as: > > - compiler: clang > testsuite: test > asan: asan +1 > > I might be wrong but it seems to me we need something similar for afxdp too. The Ubuntu 18.04 kernel does not have AF_XDP support iirc, so either we keep on building a 5.3 kernel and compile ovs with afxdp against this kernel. Or we may switch to Ubuntu 20.04 in GHA.
On 4/22/2022 8:34 AM, David Marchand wrote: > On Fri, Apr 22, 2022 at 9:24 AM Dumitru Ceara <dceara@redhat.com> wrote: >>> @@ -37,14 +30,6 @@ jobs: >>> - compiler: clang >>> opts: --disable-ssl >>> >>> - - compiler: gcc >>> - testsuite: test >>> - kernel: 3.16 >>> - - compiler: clang >>> - testsuite: test >>> - kernel: 3.16 >>> - asan: asan >>> - >> >> Maybe it's enough to just remove the 'kernel' argument here? It's >> useful to have address sanitizer enabled test jobs too. Arguably, when >> we added it we should've maybe not link it to a given kernel but, in any >> case, I think we shouldn't remove it completely now. >> >> What do you think of keeping it as: >> >> - compiler: clang >> testsuite: test >> asan: asan > > +1 > >> >> I might be wrong but it seems to me we need something similar for afxdp too. > > The Ubuntu 18.04 kernel does not have AF_XDP support iirc, so either > we keep on building a 5.3 kernel and compile ovs with afxdp against > this kernel. > Or we may switch to Ubuntu 20.04 in GHA. > > Hi David and Dimitru, thanks for the input - I'll see if I can get that working. - Greg
On 4/26/2022 9:35 AM, Gregory Rose wrote: > > > On 4/22/2022 8:34 AM, David Marchand wrote: >> On Fri, Apr 22, 2022 at 9:24 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> @@ -37,14 +30,6 @@ jobs: >>>> - compiler: clang >>>> opts: --disable-ssl >>>> >>>> - - compiler: gcc >>>> - testsuite: test >>>> - kernel: 3.16 >>>> - - compiler: clang >>>> - testsuite: test >>>> - kernel: 3.16 >>>> - asan: asan >>>> - >>> >>> Maybe it's enough to just remove the 'kernel' argument here? It's >>> useful to have address sanitizer enabled test jobs too. Arguably, when >>> we added it we should've maybe not link it to a given kernel but, in any >>> case, I think we shouldn't remove it completely now. >>> >>> What do you think of keeping it as: >>> >>> - compiler: clang >>> testsuite: test >>> asan: asan >> >> +1 >> >>> >>> I might be wrong but it seems to me we need something similar for >>> afxdp too. >> >> The Ubuntu 18.04 kernel does not have AF_XDP support iirc, so either >> we keep on building a 5.3 kernel and compile ovs with afxdp against >> this kernel. >> Or we may switch to Ubuntu 20.04 in GHA. >> >> > > Hi David and Dimitru, > > thanks for the input - I'll see if I can get that working. > > - Greg A quick update - I was able to reinstate the address sanitizer test by dropping the kernel argument but no luck with afxdp. Perhaps we could limit afxdp support to branch 2.16 or earlier until we get a mainstream Ubuntu distro that supports it working. My biggest problem to date is fixing up the debian packaging and build to not include the dkms driver package. They're pretty tightly coupled and I'm no expert in the debian rules for packaging. However, I am making some progress - one of the main issues is that every time I make a mistake I have to reclone the repo because debian seems to scatter little traps around the code that are not cleaned out when you run 'fakeroot debian/rules clean'. Stay tuned and I'll send a new series of patches after I get this debian packaging figured out. Thanks, - Greg
On 5/3/22 21:20, Gregory Rose wrote: > > > On 4/26/2022 9:35 AM, Gregory Rose wrote: >> >> >> On 4/22/2022 8:34 AM, David Marchand wrote: >>> On Fri, Apr 22, 2022 at 9:24 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>>> @@ -37,14 +30,6 @@ jobs: >>>>> - compiler: clang >>>>> opts: --disable-ssl >>>>> >>>>> - - compiler: gcc >>>>> - testsuite: test >>>>> - kernel: 3.16 >>>>> - - compiler: clang >>>>> - testsuite: test >>>>> - kernel: 3.16 >>>>> - asan: asan >>>>> - >>>> >>>> Maybe it's enough to just remove the 'kernel' argument here? It's >>>> useful to have address sanitizer enabled test jobs too. Arguably, when >>>> we added it we should've maybe not link it to a given kernel but, in any >>>> case, I think we shouldn't remove it completely now. >>>> >>>> What do you think of keeping it as: >>>> >>>> - compiler: clang >>>> testsuite: test >>>> asan: asan >>> >>> +1 >>> >>>> >>>> I might be wrong but it seems to me we need something similar for afxdp too. >>> >>> The Ubuntu 18.04 kernel does not have AF_XDP support iirc, so either >>> we keep on building a 5.3 kernel and compile ovs with afxdp against >>> this kernel. >>> Or we may switch to Ubuntu 20.04 in GHA. >>> >>> >> >> Hi David and Dimitru, >> >> thanks for the input - I'll see if I can get that working. >> >> - Greg > > A quick update - I was able to reinstate the address sanitizer test by > dropping the kernel argument but no luck with afxdp. Perhaps we could > limit afxdp support to branch 2.16 or earlier until we get a mainstream > Ubuntu distro that supports it working. > > My biggest problem to date is fixing up the debian packaging and build > to not include the dkms driver package. They're pretty tightly coupled > and I'm no expert in the debian rules for packaging. However, I am > making some progress - one of the main issues is that every time I make > a mistake I have to reclone the repo because debian seems to scatter > little traps around the code that are not cleaned out when you run > 'fakeroot debian/rules clean'. > > Stay tuned and I'll send a new series of patches after I get this debian > packaging figured out. Thanks for working on this! Sorry, I was out for some time and just dug myself out of a pile of emails. Unfortunately, I'm not familiar with debian packaging code either, so can't really help much, but will take a look at the new version of the set once it is available. Small nit for the current series: 2.17 was released in February and the module will stay there, so the removal will be part of 2.18, not 2.17. For the afxdp, I think, we can keep the install_kernel() function, but only remove a few things from it that are not needed for afxdp, e.g. diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh index 6cd38ff3e..6ef629447 100755 --- a/.ci/linux-build.sh +++ b/.ci/linux-build.sh @@ -69,13 +69,6 @@ function install_kernel() sed -i 's/CONFIG_STACK_VALIDATION=y/CONFIG_STACK_VALIDATION=n/' .config make oldconfig - # Older kernels do not include openvswitch - if [ -d "net/openvswitch" ]; then - make net/openvswitch/ - else - make net/bridge/ - fi - if [ "$AFXDP" ]; then sudo make headers_install INSTALL_HDR_PATH=/usr pushd tools/lib/bpf/ @@ -91,9 +84,6 @@ function install_kernel() sudo sed -i '/^# define __always_inline .*/i # undef __always_inline' \ /usr/include/x86_64-linux-gnu/sys/cdefs.h || true EXTRA_OPTS="${EXTRA_OPTS} --enable-afxdp" - else - EXTRA_OPTS="${EXTRA_OPTS} --with-linux=$(pwd)" - echo "Installed kernel source in $(pwd)" fi popd } --- Maybe some other parts also can be removed from the function. This way we can keep the 'kernel: 5.3' for afxdp jobs, but remove kernels from all other jobs. 'if [ "$AFXDP" ]' check can also be moved outside of the function to the place where install_kernel() is called. Best regards, Ilya Maximets.
diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index eac3504e4..cf483dc1e 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -9,16 +9,9 @@ jobs: automake libtool gcc bc libjemalloc1 libjemalloc-dev \ libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev \ ninja-build selinux-policy-dev - deb_dependencies: | - linux-headers-$(uname -r) build-essential fakeroot devscripts equivs - AFXDP: ${{ matrix.afxdp }} - ASAN: ${{ matrix.asan }} CC: ${{ matrix.compiler }} - DEB_PACKAGE: ${{ matrix.deb_package }} DPDK: ${{ matrix.dpdk }} DPDK_SHARED: ${{ matrix.dpdk_shared }} - KERNEL: ${{ matrix.kernel }} - KERNEL_LIST: ${{ matrix.kernel_list }} LIBS: ${{ matrix.libs }} M32: ${{ matrix.m32 }} OPTS: ${{ matrix.opts }} @@ -37,14 +30,6 @@ jobs: - compiler: clang opts: --disable-ssl - - compiler: gcc - testsuite: test - kernel: 3.16 - - compiler: clang - testsuite: test - kernel: 3.16 - asan: asan - - compiler: gcc testsuite: test opts: --enable-shared @@ -66,23 +51,6 @@ jobs: testsuite: test libs: -ljemalloc - - compiler: gcc - kernel_list: 5.8 5.5 5.4 4.19 - - compiler: clang - kernel_list: 5.8 5.5 5.4 4.19 - - - compiler: gcc - kernel_list: 4.14 4.9 4.4 3.16 - - compiler: clang - kernel_list: 4.14 4.9 4.4 3.16 - - - compiler: gcc - afxdp: afxdp - kernel: 5.3 - - compiler: clang - afxdp: afxdp - kernel: 5.3 - - compiler: gcc dpdk: dpdk opts: --enable-shared @@ -106,9 +74,6 @@ jobs: m32: m32 opts: --disable-ssl - - compiler: gcc - deb_package: deb - steps: - name: checkout uses: actions/checkout@v2
Remove kernel based github workflows since the OVS kernel driver is no longer supported since Release 2.17 Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- .github/workflows/build-and-test.yml | 35 ---------------------------- 1 file changed, 35 deletions(-)