Re: Re: CI caching improvement

[William Lallemand]

On Mon, Mar 21, 2022 at 04:19:38PM +0500, Илья Шипицин wrote:
> 
> I think we can adjust build-ssl.sh script to download tagged quictls (and
> cache it in the way we do cache openssl itself)
> Tags · quictls/openssl (github.com)
> 
> 

Unfortunately that won't work, those tags are inherited from the OpenSSL
repository and does not contain any QUIC code. They didn't make any tag
for quictls, probably because they don't want to do an active
maintenance.

However it looks like they are maintaining a branch with their quic
patches for each openssl subversions. And those branches are not
modified too much once they pushed it.

Maybe we could just take the latest commit of the "openssl-3.0.2+quic"
branch for now, and update it from time to time.

The other solution I considered for VTest was to cache one build
per day. Maybe we could do that for quictls, because the project is not
that active and most of the time the people developing on QUIC test
everything on their computer. That could be the best compromise between
low maintenance and automatic update of the quictls code.
The only requirement would be to display the quictls commit ID
somewehere in the log to be certain of the version we are linking with.


-- 
William Lallemand

Re: CI caching improvement

[Tim Düsterhus]

William,

On 3/18/22 11:31, William Lallemand wrote:

It looks like it is available as well on our repositories, I just test
it and it works correctly.

Honestly I really don't like the dependency to another repository with a
format specific to github.

I agree that a cleaner integration with github with their specific tools
is nice, but I don't want us to be locked with github, we are still
using cirrus, travis, sometimes gitlab, and also running some of the
scripts by hand.

We also try to avoid the dependencies to other projects and its much
simplier to have few shell scripts and a CI configuration in the
repository. And typescript is not a language we would want to depend on
if we need to debug it for example.


Okay, that's fair.


Giving that github is offering the job restart feature, we could skip
the VTest caching, since it's a little bit ugly. Only the quictls cache
need to be fixed.


Perfect, I agree here. QUICTLS caching is useful and VTest caching is 
obsolete with the single-job restart.


Best regards
Tim Düsterhus

Re: CI caching improvement

[Илья Шипицин]

пт, 18 мар. 2022 г. в 15:32, William Lallemand :

> On Wed, Mar 16, 2022 at 09:31:56AM +0100, Tim Düsterhus wrote:
> > Willy,
> >
> > On 3/8/22 20:43, Tim Düsterhus wrote:
> > >> Yes my point was about VTest. However you made me think about a very
> good
> > >> reason for caching haproxy builds as well :-)  Very commonly, some
> VTest
> > >> randomly fails. Timing etc are involved. And at the moment, it's
> impossible
> > >> to restart the tests without rebuilding everything. And it happens to
> me to
> > >> click "restart all jobs" sometimes up to 2-3 times in a row in order
> to end
> > >
> > > I've looked up that roadmap entry I was thinking about: A "restart this
> > > job" button apparently is planned for Q1 2022.
> > >
> > > see https://github.com/github/roadmap/issues/271 "any individual job"
> > >
> > > Caching the HAProxy binary really is something I strongly advice
> against
> > > based on my experience with GitHub Actions and CI in general.
> > >
> > > I think the restart of the individual job sufficiently solves the issue
> > > of flaky builds (until they are fixed properly).
> > >
> >
> > In one of my repositories I noticed that this button is now there. One
> > can now re-run individual jobs and also all failed jobs. See screenshots
> > attached.
> >
>
> Hello Tim,
>
> It looks like it is available as well on our repositories, I just test
> it and it works correctly.
>
> Honestly I really don't like the dependency to another repository with a
> format specific to github.
>
> I agree that a cleaner integration with github with their specific tools
> is nice, but I don't want us to be locked with github, we are still
> using cirrus, travis, sometimes gitlab, and also running some of the
> scripts by hand.
>
> We also try to avoid the dependencies to other projects and its much
> simplier to have few shell scripts and a CI configuration in the
> repository. And typescript is not a language we would want to depend on
> if we need to debug it for example.
>
> Giving that github is offering the job restart feature, we could skip
> the VTest caching, since it's a little bit ugly. Only the quictls cache
> need to be fixed.
>

I think we can adjust build-ssl.sh script to download tagged quictls (and
cache it in the way we do cache openssl itself)
Tags · quictls/openssl (github.com)



>
> Regards,
>
> --
> William Lallemand
>

Re: CI caching improvement

[William Lallemand]

