Run cockpit storage tests in PRs

[martinpitt]

This provides additional API/regression validation for stratisd PRs.

See https://cockpit-project.org/blog/tmt-cross-project-testing.html

Fixes https://issues.redhat.com/browse/COCKPIT-1070

---

When done seriously, this is an ongoing commitment. We found a couple of stratis bugs/regressions so far, like
#3348 or the regression fixed by # 3486. With this setup, we have a good chance of prevent landing such regressions.

This approach is still fairly new, so let's treat this as an experiment. We do need to talk about commitments and expectations, though. From our side, I propose:

• We don't expect you to become experts in our tests. When they fail, it would be nice if you could have a quick look at the log and see if it's something obvious. But in general, we expect that someone from our team will investigate and discuss that with you. The PR where that happened provides a nice place to collect notes.

• Whenever one of our tests fail, three of our team members will be notified by packit automatically, so that we can timely investigate them. However, when your own tests fail as well, I propose that you fix them first, and we only get active if only the cockpit tests fail.

• We don't expect you to block PRs on these tests, especially not if the testing farm has infrastructure problems . E.g. sometimes they to run into provisioning errors. Just ignore these then -- we see them too in our projects, and will usually prod #testing-farm then. But as you already run your own tests on TF, you should be familiar with this, and such outages will affect current PRs already.

• We would like you to at least give us a chance on that workday to look at a failed test (not infra failure, a "real" one) before you land a PR, so that this all makes sense. If something is urgent and you quick-land, then at least we still retain the benefit of having a trace in which PR tests started to fail, but then the damage is possibly already done. Note that this isn't a big new commitment from our side: we already spend many hours every week looking at regressions, and it takes a lot more effort to track them down weeks after they happened. So this will acually reduce both our and your time spent on hunting down regressions, because the context (PR) is fresh and small, as opposed to "whatever changed in Fedora in the last 4 weeks".

Please let me know about any other question that you may have. I'm happy to discuss them here or in a gmeet for higher bandwidth.

Thanks!

[packit-as-a-service]

Failed to load packit config file:

Please correct data and retry.

For more info, please check out the documentation or contact the Packit team.

[packit-as-a-service]

Cockpit tests failed for commit 9d641b4. @martinpitt, @jelly, @mvollmer please check.

[jbaublitz]

Thanks you for doing this @martinpitt. @mulkieran was discussing adding this anyway so I'll let her give her feedback but I think we'll likely all see this as a major benefit!

[martinpitt]

@jbaublitz right, @mulkieran actually asked us (how) to do that, so I thought I'll toss up a PR to see how it works and to discuss it. Our cockpit tests work now, but the TF task for your own test seems stuck. Could be a transient infra problem, or some packit config collision. I still wanted to add some detail anyway, so let's try another round.

[jbaublitz]

@martinpitt Do you have a way to have the tests run without issuing a command? We haven't figured out how to do that yet. We always have to issue /packit build to get the tests to run in packit.

[mulkieran]

@martinpitt Do you have a way to have the tests run without issuing a command? We haven't figured out how to do that yet. We always have to issue /packit build to get the tests to run in packit.

That's really just a Packit configuration choice. We could change that. Historically, we started out with stratis-cli always auto-building for every commit, which was pointless. So we prevented that by using the manual_trigger key. Then, when we started using Packit with stratisd, we copied from stratis-cli. Then, after some complications, we learned that we wanted the build to always happen, and turned that back on. But we left the manual_trigger: true setting for the test job. We could remove that, too.

[mulkieran]

/packit test

[mulkieran]

Should prove that it's just the manual_trigger setting.

[martinpitt]

Aah, thanks @mulkieran , I missed that manual_trigger. I didn't use that anywhere in our projects yet.

I leave it up to you if you want to trigger either or both tests manually. Admittedly we tend to get a bit sloppy on the "meh, let's just always run it" to avoid missing regressions, for the price of requiring more CI resources. From our POV, these are essentially "free"... 🤷‍♂️

[jbaublitz]

@mulkieran Oh, I didn't realize you did that intentionally. I'd personally prefer to run the tests automatically.

[martinpitt]

Note that we currently run all cockpit storage tests (and a few more) here. We can currently only select between three tmt scenarios, but not a more fine-grained "test stratis" only. If that is or becomes a problem, I can look into making the selection more fine-grained.

[mulkieran]

Ok! We all seem to be happy w/ removing the manual_trigger key so I merged a PR to do that. Please rebase to pull in that change.

Also, I have one request for using an identifier key in the existing job which I hope makes sense.

Other than that, this is all fine w/ me, so we should be able to merge the PR shortly.

[mulkieran]

Would prefer this identifier value to be "cockpit" as we may well seek out some reverse dependency testing with other clients.

Would prefer adding identifier key to our job above giving it value "local", and then sorting out fmf adjust+ entries as appropriate.

What is the meaning of + at the end of "adjust", btw?

[martinpitt]

Also, I have one request for using an identifier key in the existing job which I hope makes sense.

I previously had "self" (but argh, I put it onto the COPR build, not the test 🙈 ), but changing it to "local" is fine for me (sounds a bit strange, though).

adjust+ means that it adds to other adjust+ rules, instead of overwriting it. I'm not sure how useful that is. I dropped the pluses for now, I doubt that it'll get significantly more complex anyway.

I also made the context selector more symmetrical, and it should now easily allow adding more test plans for other projects.

[martinpitt]

To make it completely consistent, the identifier should actually be "all" now, to correspond to plans/all.fmf. Please let me know if you want me to rename the file to "local.fmf", or change the identifier to "all".

[martinpitt]

Ah, the explicit plan = all fails, and I also think that will badly break downstream gating. I reverted that.

[mulkieran]

@martinpitt Looks like we're ok w/ it as is. Plz rebase as soon as convenient, and I'll merge.

[martinpitt]

Sure! Thanks!