On 6/5/24 3:05 PM, Clark Boylan wrote:
On Wed, Jun 5, 2024, at 10:38 AM, Ghanshyam Mann wrote:
---- On Tue, 04 Jun 2024 18:15:49 -0700 Ihar Hrachyshka wrote ---
On Tue, Jun 4, 2024 at 4:52 PM Clark Boylan cboylan@sapwetik.org>
wrote:
On Tue, Jun 4, 2024, at 1:06 PM, Brian Haley wrote:
Hi Neutrinos,
It was noticed today by Mohammed Naser that the neutron gate was
only
running the docs and pep8 jobs. We were able to track it down to
the RE2
patches that were recently merged [0]. Until the reverts land
please
don't +W anything since it could merge untested and would just
need to
be reverted.
Other projects might have similar issues if they updated code
using the
'negate' directive for regex recently as we found it doesn't
actually
work with RE2 when using negative lookahead [1].
Note this is very misleading. It is true that re2 does not support
negative lookahead. Zuul's implementation does a positive match then negates the result. This is why you have to say things like regex: foo negate: true. This works outside of typical regex syntax to negate the result.
I think neutron's problem was assuming that negate will be in-place
replacement for the use case. Now I don't see how it could in this specific scenario (where irrelevant-files filters are ORed.)
The problem here is not re2 or negative lookahead regexes themselves
but instead a conversion to non equivalent regular expressions. If we look at an ironic example [2] the old working regex with native negative lookahead support said "all files in tools/ except for bandit.yml are an irrelevant file". The new converted regex said "all files but tools/bandit.yml are an irrelevant file". This meant only changes including updates to tools/bandit.yml would trigger the job.
Yeah, and that is why "negate=true" was considered as direct replacement of "?^" negative regex for zuul config which make it wrong when used in 'irrelevant-files' or 'files'
No, this isn't necessarily wrong for irrelevant-files or files. It depends on what you are trying to achieve. You need to write regex rules that match the files appropriately for your goals. That may end up using negate: true. This is why I said the previous statement is misleading. It made it sound like negate: true is fundamentally broken. It isn't; it does what it says on the tin. Unfortunately, that wasn't what you intended it to do.
Understood, it was our misunderstanding of things that lead to the breakage, and I don't think we can get it to work correctly in the irrelevant-files section based on testing.
We do still need to drop the use of native negative lookaheads in
zuul configs as re2 regexes will be the only valid regexes in future zuul releases. Quick reverts are a good initial step, but I don't want to have an impression this is something we can revert then leave as is.
So what is the solution here? adding all files explicitly in either 'irrelevant-files' or 'files' does not look good as it can end up having a very lengthy file or need updates every new file is added in that directory. And we cannot add both 'irrelevant-files' or 'files' together.
For Neutron it appears that the intention of those irrelevant files rules is to keep jobs from running when source code is not modified. I made a suggestion in IRC today that Neutron update their jobs to do something like:
files: - ^.*.py - zuul.d/project.yaml irrelevant-files: - ^neutron/tests/unit/.*$ - ^neutron/tests/fullstack/.*$ - other overmatch exclusion rules here
This matches the intended behavior of "only run this job when source code is changed" much better than managing a massive amount of irrelevant files rules. In the ironic case they decided they could just run bandit jobs more often. I think this is also a valid approach as we often try to over optimize these rules and confuse ourselves (the current situation is an excellent example of this).
Thanks for the info, I did open a bug to track this as a better (read: shorter) way to accomplish what the old "?!(pattern)" expression was doing.
-Brian
[0] https://bugs.launchpad.net/neutron/+bug/2068523
Also, another point is that if Zuul converts these warnings to errors then this will have another big impact on OpenStack where things were working in the past and ended up in error. Can we keep Zuul more backward compatible especially for the things already being in used so many places?
Zuul is intentionally taking this slow as we knew this would be impactful to openstack and others. The backward compatibility path is warning projects that this will become an error giving people plenty of time to convert their rules. That said eventually we do need to stop using the builtin regex library.
-gmann
We are landing definitions that will list all files to ignore
explicitly:
https://review.opendev.org/c/openstack/neutron-tempest-plugin/+/921330
https://review.opendev.org/c/openstack/neutron/+/921309
I suspect this could be cleaned up to reuse a single variable
listing files to ignore through */& syntax. >
Thanks,
-Brian
[0] https://review.opendev.org/q/topic:%22files-negate" [1] https://github.com/google/re2/issues/156
[2]
https://review.opendev.org/c/openstack/ironic/+/921319/1/zuul.d/ironic-jobs....