On Wed, Mar 16, 2022 at 09:31:56AM +0100, Tim Düsterhus wrote:
> Willy,
> 
> On 3/8/22 20:43, Tim Düsterhus wrote:
> >> Yes my point was about VTest. However you made me think about a very good
> >> reason for caching haproxy builds as well :-)  Very commonly, some VTest
> >> randomly fails. Timing etc are involved. And at the moment, it's impossible
> >> to restart the tests without rebuilding everything. And it happens to me to
> >> click "restart all jobs" sometimes up to 2-3 times in a row in order to end
> > 
> > I've looked up that roadmap entry I was thinking about: A "restart this
> > job" button apparently is planned for Q1 2022.
> > 
> > see https://github.com/github/roadmap/issues/271 "any individual job"
> > 
> > Caching the HAProxy binary really is something I strongly advice against
> > based on my experience with GitHub Actions and CI in general.
> > 
> > I think the restart of the individual job sufficiently solves the issue
> > of flaky builds (until they are fixed properly).
> > 
> 
> In one of my repositories I noticed that this button is now there. One 
> can now re-run individual jobs and also all failed jobs. See screenshots 
> attached.
> 

Hello Tim,

It looks like it is available as well on our repositories, I just test
it and it works correctly.

Honestly I really don't like the dependency to another repository with a
format specific to github.

I agree that a cleaner integration with github with their specific tools
is nice, but I don't want us to be locked with github, we are still
using cirrus, travis, sometimes gitlab, and also running some of the
scripts by hand. 

We also try to avoid the dependencies to other projects and its much
simplier to have few shell scripts and a CI configuration in the
repository. And typescript is not a language we would want to depend on
if we need to debug it for example.

Giving that github is offering the job restart feature, we could skip
the VTest caching, since it's a little bit ugly. Only the quictls cache
need to be fixed.

Regards,

-- 
William Lallemand

Re: CI caching improvement

[Tim Düsterhus]

Willy,

On 3/8/22 20:43, Tim Düsterhus wrote:

Yes my point was about VTest. However you made me think about a very good
reason for caching haproxy builds as well :-)  Very commonly, some VTest
randomly fails. Timing etc are involved. And at the moment, it's impossible
to restart the tests without rebuilding everything. And it happens to me to
click "restart all jobs" sometimes up to 2-3 times in a row in order to end


I've looked up that roadmap entry I was thinking about: A "restart this
job" button apparently is planned for Q1 2022.

see https://github.com/github/roadmap/issues/271 "any individual job"

Caching the HAProxy binary really is something I strongly advice against
based on my experience with GitHub Actions and CI in general.

I think the restart of the individual job sufficiently solves the issue
of flaky builds (until they are fixed properly).



In one of my repositories I noticed that this button is now there. One 
can now re-run individual jobs and also all failed jobs. See screenshots 
attached.


Best regards
Tim Düsterhus

Re: CI caching improvement

[Илья Шипицин]

script/build-vtest.sh was/is reused for cirrus,travis

On Wed, Mar 9, 2022, 12:05 AM Tim Düsterhus  wrote:

> William,
>
> On 3/8/22 16:06, William Lallemand wrote:
> > Let me know if we can improve the attached patch, otherwise I'll merge
> > it.
> >
>
> Let me make a competing proposal that:
>
> 1. Keeps the complexity out of the GitHub workflow configuration in
> haproxy/haproxy.
> 2. Still allows VTest caching.
>
> For my https://github.com/TimWolla/haproxy-auth-request repository I
> have created a reusable GitHub Action to perform the HAProxy
> installation similar to 'actions/checkout':
>
> https://github.com/TimWolla/action-install-haproxy/
>
> I just spent a bit of time to fork that action and to make it VTest
> specific:
>
> https://github.com/TimWolla/action-install-vtest/
>
> The action receives the VTest branch or commit as the input and will
> handle all the heavy lifting of downloading, compiling and caching VTest.
>
> The necessary changes to HAProxy then look like this:
>
>
> https://github.com/TimWolla/haproxy/commit/78af831402e354f22d67682be0f323dec9c26a52
>
> This basically replaces the use of 'scripts/build-vtest.sh' by
> 'timwolla/action-install-vtest@main', so the configuration in the
> haproxy/haproxy repository is not getting any more complicated, as all
> the heavy lifting is done in the action which can be independently
> tested and maintained.
>
> If this proposal sounds good to you, then I'd like to suggest the
> following:
>
> 1. Willy creates a new haproxy/action-install-vtest repository in the
> haproxy organization.
> 2. Willy creates a new GitHub team with direct push access to that
> repository.
> 3. Willy adds me to that team, so that I can maintain that repository
> going forward (e.g. to handle the Dependabot pull requests that keep the
> JavaScript dependencies up to date).
>
> If that repository is properly set up, I'll send a patch to switch over
> haproxy/haproxy to make use of that action.
>
> Best regards
> Tim Düsterhus
>

Re: [EXTERNAL] Re: CI caching improvement

[Tim Düsterhus]

William,

On 3/8/22 21:30, Tim Düsterhus wrote:

- The action is also easily reusable by other projects. For testing my


Adding to that: It's also easily reusable by the other workflows. We 
currently have the separate musl.yml workflow that does this:


https://github.com/haproxy/haproxy/blob/5ce1299c643543c9b17b4124b299feb3caf63d9e/.github/workflows/musl.yml#L19-L20

Your patch proposal doesn't adjust that, but with the dedicated action 
it could automatically benefit from caching or any other improvements we 
make to the VTest installation without needing to touch all the .yml 
files separately.


