Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
On Thu, Jun 17, 2021 at 12:31:45PM +0200, Tim Düsterhus wrote: > Willy, > > On 6/13/21 3:02 PM, Tim Duesterhus wrote: > > Using 'sudo' required quite a few workarounds in various places. Setting an > > explicit 'ulimit -n' removes the requirement for 'sudo', resulting in a > > cleaner > > workflow configuration. > > You might've missed this one. Ilya already acked it and I tested it here: > > https://github.com/TimWolla/haproxy/actions/runs/933195103 Now merged, thanks guys. Willy
Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
Hi Tim, On Thu, Jun 17, 2021 at 12:31:45PM +0200, Tim Düsterhus wrote: > Willy, > > On 6/13/21 3:02 PM, Tim Duesterhus wrote: > > Using 'sudo' required quite a few workarounds in various places. Setting an > > explicit 'ulimit -n' removes the requirement for 'sudo', resulting in a > > cleaner > > workflow configuration. > > You might've missed this one. Ilya already acked it and I tested it here: > > https://github.com/TimWolla/haproxy/actions/runs/933195103 Yep thank you. In fact it was not totally missed, I knew I had a few patches from you to merge, but was focused on finishing the recent fixes for a release :-) And my todo-list for this afternoon starts with merging your series ;-) Willy
Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
Willy, On 6/13/21 3:02 PM, Tim Duesterhus wrote: Using 'sudo' required quite a few workarounds in various places. Setting an explicit 'ulimit -n' removes the requirement for 'sudo', resulting in a cleaner workflow configuration. You might've missed this one. Ilya already acked it and I tested it here: https://github.com/TimWolla/haproxy/actions/runs/933195103 Best regards Tim Düsterhus
Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
Anyway, ACK from me. ulimit is less evil than sudo On Sun, Jun 13, 2021, 4:25 PM Tim Düsterhus wrote: > Ilya, > > On 6/13/21 3:18 PM, Илья Шипицин wrote: > > It was in my to do list as well. I recall I ran tests without increasing > > limits. > > > > Which test requires 5 open files? Maybe the one currently disabled? > > > > Also, it does not like a good pattern to open so many files. If we really > > use as many files, shouldn't we revisit that area? > > The issue is that HAProxy attempts to increase the soft ulimit to the > hard ulimit during startup. This fails on macOS which has a soft limit > of ~10k and a hard limit of ~500k, but does not allow to actually > increase the soft limit without being root. > > That's why I'm updating the ulimit manually before starting HAProxy. > This will update both the soft as well as the hard limit, resulting in > HAProxy not needing to do anything, thus succeeding on macOS. > > I surely could've used an even smaller number, but 5k is something that > worked just fine on my first attempt, so that one it is. In fact the > limit I set is even lower than the default of macOS. > > Best regards > Tim Düsterhus >
Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
Ilya, On 6/13/21 3:18 PM, Илья Шипицин wrote: It was in my to do list as well. I recall I ran tests without increasing limits. Which test requires 5 open files? Maybe the one currently disabled? Also, it does not like a good pattern to open so many files. If we really use as many files, shouldn't we revisit that area? The issue is that HAProxy attempts to increase the soft ulimit to the hard ulimit during startup. This fails on macOS which has a soft limit of ~10k and a hard limit of ~500k, but does not allow to actually increase the soft limit without being root. That's why I'm updating the ulimit manually before starting HAProxy. This will update both the soft as well as the hard limit, resulting in HAProxy not needing to do anything, thus succeeding on macOS. I surely could've used an even smaller number, but 5k is something that worked just fine on my first attempt, so that one it is. In fact the limit I set is even lower than the default of macOS. Best regards Tim Düsterhus
Re: [PATCH] CI: Replace the requirement for 'sudo' with a call to 'ulimit -n'
It was in my to do list as well. I recall I ran tests without increasing
limits.
Which test requires 5 open files? Maybe the one currently disabled?
Also, it does not like a good pattern to open so many files. If we really
use as many files, shouldn't we revisit that area?
On Sun, Jun 13, 2021, 4:02 PM Tim Duesterhus wrote:
> Using 'sudo' required quite a few workarounds in various places. Setting an
> explicit 'ulimit -n' removes the requirement for 'sudo', resulting in a
> cleaner
> workflow configuration.
> ---
> .github/workflows/vtest.yml | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/.github/workflows/vtest.yml b/.github/workflows/vtest.yml
> index 786713374..1dc216eeb 100644
> --- a/.github/workflows/vtest.yml
> +++ b/.github/workflows/vtest.yml
> @@ -99,13 +99,14 @@ jobs:
>run: echo "::add-matcher::.github/vtest.json"
> - name: Run VTest for HAProxy ${{ steps.show-version.outputs.version
> }}
>id: vtest
> - # sudo is required, because macOS fails due to an open files limit.
> - run: sudo make reg-tests VTEST_PROGRAM=../vtest/vtest
> REGTESTS_TYPES=default,bug,devel
> + run: |
> +# This is required for macOS which does not actually allow to
> increase
> +# the '-n' soft limit to the hard limit, thus failing to run.
> +ulimit -n 5000
> +make reg-tests VTEST_PROGRAM=../vtest/vtest
> REGTESTS_TYPES=default,bug,devel
> - name: Show results
>if: ${{ failure() }}
> - # The chmod / sudo is necessary due to the `sudo` while running the
> tests.
>run: |
> -sudo chmod a+rX ${TMPDIR}/haregtests-*/
> for folder in ${TMPDIR}/haregtests-*/vtc.*; do
>printf "::group::"
>cat $folder/INFO
> @@ -115,6 +116,6 @@ jobs:
> shopt -s nullglob
> for asan in asan.log*; do
>echo "::group::$asan"
> - sudo cat $asan
> + cat $asan
>echo "::endgroup::"
> done
> --
> 2.32.0
>
>
