On Wed, Jun 5, 2024, at 10:38 AM, Ghanshyam Mann 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
patches that were recently merged [0]. Until the reverts land
---- On Tue, 04 Jun 2024 18:15:49 -0700 Ihar Hrachyshka wrote --- the RE2 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.
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).
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/+/921309
I suspect this could be cleaned up to reuse a single variable
https://review.opendev.org/c/openstack/neutron-tempest-plugin/+/921330 listing files to ignore through */& syntax. >
Thanks,
-Brian
[0] https://review.opendev.org/q/topic:"files-negate" [1] https://github.com/google/re2/issues/156
[2] https://review.opendev.org/c/openstack/ironic/+/921319/1/zuul.d/ironic-jobs....