Here's an example run with an updated commit:

https://github.com/TimWolla/haproxy/runs/5470838975?check_suite_focus=true

https://github.com/TimWolla/haproxy/commit/cefe211b774f0393d4f78268a23036f32b74ee4b

Best regards
Tim Düsterhus

Re: [EXTERNAL] Re: CI caching improvement

[Tim Düsterhus]

William,

On 3/8/22 20:54, William Lallemand wrote:

Honestly I'm confused, it is overcomplicated in my opinion :(

I don't really see the benefits in creating a whole new repository
instead of the few lines in the yaml file.


I believe that having a non-trivial amount of logic in a YAML file will 
ultimately result in a hard to understand configuration file.


As an example: YAML doesn't support any kind of syntax highlighting or 
autocompletion.



We are talking about doing a new project for just the equivalent of a 5
lines shell script... which really don't need to be tested and
maintained outside of the project.


With your suggested diff you needed to change 4 different locations 
within the vtest.yml, growing the file from 152 to 168 lines (+10%). And 
none of those lines are specific to HAProxy itself!


By separating out the VTest installation logic all that's needed in 
vtest.yml is the following:


- name: Install VTest
  uses: haproxy/action-install-vtest@main
  with:
branch: master
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

I think it's pretty obvious what that would do: It installs VTest and 
when that step is finished you can simply use vtest.


How this action does this is left to the action, it just promises you to 
do the right thing and then the HAProxy repository just needs to worry 
about the HAProxy specific parts.


It's really the same like any other programming library. Just like 
HAProxy uses libraries (e.g. mjson or SLZ) to perform some task, the CI 
can do the same.



I feel like I'm missing something with my simple implementation, we are
already downloading all the SSL libraries, should we stop doing it this
way? What could be the problems with this?


I'd like to simplify the installation of the various SSL libs as well, 
but I don't have a good proposal for that.



It seems like you want to do this in a strict github way, which is
probably convenient for a lot of usecase, but it just look really more
complicated that my first proposal.



It sure comes with a bit of initial set-up work, but I'm volunteering to 
do that. I'm also volunteering the maintenance of that. As I've said: 
This is nothing I just came up with, I'm using that for 
haproxy-auth-request since July 2020 and I'm pretty happy with that.


The benefits of this dedicated repository are:
- Fixes and improvements VTest installation no longer require a testing 
commit to the HAProxy repository. They can be developed in the dedicated 
repository with a specialized CI for VTest installation.

- It uses a proper programming language instead of embedding bash in a YAML.
- The action is also easily reusable by other projects. For testing my 
haproxy-auth-request repository I could remove the VTest installation 
logic from action-install-haproxy and simply use the existing action. 
This might also come in handy to test 
https://github.com/haproxytech/haproxy-lua-oauth and other official 
extensions.


Best regards
Tim Düsterhus

Re: [EXTERNAL] Re: CI caching improvement

[William Lallemand]

On Tue, Mar 08, 2022 at 08:05:31PM +0100, Tim Düsterhus wrote:
> 
> Let me make a competing proposal that:
> 
> 1. Keeps the complexity out of the GitHub workflow configuration in 
> haproxy/haproxy.
> 2. Still allows VTest caching.
> 
> For my https://github.com/TimWolla/haproxy-auth-request repository I 
> have created a reusable GitHub Action to perform the HAProxy 
> installation similar to 'actions/checkout':
> 
> https://github.com/TimWolla/action-install-haproxy/
> 
> I just spent a bit of time to fork that action and to make it VTest 
> specific:
> 
> https://github.com/TimWolla/action-install-vtest/
> 
> The action receives the VTest branch or commit as the input and will 
> handle all the heavy lifting of downloading, compiling and caching VTest.
> 
> The necessary changes to HAProxy then look like this:
> 
> https://github.com/TimWolla/haproxy/commit/78af831402e354f22d67682be0f323dec9c26a52
> 
> This basically replaces the use of 'scripts/build-vtest.sh' by 
> 'timwolla/action-install-vtest@main', so the configuration in the 
> haproxy/haproxy repository is not getting any more complicated, as all 
> the heavy lifting is done in the action which can be independently 
> tested and maintained.
> 
> If this proposal sounds good to you, then I'd like to suggest the following:
> 
> 1. Willy creates a new haproxy/action-install-vtest repository in the 
> haproxy organization.
> 2. Willy creates a new GitHub team with direct push access to that 
> repository.
> 3. Willy adds me to that team, so that I can maintain that repository 
> going forward (e.g. to handle the Dependabot pull requests that keep the 
> JavaScript dependencies up to date).
> 
> If that repository is properly set up, I'll send a patch to switch over 
> haproxy/haproxy to make use of that action.
> 
> Best regards
> Tim Düsterhus

Tim,

Honestly I'm confused, it is overcomplicated in my opinion :(

I don't really see the benefits in creating a whole new repository
instead of the few lines in the yaml file.

