QA (public) » openQA Project (public) » openQA Infrastructure (public)

I'd like to suggest optional adoption of new API. One possible approach that come to my mind is

if(get_var('NEW_MULTILINE_CHECK') {
   die "Multiline commands are not supported:\n$cmd;\n" if $cmd =~ m/\n/;
}

So any team or JobGroup can OptIn to the new code. Code like this can be probably merged without to ask or wait anyone. After some time the code can be changed to more direct and "mandatory"

die "Multiline commands are not supported:\n$cmd;\n" if $cmd =~ m/\n/;

I would go further than what @mpagot suggests, and log the "would have been errors" to tackle them before going from optional to mandatory.

Hello, from openQA reviewer's of that week perspective, I feel like the question should not be

Why wasn't it clear to everyone that this was going to break some tests?

but instead

Why the intention to merge something that was going to break some tests, instead of either fixing or asking test owners to fix tests to make sure anything that gets merged does not break?

I think I can be defined as a openQA stakeholder as both a core team developer and openQA reviewer and I think (of course looking at my interests, but I feel that aligns to customer's) that OSD shall be treated more as a production environment where 'keep it running' is the first and foremost priority.

One day of delay for update release is more important than 2-4 months delay to merge this PR IMHO, open to be corrected as I am not familiar with the real impact of such fix.

A bit of context:
We all know that openQA is one crucial tool to automate testing and approval of updates to our customers, we also know that the process (for various reasons) is not at its best and we happen to delay approvals for several days even when all tools work at their best.
Because of that, I don't like when we deliberately break things, distracting multiple people from multiple squads from working on what they planned to do and from working on problems that could not be easily avoided like the one we are talking about.

I consider distracting devs also the time they need to review an already prepared PR, not just fixing the real issue, due to the mindset 'You don't rush stuff in "production", not even fixes, if you can "easily" revert the problem instead.'

I understand that there was also a good dose of bad luck, as the easy revert was not easy in the end due to (AFAIK) having some experts in the topic on vacation (maybe a reason to delay the merge of non urgent breaking things?) and some datacenter/network work being done.
I also see that a super strict process to change stuff in openQA can be painful and in the end if no one takes action from a "well advertised warning" we need to break things to avoid being stuck forever.

But I am convinced we can meet somehow in the middle.

EDIT:
I really like the point of @mpagot ! Handling disruptive changes with an easy way out, especially if the revert is not that simple.
I think we had a similar discussion some months/years ago when the needle detecting algorithm was changed, causing the need of recreation of hundred of needles, in "emergency mode".
I see it's a multi step and more painful thing, but would avoid such situations, something like:

• deployed, disabled, OPT-IN feature flag
• deployed, enabled, OPT-OUT feature flag
• deployed, enabled, flag removed With the possibility to avoid step 1 for simple situations.

I catch the occasion to apologize if I sounded "bothered" by this event, I want to clarify that have nothing against anyone involved, I am just passionate about the painful review process and (very) opinionated about the 'production' concept that I tried to describe on this comment.

I am happy to hear different opinions and also discuss that in a meeting if needed!

I agree with all points from mgrifalconi

@livdywan what I see what happened is that you removed my review in https://github.com/os-autoinst/os-autoinst/pull/2342#event-10171498200 which I would not have done. I would not have approved the PR due to the "this will break stuff" notice that the pull request clearly notes and I would have at least asked more people to review the potential impact and give them time to prepare fixes upfront or propose alternatives. So my suggestion for the future: Be more careful to remove reviews from others. Or in more simple terms: Do not remove reviews from others!

mpagot wrote in #note-1:

So any team or JobGroup can OptIn to the new code. Code like this can be probably merged without to ask or wait anyone. After some time the code can be changed to more direct and "mandatory"

That would simply result in everybody ignoring the code change until it becomes mandatory and then get caught by surprise just as much.

bschmidt wrote in #note-2:

I would go further than what @mpagot suggests, and log the "would have been errors" to tackle them before going from optional to mandatory.

If there was an easy option to do that, I'd do that. Do you have a specific suggestion how to find script_run() calls with multiline commands other than making them break?

Note that @dimstar identified a total of 14 broken tests (+2 more identified by others) and fixes for all of them were submitted within 24 hours.

mgrifalconi wrote in #note-4:

A bit of context:
We all know that openQA is one crucial tool to automate testing and approval of updates to our customers, we also know that the process (for various reasons) is not at its best and we happen to delay approvals for several days even when all tools work at their best.
Because of that, I don't like when we deliberately break things, distracting multiple people from multiple squads from working on what they planned to do and from working on problems that could not be easily avoided like the one we are talking about.

I consider distracting devs also the time they need to review an already prepared PR, not just fixing the real issue, due to the mindset 'You don't rush stuff in "production", not even fixes, if you can "easily" revert the problem instead.'

I understand that there was also a good dose of bad luck, as the easy revert was not easy in the end due to (AFAIK) having some experts in the topic on vacation (maybe a reason to delay the merge of non urgent breaking things?) and some datacenter/network work being done.
I also see that a super strict process to change stuff in openQA can be painful and in the end if no one takes action from a "well advertised warning" we need to break things to avoid being stuck forever.

But I am convinced we can meet somehow in the middle.

Let me also add some context that my PR was not just a change for the sake of change. We've been slowly transitioning tests from VNC console to serial terminal because it's an order of magnitude faster and more reliable. But multiline commands don't work properly on serial terminal because each newline will cause shell to print extra characters back and the check whether the command was echoed correctly will fail. It's a really confusing failure when you see that for the first time so most of our colleagues will waste many hours trying to figure out why it's suddenly failing. And let's just say that I've seen enough wrong attempts to fix those problems that would actually make things worse so I decided to eliminate this problem once and for all.

MDoucha wrote in #note-6:

If there was an easy option to do that, I'd do that. Do you have a specific suggestion how to find script_run() calls with multiline commands other than making them break?

record_soft_fail "poo#12345 - Multiline commands are not supported:\n$cmd;\n"; instead of die, leaving it run for some days and then querying the results?

Usually in packages we submit to our customers, whenever we're dropping an argument or some command, we keep it for some time with a message pointing out that the argument is soon to be deprecated. Why not apply the same to our test code?

okurz wrote in #note-5:

I would not have approved the PR due to the "this will break stuff" notice that the pull request clearly notes and I would have at least asked more people to review the potential impact and give them time to prepare fixes upfront or propose alternatives.

My PR was open for 43 days. Why did you not ask more people to review the impact during that time instead of asking for:

• removing a blank line
• changing a timeout constant to a parameter
• bumping API version which would not have prevented any test failures on its own

I'll argue what I've argued in previous cases like this before: Making such checks opt-in will just not work. Opt-out is the only way to address it.

In this particular case I think it would've been possible to scan the entire testsuite for cases where a newline is in a string literal. That should detect the vast majority of cases.

MDoucha wrote in #note-9:

okurz wrote in #note-5:

I would not have approved the PR due to the "this will break stuff" notice that the pull request clearly notes and I would have at least asked more people to review the potential impact and give them time to prepare fixes upfront or propose alternatives.

My PR was open for 43 days. Why did you not ask more people to review the impact during that time instead of asking for [...]

I only ever commented on the PR in the first two days after opening while it had the "not-ready" label. I did not look into it the following weeks. If I would have stumbled upon the PR again I might have observed that not enough test responsible people have seen the pull request or actively commented on it. However, in general I consider 43 days enough. Everybody had their chance to take a look at the pull request. If they don't, well, that happens :)

favogt wrote in #note-10:

I'll argue what I've argued in previous cases like this before: Making such checks opt-in will just not work. Opt-out is the only way to address it.

I agree. Still, offering the multiple steps that Michael has proposed would not give anyone a reason to complain that they didn't have a chance ;)

acarvajal wrote in #note-8:

MDoucha wrote in #note-6:

If there was an easy option to do that, I'd do that. Do you have a specific suggestion how to find script_run() calls with multiline commands other than making them break?

record_soft_fail "poo#12345 - Multiline commands are not supported:\n$cmd;\n"; instead of die, leaving it run for some days and then querying the results?

Usually in packages we submit to our customers, whenever we're dropping an argument or some command, we keep it for some time with a message pointing out that the argument is soon to be deprecated. Why not apply the same to our test code?

No. record_soft_fail should only be used with product issues. You can use record_info or for messages in the logfile.

MDoucha wrote in #note-7:

Let me also add some context that my PR was not just a change for the sake of change. [...]

Good morning, I never meant to question the validity or reasoning behind your change and I apologize if it sounded otherwise. All my observations are on the impact, deployment and its urgency.
I understand sometimes we need big changes to improve stuff we might take for granted and we need experts like you to develop the best solution possible to go forward even if it require some additional work by multiple people to have a better service in the long run!

I still question that disrupting multiple squads and delaying update approval (in a specific time were the situation was painful already) is not a decision for a single or a few individuals to take. Could have been a valid point of discussion in the POs QE-Sync meeting Wednesdays where all POs would agree on urgency and would allocate resources accordingly.

I see few comments about "making people aware of changes", I'd like to expand on that, since we might lack some common grounds on what "making all stakeolders aware" means, these is my view:
I would not expect all participants/stakeholders of QE to scan os-autinst; openqa; os-autoinst-distri-opensuse all the time, but not even every 6months. Feels a waste of time for dozen of people to read and understand changes that most of the times do not impact their work.
We have regular meetings to sync and talk about common topics and possibly the qe-sync meeting is a good choice.

This is part of a bigger topic that I started to mention a couple of years ago about ownership and areas of responsibility, where it should be well defined who (which squad/team) is responsible for a specific asset(code,server,service).
Then the owner shall be able to know who depends its products, to be able to communicate breaking changes or outages (direct communication, ping to POs, meetings = TCP; not a broadcast into the wind like a PR = UDP).
I see in the meantime we did move towards that direction by:

• using squads instead of individuals in test maintainer description
• moving to git job groups/test suites to find out about changes on openqa

That is just my personal view and again open to discuss and try to better elaborate maybe in a different ticket as I fear we might go too much of topic.

By the way the recording is available internally. We opted to err on the safe side since we were very focussed on the internal SUSE perspectives.
^{^} If anyone has feedback from other communities outside of SUSE, please do feel free to share. We actually talked about communicating this better in release notes.

Trying to come up with a little best practice guide on handling cases like this in the future. Feedback welcome:

When proposing non-trivial changes with the potential of breaking existing tests consider the follow best practice patterns:

• Make the problematic change opt-in via a test variable like MY_NEW_FEATURE_ENABLED to enable the new behavior, and otherwise log a warning only
• If a BARK test is to be conducted to assess the full impact of the change an autoreview regex matching the most relevant error message should be prepared so that affected jobs can be restarted trivially without disrupting daily operation too much.
• Inform all stakeholders in relevant Slack channels, Matrix and mailing lists
• Include an explicit mention in the release notes

One more idea for the future: The main problem with less fatal approaches to marking the buggy tests was that nobody would notice a log message or even a softfail. OpenQA lacks some kind of clearly visible non-fatal warning. Let's add a new function like record_code_warning() that'll behave similarly to record_soft_fail(). It'll add a result box to the test module with a warning icon ⚠️ and a pop-up description.

The result box will have no effect on module/job result value but it'll set a special "warning" flag on the module and job. Test modules with the warning flag will stay visible if you click "Show only failures" on the job detail page (even if the module passed). Jobs with the warning flag will have the warning icon on the test overview page right next to the usual BSC and POO icons.

livdywan wrote in #note-17:

• Make the problematic change opt-in via a test variable like MY_NEW_FEATURE_ENABLED to enable the new behavior, and otherwise log a warning only
• If a BARK test is to be conducted to assess the full impact of the change an autoreview regex matching the most relevant error message should be prepared so that affected jobs can be restarted trivially without disrupting daily operation too much.

I'd like the message to include a link to the according github issue or PR.
That would add all the context for whoever sees the error message.

MDoucha wrote in #note-19:

One more idea for the future: The main problem with less fatal approaches to marking the buggy tests was that nobody would notice a log message or even a softfail. OpenQA lacks some kind of clearly visible non-fatal warning. Let's add a new function like record_code_warning() that'll behave similarly to record_soft_fail(). It'll add a result box to the test module with a warning icon ⚠️ and a pop-up description.

The result box will have no effect on module/job result value but it'll set a special "warning" flag on the module and job. Test modules with the warning flag will stay visible if you click "Show only failures" on the job detail page (even if the module passed). Jobs with the warning flag will have the warning icon on the test overview page right next to the usual BSC and POO icons.

I really like this as well!!!
Code as documentation is always better than communication via release notes or any other channel.

MDoucha wrote in #note-19:

One more idea for the future: The main problem with less fatal approaches to marking the buggy tests was that nobody would notice a log message or even a softfail. OpenQA lacks some kind of clearly visible non-fatal warning. Let's add a new function like record_code_warning() that'll behave similarly to record_soft_fail(). It'll add a result box to the test module with a warning icon ⚠️ and a pop-up description.

The result box will have no effect on module/job result value but it'll set a special "warning" flag on the module and job. Test modules with the warning flag will stay visible if you click "Show only failures" on the job detail page (even if the module passed). Jobs with the warning flag will have the warning icon on the test overview page right next to the usual BSC and POO icons.

I like this, great idea! If this can be made to also leave a result that's easy to query, better yet.