sig-monitoring lane should not be optional

[dhiller]

kubevirt/kubevirt#15718 introduced a bug that caused sig-monitoring to fail all the time. Since the lane is optional (i.e. run_if_changed is set), it should be converted to either always_run: true or run_before_merge: true

It needs to get proved that the latter combinations is possible though.

General note: the sig-monitoring is an optional lane not running on every commit, since run_if_changed is set:

project-infra/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml

Line 1285 in ab99372

run_if_changed: ^pkg/monitoring/.*|^tests/monitoring/.*|^tests/libmonitoring/.*|^vendor/github.com/machadovilaca/operator-observability/.*

This might cause issues when changes outside that scope occur that affect the lane, i.e. dependencies etc. .

I'll take a look on why the lane started to fail, maybe coinciding with findings from @avlitman who noticed that another lane was not run at all for quite a while - Aviv can you ensure my memory doesn't fail me here please?

Originally posted by @dhiller in #16017

[dhiller]

/cc @sradco @machadovilaca

[dhiller]

/sig observability

[machadovilaca]

@dhiller do you have an example of the failing test?
i was thinking that maybe we can come up with a way to run just a quick subset of the sig-monitoring tests on all the PRs that we think would already give us a good indication of whether something is broken or not due to dependencies, api changes or such across the codebase

because I don't think we need to run, for example, the destructive tests for the component-related alerts in all PRs

maybe something like this file would be enough: https://github.com/kubevirt/kubevirt/blob/main/tests/monitoring/metrics.go

[dhiller]

@dhiller do you have an example of the failing test? i was thinking that maybe we can come up with a way to run just a quick subset of the sig-monitoring tests on all the PRs that we think would already give us a good indication of whether something is broken or not due to dependencies, api changes or such across the codebase

because I don't think we need to run, for example, the destructive tests for the component-related alerts in all PRs

maybe something like this file would be enough: kubevirt/kubevirt@main/tests/monitoring/metrics.go

@machadovilaca no - not that I remember right now, will dig into that.

What I was wanting to point out was that the PR author can trigger any lane that they want whenever they want, but that doesn't create a safety net in the way that a mandatory lane run before merge does - and which was what hurt us in the mentioned case.

However, my proposition was to convert this to be run only right before merge every time. I'd also be open to split the observability tests into two lanes, one that runs all the time with the named subset and another one that runs the essential things, but just before merge.

WDYT?

[dhiller]

@dhiller do you have an example of the failing test? i was thinking that maybe we can come up with a way to run just a quick subset of the sig-monitoring tests on all the PRs that we think would already give us a good indication of whether something is broken or not due to dependencies, api changes or such across the codebase
because I don't think we need to run, for example, the destructive tests for the component-related alerts in all PRs
maybe something like this file would be enough: kubevirt/kubevirt@main/tests/monitoring/metrics.go

@machadovilaca no - not that I remember right now, will dig into that.

What I was wanting to point out was that the PR author can trigger any lane that they want whenever they want, but that doesn't create a safety net in the way that a mandatory lane run before merge does - and which was what hurt us in the mentioned case.

However, my proposition was to convert this to be run only right before merge every time. I'd also be open to split the observability tests into two lanes, one that runs all the time with the named subset and another one that runs the essential things, but just before merge.

WDYT?

Hey @machadovilaca I didn't follow your discussion about the topic - are there any updates on this one?

[dhiller]

/triage accepted

/assign @machadovilaca

[machadovilaca]

sorry @dhiller missed the update

i think there is a lot of monitoring tests that are very heavy and don't really make sense to run before every merge (namely the ones i mentioned: the destructive tests for the component-related alerts)

i think we can "split the observability tests into two lanes", i can write a proposal for this split, which tests should always run and which should only run_if_changed: *$something/monitoring, and we can discuss over that proposal the degree of confidence they would bring