We are talking about doing a new project for just the equivalent of a 5
lines shell script... which really don't need to be tested and
maintained outside of the project.

I feel like I'm missing something with my simple implementation, we are
already downloading all the SSL libraries, should we stop doing it this
way? What could be the problems with this?

It seems like you want to do this in a strict github way, which is
probably convenient for a lot of usecase, but it just look really more
complicated that my first proposal.

-- 
William Lallemand

Re: CI caching improvement

[Tim Düsterhus]

Willy,

On 3/8/22 20:11, Willy Tarreau wrote:

Yes my point was about VTest. However you made me think about a very good
reason for caching haproxy builds as well :-)  Very commonly, some VTest
randomly fails. Timing etc are involved. And at the moment, it's impossible
to restart the tests without rebuilding everything. And it happens to me to
click "restart all jobs" sometimes up to 2-3 times in a row in order to end


I've looked up that roadmap entry I was thinking about: A "restart this 
job" button apparently is planned for Q1 2022.


see https://github.com/github/roadmap/issues/271 "any individual job"

Caching the HAProxy binary really is something I strongly advice against 
based on my experience with GitHub Actions and CI in general.


I think the restart of the individual job sufficiently solves the issue 
of flaky builds (until they are fixed properly).



If those are gone,
then I expect the vast majority of the commits to be green so that it only
catches mistakes in strange configurations that one cannot easily test
locally.


This is already the case for the vast majority of them. I think that if we
eliminate the random failures on vtest, we're around 95-98% of successful
pushes. The remaining ones are caused by occasional download failures to
install dependencies and variables referenced outside an obscure ifdef
combination that only strikes on one platform.


Yeah, exactly that's what I wanted to say: If the CI is expected to be 
green, then there should not be a need to manually check it for *every* 
push and thus it doesn't matter as much of it takes 65 seconds or 80 
seconds.



Of course if you think that the 8 seconds will improve your workflow, then
by all means: Commit it. From my POV in this case the cost is larger than
the benefit. Caching is one of the hard things in computer science :-)


[…]
extremely useful. Translating this back to haproxy builds, for me it
means: "if at any point there is any doubt that something that might
affect the build result might have changed, better rebuild". And that's


Yes, that's why I explicitly recommend against caching the HAProxy 
binary. Cache invalidation really *is* hard for that without increasing 
complexity of the vtest.yml which already is longer than I'm comfortable 
with. At least with matrix.py it's manageable.



Now rest assured that I won't roll over the floor crying for this, but
I think that this is heading in the right direction. And with more CI
time available, we can even hope to test more combinations and squash
more bugs before they reach users. That's another aspect to think about.
I'm really happy to see that the duration between versions .0 and .1
increases from version to version, despite the fact that we add a lot
more code and more complex one. For now the CI scales well with the
code, I'm interested in continuing to help it scale even better.



40 minutes ago I've already sent a proposal that should make both you as 
the developers and me as one of the CI experts happy :-) My concerns 
were primarily with regard the number of additional steps in William's 
proposal, not the caching of VTest per se.


Best regards
Tim Düsterhus

Re: CI caching improvement

[Willy Tarreau]

On Tue, Mar 08, 2022 at 04:40:40PM +0100, Tim Düsterhus wrote:
> > > Please don't. You always want to run a clean build, otherwise you're going
> > > to get super-hard-to-debug failures if some object file is accidentally
> > > taken from the cache instead of a clean build.
> > 
> > What risk are you having in mind for example ? I'm personally thinking
> > that vtest is sufficiently self-contained to represent almost zero risk.
> > We could possibly even pre-populate it and decide to rebuild if it's not
> > able to dump its help page.
> 
> This was a reply to "cache the build of HAProxy", so unrelated to VTest. As
> the HAProxy build is what we want to test, it's important to always perform
> a clean build.

Yes my point was about VTest. However you made me think about a very good
reason for caching haproxy builds as well :-)  Very commonly, some VTest
randomly fails. Timing etc are involved. And at the moment, it's impossible
to restart the tests without rebuilding everything. And it happens to me to
click "restart all jobs" sometimes up to 2-3 times in a row in order to end
up on a valid one. It really takes ages. Just for this it would be really
useful to cache the haproxy builds so that re-running the jobs only runs
vtest.

The way I'm seeing an efficient process would be this:
  1) prepare OS images
  2) retrieve cached dependencies if any, and check for their freshness,
 otherwise build them
  3) retrieve cached haproxy if any and check for its freshness, otherwise
 build it (note: every single push will cause a rebuild). If we can
 include the branch name and/or tag name it's even better.
  4) retrieve cached VTest if any, and check for its freshness,
 otherwise build it
  5) run VTest

This way, a push to a branch will cause a rebuild, but restarting the exact
same test will not. This would finally allow us to double-check for unstable
VTest reports. Right now we re-discover them when stumbling upon an old tab
in the browser that was left there all day.

> Looking at this: https://github.com/haproxy/haproxy/actions/runs/1952139455,
> the fastest build is the gcc no features build with 1:33. Even if we
> subtract the 8 seconds for VTest, then that's more than I'd be willing to
> synchronously wait.

