CI: Allow github merge queue to run build tests

[kinkie]

Make it easier to land v7 backports by enabling slow tests for
github merge queues.

[rousskov]

Make it easier to land v7 backports by enabling github merge queues

FWIW, it is not clear to me how merge queues make backports "easier to land". I assume we are talking about backports automated using the current mechanism that creates cumulative PRs for fast-forward merging into v7. From what I have seen, all pending automated backports are accumulated into one PR (until that PR is manually merged). Merge queues help when there are multiple concurrent PRs ready to be merged. Let's discuss what I am missing during the next meeting.

[yadij]

Sigh. PR title and description were wrong leading to some confusion. This change does not enable the queue, it enables the tests in slow.yaml to be run by the github-merge-queue bot.

See PR #2333 timeline for what happens if we try to use merge queue without this change:

• yadij added this pull request to the merge queue
• github-merge-queue bot removed this pull request from the merge queue due to no response for status checks
  • 0 checks passed

... that is 0 passed from 0 checks run.

[yadij]

Make it easier to land v7 backports by enabling github merge queues

FWIW, it is not clear to me how merge queues make backports "easier to land". I assume we are talking about backports automated using the current mechanism that creates cumulative PRs for fast-forward merging into v7.

From what I have seen, all pending automated backports are accumulated into one PR (until that PR is manually merged). Merge queues help when there are multiple concurrent PRs ready to be merged.

Indeed, the group is today acting as a form of poor-mans merge queue. One with side effects that prevents us from changing the order easily, doing manual fixes of the ported changes, and forces a separate manual PR creation for anything that does not git cherry-pick trivially (or cases like this PR ironically).

Most of those issues are solved by using the github merge queue out-of-the-box. The rest with some small additional squidadm or CI updates.

Let's discuss what I am missing during the next meeting.

I am unlikely to be there, so I hope the above clears things up.

[rousskov]

Sigh. PR title and description were wrong leading to some confusion. This change does not enable the queue,

Glad my question exposed this problem.

it enables the tests in slow.yaml to be run by the github-merge-queue bot.

See PR #2333 timeline for what happens if we try to use merge queue without this change: ...

Yes, I have seen several failed attempts to use merge queue, for several PRs. I assume that there is no positive test result for this change (in some sandbox repository) because we are (ab)using official repository to figure out what might work and nothing has worked so far, for various reasons.

FWIW, it is not clear to me how merge queues make backports "easier to land". I assume we are talking about backports automated using the current mechanism that creates cumulative PRs for fast-forward merging into v7.

From what I have seen, all pending automated backports are accumulated into one PR (until that PR is manually merged). Merge queues help when there are multiple concurrent PRs ready to be merged.

Indeed, the group is today acting as a form of poor-mans merge queue. One with side effects that prevents us from changing the order easily, doing manual fixes of the ported changes, and forces a separate manual PR creation for anything that does not git cherry-pick trivially

Yes, I am familiar with problems associated with the current backporting automation -- I have been wasting time pointing them out and suggesting changes for years!

Most of those issues are solved by using the github merge queue out-of-the-box.

I strongly disagree. Most problems stem from violating a key "one PR per change" principle in the center of the current automation approach. Merge queues are not going to address those problems.

Let's discuss what I am missing during the next meeting.

I am unlikely to be there, so I hope the above clears things up.

It is still not clear to me how merge queues make backports "easier to land" as claimed in the current PR description. As I noted in my original review, merge queues are not meant to help with merging v7 PRs that have no other/concurrent v7 PRs to queue -- which covers 99% of our backporting use cases. However, perhaps we can make progress by focusing on the new (more specific) claim:

PR title: CI: Allow github merge queue to run build tests

There are build tests in quick.yaml as well, and those tests are executed already, so I assume that "build tests" above implies "slow.yaml tests". Please fix the title if my assumption is correct.

If the goal is to run slow.yaml tests for changes being backported to v7, then let's just add "v7" to v7's slow.yaml push:branches setting. Why drag GitHub merge queues (with all their overheads/complexities/unknowns) into this?

An even better alternative would be to switch to "one PR per change" approach for backported changes, which would eliminate all those other problems you have mentioned and let current Anubis code handle backported PRs as well (AFAICT). This better alternative would require some changes to backporting automation scripts, but since merge queues are expected to required "some small additional squidadm or CI updates", that additional work does not represent a big difference AFAICT, and the overall outcome would be much better.

[yadij]

Sigh. PR title and description were wrong leading to some confusion. This change does not enable the queue,

Glad my question exposed this problem.

it enables the tests in slow.yaml to be run by the github-merge-queue bot.
See PR #2333 timeline for what happens if we try to use merge queue without this change: ...

Yes, I have seen several failed attempts to use merge queue, for several PRs. I assume that there is no positive test result for this change (in some sandbox repository) because we are (ab)using official repository to figure out what might work and nothing has worked so far, for various reasons.

So far all tests have been quite successful. All the cases we need rejection have been proven to work (thanks, I guess, for triggering so many of them with your actions so far). We have only to confirm the case where reviewers approve and the PR is allowed to merge. Other code projects use the bot, so I am confident it will work without having to sandbox a test.

FWIW, it is not clear to me how merge queues make backports "easier to land". I assume we are talking about backports automated using the current mechanism that creates cumulative PRs for fast-forward merging into v7.

From what I have seen, all pending automated backports are accumulated into one PR (until that PR is manually merged). Merge queues help when there are multiple concurrent PRs ready to be merged.