I understand, but that starts to count when you have to re-do all this
just for a single failed vtest timing out on an overloaded VM.

> The slowest are the QUICTLS builds with 6:21, because
> QUICTLS is not currently cached.

Which is an excellent reason for also caching QUICTLS as is already done
for libressl or openssl3, I don't remember (maybe both).

> FWIW: Even 6:21 is super fast compared to other projects. I've recently
> contributed to PHP and CI results appear after 20 minutes or so.

The fact that others spend even longer than us is not an excuse for
us to be as bad :-)

You & Ilya were among those insisting for a CI to improve our quality,
and it works! It works so well that developers are now impatient to see
the result to be sure they didn't break $RANDOM_OS. This morning I merged
David's build fix for kfreebsd and started to work on the backports. When
I finished, it looked like the build was OK but apparently I was still on
a previous result, I don't know, or maybe Cirrus is much more delayed, I
don't know, but in the end I thought everything was good and I pushed the
backport to 2.5. Only later I noticed I was receiving more "failed" mails
than usual, looked at it and there it was, the patch broke freebsd after
I had already pushed the backport to two branches. It's not dramatic, but
this method of working increases the context-switch rate and I often find
myself doing more mistakes or at least less controls when switching all
the time between branches and tests, because once you see an error, you
switch back to the original branch, fix the issue, push again, switch back
to the other branch you were, get notified about another issue (still
unrelated to what you're doing) etc. It's mentally difficult (at least
for me). Being able to shorten the time between a push and a result will
leave me less concentration time on another subject and destroy a bit less
what remains of my brain.

Nothing there is critical at all, the quality is already improved, but it
seems to me that for a little cost we can significantly improve some of
the remaining rough edges.

> In any case I believe that our goal should be that one does not need to
> check the CI, because no bad commits make it into the repository.

That's utopic and contrary to any principles of development because it
contests the sole existence of bugs. What matters is that we limit the
number of issues, we limit their duration, we remain as transparent as
possible on the fixes for these issues, and we limit the impact and
exposure for the undetected ones. The amount of mental efforts needed
to go through hard processes is huge and a first cause of errors. I
don't count the number of times I've done some mistakes when pushing

Re: CI caching improvement

[Tim Düsterhus]

William,

On 3/8/22 16:06, William Lallemand wrote:

Let me know if we can improve the attached patch, otherwise I'll merge
it.



Let me make a competing proposal that:

1. Keeps the complexity out of the GitHub workflow configuration in 
haproxy/haproxy.

2. Still allows VTest caching.

For my https://github.com/TimWolla/haproxy-auth-request repository I 
have created a reusable GitHub Action to perform the HAProxy 
installation similar to 'actions/checkout':


https://github.com/TimWolla/action-install-haproxy/

I just spent a bit of time to fork that action and to make it VTest 
specific:


https://github.com/TimWolla/action-install-vtest/

The action receives the VTest branch or commit as the input and will 
handle all the heavy lifting of downloading, compiling and caching VTest.


The necessary changes to HAProxy then look like this:

https://github.com/TimWolla/haproxy/commit/78af831402e354f22d67682be0f323dec9c26a52

This basically replaces the use of 'scripts/build-vtest.sh' by 
'timwolla/action-install-vtest@main', so the configuration in the 
haproxy/haproxy repository is not getting any more complicated, as all 
the heavy lifting is done in the action which can be independently 
tested and maintained.


If this proposal sounds good to you, then I'd like to suggest the following:

1. Willy creates a new haproxy/action-install-vtest repository in the 
haproxy organization.
2. Willy creates a new GitHub team with direct push access to that 
repository.
3. Willy adds me to that team, so that I can maintain that repository 
going forward (e.g. to handle the Dependabot pull requests that keep the 
JavaScript dependencies up to date).


If that repository is properly set up, I'll send a patch to switch over 
haproxy/haproxy to make use of that action.


Best regards
Tim Düsterhus

Re: CI caching improvement

[William Lallemand]

On Tue, Mar 08, 2022 at 04:17:00PM +0100, Tim Düsterhus wrote:
> William
> 
> On 3/8/22 16:06, William Lallemand wrote:
> > Also, I'm wondering if we could also cache the build of HAProxy, you
> > could think that weird, but in fact it will help relaunch the tests when
> > one is failing, without rebuilding the whole thing.
> 
> Please don't. You always want to run a clean build, otherwise you're 
> going to get super-hard-to-debug failures if some object file is 
> accidentally taken from the cache instead of a clean build.
> 

The cache is supposed to be done once the build is valid, if the build
are not reproducible I think we have a bigger problem here, we are not
supposed to trigger random behavior of the compiler.

> I've heard that redoing a single failed build instead of the full matrix 
> is already on GitHub's roadmap, so the problem will solve itself.
> 
Maybe but that will still build HAProxy for nothing, if you have an
example of an unreproduicible build case with HAProxy I'm fine with this
statement but honestly nothing come to my mind. Even if one of the
distribution component is broken during a lapse of time, the problem
will reappear without the cache.

> > Let me know if we can improve the attached patch, otherwise I'll merge
> > it.
> > 
> 
> I don't like it. As you say: It's ugly and introduces complexity for a 
> mere 8 second gain. Sure, we should avoid burning the planet by wasting 
> CPU cycles in CI, but there's a point we're the additional complexity 
> results in a maintenance nightmare.

I don't really see where there is complexity, it's just a curl to get an
ID, and I don't have the impression that the github cache is not working
corretly since we are using it with the ssl libraries.

The build is conditioned by a unique if statement and a simple sh
oneliner.

It's not really 8s, it's 8s per job, and someting not everything is run in
parallel, without mentionning private repositories where the time is
accounted. Also that was also a way that could be used for quictls which
still takes a lot of time.

-- 
William Lallemand

Re: CI caching improvement

[Илья Шипицин]

вт, 8 мар. 2022 г. в 21:13, William Lallemand :

> On Tue, Mar 08, 2022 at 08:38:00PM +0500, Илья Шипицин wrote:
> >
> > I'm fine with swapping "vtest" <--> "haproxy" order.
> >
>
> Ok, I can do that.
>
> > also, I do not think current patch is ugly, it is acceptable for me (if
> we
> > agree to save 8 sec).
>
> Honestly I don't see the value in building the same binary which never
> change again and again, and it does not add much complexity so I think
> that's fine.
>
>
> > I'm afraid that current patch require some fix,
> > because GitHub uses cache in exclusive way, i.e.
> > you need unique cache key per job, current cache key is not job dependent
> > (but the rest looks fine)
> >
>
> I don't think I get that, the key is a combination of the VTest commit +
> the hash per job.
>
> key: vtest-${{ steps.vtest-id.outputs.key }}-${{
> steps.generate-cache-key.outputs.key }}
>

oops. as long as key includes ${{ steps.generate-cache-key.outputs.key }}
it is unique. no other change is required.


>
>
> Thanks,
>
> --
> William Lallemand
>

Re: CI caching improvement

[William Lallemand]

On Tue, Mar 08, 2022 at 08:38:00PM +0500, Илья Шипицин wrote:
> 
> I'm fine with swapping "vtest" <--> "haproxy" order.
> 

Ok, I can do that.

> also, I do not think current patch is ugly, it is acceptable for me (if we
> agree to save 8 sec).

Honestly I don't see the value in building the same binary which never
change again and again, and it does not add much complexity so I think
that's fine.


> I'm afraid that current patch require some fix,
> because GitHub uses cache in exclusive way, i.e.
> you need unique cache key per job, current cache key is not job dependent
> (but the rest looks fine)
> 

I don't think I get that, the key is a combination of the VTest commit +
the hash per job.

key: vtest-${{ steps.vtest-id.outputs.key }}-${{ 
steps.generate-cache-key.outputs.key }}


Thanks,

-- 
William Lallemand

Re: CI caching improvement

[Tim Düsterhus]

Willy,

On 3/8/22 16:24, Willy Tarreau wrote:

Hi Tim,

On Tue, Mar 08, 2022 at 04:17:00PM +0100, Tim Düsterhus wrote:

William

On 3/8/22 16:06, William Lallemand wrote:

Also, I'm wondering if we could also cache the build of HAProxy, you
could think that weird, but in fact it will help relaunch the tests when
one is failing, without rebuilding the whole thing.


Please don't. You always want to run a clean build, otherwise you're going
to get super-hard-to-debug failures if some object file is accidentally
taken from the cache instead of a clean build.


What risk are you having in mind for example ? I'm personally thinking
that vtest is sufficiently self-contained to represent almost zero risk.
We could possibly even pre-populate it and decide to rebuild if it's not
able to dump its help page.


This was a reply to "cache the build of HAProxy", so unrelated to VTest. 
As the HAProxy build is what we want to test, it's important to always 
perform a clean build.



I don't like it. As you say: It's ugly and introduces complexity for a mere
8 second gain. Sure, we should avoid burning the planet by wasting CPU
cycles in CI, but there's a point we're the additional complexity results in
a maintenance nightmare.


In fact it's not just the number of CPU cycles, it's also a matter of
interactivity. A long time ago I remember that we used to push and wait
for the build to complete. For a long time it has changed, we push and
switch to something else, and completely forget that we'd submitted a
build. For sure it's not vtest alone that will radically change this,
but my impression is that it could help, and near-zero risk. But that's
my perception and I could be mistaken.


I can only comment from my side: For me any patches are sent are 
inherently asynchronously processed by the CI, because I don't know when 
you are taking them. So I test them as good as I can locally and usually 
the CI is green afterwards.


Looking at this: 
https://github.com/haproxy/haproxy/actions/runs/1952139455, the fastest 
build is the gcc no features build with 1:33. Even if we subtract the 8 
seconds for VTest, then that's more than I'd be willing to synchronously 
wait. The slowest are the QUICTLS builds with 6:21, because QUICTLS is 
not currently cached.


FWIW: Even 6:21 is super fast compared to other projects. I've recently 
contributed to PHP and CI results appear after 20 minutes or so.


In any case I believe that our goal should be that one does not need to 
check the CI, because no bad commits make it into the repository. 
Unfortunately we still have some flaky tests, that needlessly take up 
attention, because one will need to check the red builds. If those are 
gone, then I expect the vast majority of the commits to be green so that 
it only catches mistakes in strange configurations that one cannot 
easily test locally.


Of course if you think that the 8 seconds will improve your workflow, 
then by all means: Commit it. From my POV in this case the cost is 
larger than the benefit. Caching is one of the hard things in computer 
science :-)


Best regards
Tim Düsterhus

Re: CI caching improvement

[Илья Шипицин]

I thought to build "vtest" just once and deliver using artifacts to all
jobs. It will save some electricity, also GitHub sometimes throw 429 when
we download "vtest" in too many parallel ways.
however, it will not speed up, so I postoned that idea (something like that
https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts
)


I'm fine with swapping "vtest" <--> "haproxy" order.

also, I do not think current patch is ugly, it is acceptable for me (if we
agree to save 8 sec). I'm afraid that current patch require some fix,
because GitHub uses cache in exclusive way, i.e.
you need unique cache key per job, current cache key is not job dependent
(but the rest looks fine)

вт, 8 мар. 2022 г. в 20:06, William Lallemand :

> Hello,
>
> The attached patch implements a somehow ugly way to cache the VTest
> binary, basically it gets the commit ID by doing a curl of the
> master.patch on the github URL.
>
> It allows to save ~8s per matrix row, which is around 160s in total. I
> know there is a small window where the curl and the git clone won't have
> the same ID but that will be rebuild anyway for the next build, so
> that's fine in my opinion.
>
> We could probably use the same approach to cache quictls or anything
> that uses a git repository.
>
> Also, I'm wondering if we could also cache the build of HAProxy, you
> could think that weird, but in fact it will help relaunch the tests when
> one is failing, without rebuilding the whole thing.
>
> Let me know if we can improve the attached patch, otherwise I'll merge
> it.
>
> Regards,
>
> --
> William Lallemand
>