Indeed, the group is today acting as a form of poor-mans merge queue. One with side effects that prevents us from changing the order easily, doing manual fixes of the ported changes, and forces a separate manual PR creation for anything that does not git cherry-pick trivially

Yes, I am familiar with problems associated with the current backporting automation -- I have been wasting time pointing them out and suggesting changes for years!

ROFLMAO! Oh that was a good one. Surely you have not forgotten the outstanding TODO I have had with you since Anubis was in design planning a decade ago ... to support merging onto our vN branches. Github simply beat you to implementing a bot with that feature.

And back to your statement; even small attempts to update the squidadm bot scripts get perfectionist objections such that larger improvements are almost not worth attempting anymore.

Most of those issues are solved by using the github merge queue out-of-the-box.

I strongly disagree. Most problems stem from violating a key "one PR per change" principle in the center of the current automation approach. Merge queues are not going to address those problems.

That is a self-contradicting paragraph. Either replacing the bad design with a correct one solves the issues, or it does not.
... "one bad queue" != "all queues are bad"

Implementing a full merge queue using custom code in our bot is no different that using a third-party bot to do exactly the same thing. Whether we fix squidadm or fix Anubis - both are lacking today; both mean more workload for someone maintaining the bot.

Let's discuss what I am missing during the next meeting.

I am unlikely to be there, so I hope the above clears things up.

It is still not clear to me how merge queues make backports "easier to land" as claimed in the current PR description. As I noted in my original review, merge queues are not meant to help with merging v7 PRs that have no other/concurrent v7 PRs to queue -- which covers 99% of our backporting use cases. However, perhaps we can make progress by focusing on the new (more specific) claim:

I listed more than one need. Focus on the one which has best support as being "enough" is not a helpful approach here to resolving the ~100% failure the other cases have.

The reasons we had/have the grouping in the first place are still valid today in our status quo situation. Simply removing it will only open more issues. We need a proper merge queue bot working before that squidadm mis-feature can be dropped.

PR title: CI: Allow github merge queue to run build tests

There are build tests in quick.yaml as well, and those tests are executed already, so I assume that "build tests" above implies "slow.yaml tests". Please fix the title if my assumption is correct.

Yes, that should be clarified by a better description once the non-maintainer agrees to allow the maintainers to change their CI process.

If the goal is to run slow.yaml tests for changes being backported to v7, then let's just add "v7" to v7's slow.yaml push:branches setting. Why drag GitHub merge queues (with all their overheads/complexities/unknowns) into this?

Consider why we split them in the first place. All the same ($$cash) reasons apply here as to on master. Just with a lesser magnitude - starting at about M/2 for beta branch, and reducing over the year leading up to around M/10 for initial few stable.

An even better alternative would be to switch to "one PR per change" approach for backported changes, which would eliminate all those other problems you have mentioned and let current Anubis code handle backported PRs as well (AFAICT).

Recall that I have been asking for such a feature to be provided by Anubis since before it even existed. Over the past decade there have been no contributions towards either squidadm or Anubis to implement the feature better

... until now. Our github sponsor is providing a bot with all our necessary features (and more we dared only dream about).

This better alternative would require some changes to backporting automation scripts, but since merge queues are expected to required "some small additional squidadm or CI updates", that additional work does not represent a big difference AFAICT, and the overall outcome would be much better.

To clarify the options ahead of us are:
A. do nothing.
B. accept this PR; turn on github queue; update squidadm.
C. fix Anubis; update squidadm.
D. re-implement what Anubis does inside squidadm bot.

Option D ... LOL. Yeah, right.

Option C is a lot more work, both right now for the "fix Anubis" part, ongoing maintenance costs to Measurement Factory fixing bugs we find, and ongoing VM/hardware costs to Measurement Factory[1] hosting for Anubis, and ongoing bandwidth costs to Measurement Factory for the work Anubis does.

Option B has github sponsorship for all the maintenance and bandwidth costs. Squid Foundation takes on VM cycle costs within our existing CI budget.

[kinkie]

My 2c: I have not used merge queues outside of this context. However, enabling them will not
prevent us moving forward with the current process, however good or bad it is, while any kinks
are ironed out.
From what I've seen, they are functioning well enough to help with our v7 maintenance needs

[kinkie]

Let me add: apologies for getting the desciription wrong. I have amended it, it's hopefully correct now,
but @yadij feel free to update it as needed

[rousskov]

Surely you have not forgotten the outstanding TODO I have had with you since Anubis was in design planning a decade ago ... to support merging onto our vN branches.

Anubis supports merging PRs into vN branches. Your request was different.

It is still not clear to me how merge queues make backports "easier to land" as claimed in the current PR description. As I noted in my original review, merge queues are not meant to help with merging v7 PRs that have no other/concurrent v7 PRs to queue -- which covers 99% of our backporting use cases. However, perhaps we can make progress by focusing on the new (more specific) claim:

I listed more than one need.

FWIW, I do not know what specific list you are referring to, but I have read all your responses, and it is still not clear to me (as I have stated). An "I listed" response cannot improve clarity in such a situation. This PR was merged without addressing my concerns.

Consider why we split them in the first place. All the same ($$cash) reasons apply here as to on master. Just with a lesser magnitude - starting at about M/2 for beta branch, and reducing over the year leading up to around M/10 for initial few stable.

In most cases, even without fixing backporting process, the number of tests can be about the same because backporting PRs can usually be merged with a single commit on the backporting PR branch -- we do not merge a lot of master PRs to necessitate accumulating many backporting commits in one backporting PR.