Re: CI caching improvement

[Willy Tarreau]

Hi Tim,

On Tue, Mar 08, 2022 at 04:17:00PM +0100, Tim Düsterhus wrote:
> William
> 
> On 3/8/22 16:06, William Lallemand wrote:
> > Also, I'm wondering if we could also cache the build of HAProxy, you
> > could think that weird, but in fact it will help relaunch the tests when
> > one is failing, without rebuilding the whole thing.
> 
> Please don't. You always want to run a clean build, otherwise you're going
> to get super-hard-to-debug failures if some object file is accidentally
> taken from the cache instead of a clean build.

What risk are you having in mind for example ? I'm personally thinking
that vtest is sufficiently self-contained to represent almost zero risk.
We could possibly even pre-populate it and decide to rebuild if it's not
able to dump its help page.

> I don't like it. As you say: It's ugly and introduces complexity for a mere
> 8 second gain. Sure, we should avoid burning the planet by wasting CPU
> cycles in CI, but there's a point we're the additional complexity results in
> a maintenance nightmare.

In fact it's not just the number of CPU cycles, it's also a matter of
interactivity. A long time ago I remember that we used to push and wait
for the build to complete. For a long time it has changed, we push and
switch to something else, and completely forget that we'd submitted a
build. For sure it's not vtest alone that will radically change this,
but my impression is that it could help, and near-zero risk. But that's
my perception and I could be mistaken.

Thanks,
Willy

Re: CI caching improvement

[Tim Düsterhus]

Willy,
William,

On 3/8/22 16:14, Willy Tarreau wrote:

Due to this I think we should move the build of vtest after the build
of haproxy (and generally, anything that's not needed for the build
ought to be moved after). This will at least save whatever can be
saved on failed builds.


That on the other hand makes sense to me. It just changes the order of 
the steps and thus brings a benefit without adding complexity.


Best regards
Tim Düsterhus

Re: CI caching improvement

[Tim Düsterhus]

William

On 3/8/22 16:06, William Lallemand wrote:

Also, I'm wondering if we could also cache the build of HAProxy, you
could think that weird, but in fact it will help relaunch the tests when
one is failing, without rebuilding the whole thing.


Please don't. You always want to run a clean build, otherwise you're 
going to get super-hard-to-debug failures if some object file is 
accidentally taken from the cache instead of a clean build.


I've heard that redoing a single failed build instead of the full matrix 
is already on GitHub's roadmap, so the problem will solve itself.



Let me know if we can improve the attached patch, otherwise I'll merge
it.



I don't like it. As you say: It's ugly and introduces complexity for a 
mere 8 second gain. Sure, we should avoid burning the planet by wasting 
CPU cycles in CI, but there's a point we're the additional complexity 
results in a maintenance nightmare.


Best regards
Tim Düsterhus

Re: CI caching improvement

[Willy Tarreau]

Hi William,

On Tue, Mar 08, 2022 at 04:06:45PM +0100, William Lallemand wrote:
> Hello,
> 
> The attached patch implements a somehow ugly way to cache the VTest
> binary, basically it gets the commit ID by doing a curl of the
> master.patch on the github URL.
> 
> It allows to save ~8s per matrix row, which is around 160s in total. I
> know there is a small window where the curl and the git clone won't have
> the same ID but that will be rebuild anyway for the next build, so
> that's fine in my opinion.
> 
> We could probably use the same approach to cache quictls or anything
> that uses a git repository.
> 
> Also, I'm wondering if we could also cache the build of HAProxy, you
> could think that weird, but in fact it will help relaunch the tests when
> one is failing, without rebuilding the whole thing.
> 
> Let me know if we can improve the attached patch, otherwise I'll merge
> it.

Thanks for this. I can't judge on how this can be improved, however
I noticed today that one of the important goals of the CI is to see
if stuff builds at all (and we often break one platform or another).
Due to this I think we should move the build of vtest after the build
of haproxy (and generally, anything that's not needed for the build
ought to be moved after). This will at least save whatever can be
saved on failed builds.

Willy

CI caching improvement

[William Lallemand]

Hello,

The attached patch implements a somehow ugly way to cache the VTest
binary, basically it gets the commit ID by doing a curl of the
master.patch on the github URL.

It allows to save ~8s per matrix row, which is around 160s in total. I
know there is a small window where the curl and the git clone won't have
the same ID but that will be rebuild anyway for the next build, so
that's fine in my opinion.

We could probably use the same approach to cache quictls or anything
that uses a git repository.

Also, I'm wondering if we could also cache the build of HAProxy, you
could think that weird, but in fact it will help relaunch the tests when
one is failing, without rebuilding the whole thing.

Let me know if we can improve the attached patch, otherwise I'll merge
it.

Regards,

-- 
William Lallemand
>From 34649ae5549a73d0f43530794f47861fb679510e Mon Sep 17 00:00:00 2001
From: William Lallemand 
Date: Tue, 8 Mar 2022 14:49:25 +0100
Subject: [PATCH] CI: github: add VTest to the github actions cache

Get the latest master commit ID from VTest and use it as a key for the
cache. VTest takes around 8s to build per matrix row, we save around
160s of CI with this.
---
 .github/workflows/vtest.yml | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/vtest.yml b/.github/workflows/vtest.yml
index a9e86b6a22..38099093cd 100644
--- a/.github/workflows/vtest.yml
+++ b/.github/workflows/vtest.yml
@@ -55,6 +55,11 @@ jobs:
   run: |
 echo "::set-output name=key::$(echo ${{ matrix.name }} | sha256sum | awk '{print $1}')"
 
+- name: Get VTest master commit ID
+  id: vtest-id
+  run: |
+echo "::set-output name=key::$(curl -s https://github.com/vtest/VTest/commit/master.patch 2>/dev/null | head -n1 | awk '{print $2}')"
+
 - name: Cache SSL libs
   if: ${{ matrix.ssl && matrix.ssl != 'stock' && matrix.ssl != 'BORINGSSL=yes' && matrix.ssl != 'QUICTLS=yes' }}
   id: cache_ssl
@@ -70,6 +75,14 @@ jobs:
   with:
 path: '~/opt-ot/'
 key: ot-${{ matrix.CC }}-${{ env.OT_CPP_VERSION }}-${{ contains(matrix.name, 'ASAN') }}
+
+- name: Cache VTest binary
+  id: cache_vtest
+  uses: actions/cache@v2
+  with:
+path: '~/vtest/'
+key: vtest-${{ steps.vtest-id.outputs.key }}-${{ steps.generate-cache-key.outputs.key }}
+
 - name: Install apt dependencies
   if: ${{ startsWith(matrix.os, 'ubuntu-') }}
   run: |
@@ -86,8 +99,11 @@ jobs:
 brew install socat
 brew install lua
 - name: Install VTest
+  if: ${{ steps.cache_vtest.outputs.cache-hit != 'true' }}
   run: |
 scripts/build-vtest.sh
+mkdir ~/vtest/
+cp ../vtest/vtest ~/vtest/
 - name: Install SSL ${{ matrix.ssl }}
   if: ${{ matrix.ssl && matrix.ssl != 'stock' && steps.cache_ssl.outputs.cache-hit != 'true' }}
   run: env ${{ matrix.ssl }} scripts/build-ssl.sh
@@ -134,7 +150,7 @@ jobs:
 # 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
+make reg-tests VTEST_PROGRAM=~/vtest/vtest REGTESTS_TYPES=default,bug,devel
 - name: Show VTest results
   if: ${{ failure() && steps.vtest.outcome == 'failure' }}
   run: |
-- 
2.32.0