sriov, Let SyncVMI DeadlineExceeded warning be non fatal in tests

[oshoval]

In case running multi sriov jobs on CI,
transient warnings such as the following, occur [1]
unknown error encountered sending command SyncVMI: rpc error: code = DeadlineExceeded desc = context deadline exceeded
It happens only when there are 2+ jobs at the same time, and if each of the job runs
2 VMs at the same time.
In this case it happens to around 30% of the jobs.

Kubevirt itself has a re-enqueuing mechanism for this kind of warnings,
so their effect is transient, and the test would pass once continued.

Add a mechanism to allow switching selected warnings
to be non fatal.
Use this mechanism in order to update this warning severity
to log only instead of failing the test.

There are some open issues that point
its related to resource extensive usage [2] (tried the suggested method):
once there are lots of vms spinning, each vm needs its own cpu quota,
1 cpu goes to kube-reserve (0.5) and system-reserve (0.5) according kind config
which leaves us with 1 cpu total (i tried allocating 2 cpus total for the cluster),
and it seems its not enough.

See also [3] about open issues with system reserve,
and why its better to not try and solve it at this very moment, for kind.

Once we bump k8s to 1.19 in kind it might help, unless it affects only windows [4].

The timeout happens between the virt-handler and virt-launcher gRPC.
See [5] for more info.

[1]
https://prow.apps.ovirt.org/view/gcs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/822/rehearsal-pull-kubevirt-e2e-kind-1.17-sriov/1348156475482050563

[2] etcd-io/etcd#12234 (comment)
[3] kubernetes/kubernetes#72881
[4] kubernetes/kubernetes#95735 (comment)

[5] #5027

Signed-off-by: Or Shoval oshoval@redhat.com

None

[oshoval]

/cc @EdDev @qinqon

[EdDev]

Ignoring warnings may mask real issues and we do not want that.
The reconciliation mechanism is there to recover from problems, but in tests we are suppose to control those or know what to expect explicitly.

If the specific warning that is detected is an acceptable one, then only it should be accepted and nothing more.
A detail explanation of what it is and why we can accept it under a specific condition is to be given (it has been given here partially, mainly through links).

Please also include the failure percentage due to this issue.

[oshoval]

Ignoring warnings may mask real issues and we do not want that.
The reconciliation mechanism is there to recover from problems, but in tests we are suppose to control those or know what to expect explicitly.

If the specific warning that is detected is an acceptable one, then only it should be accepted and nothing more.
A detail explanation of what it is and why we can accept it under a specific condition is to be given (it has been given here partially, mainly through links).

Please also include the failure percentage due to this issue.

I think in this case we can create that mechanism in a following PR, and focus atm on the stuff that is more important (at least in my view).
Running several clusters with several VMs at the same time is resource extensive, so this specific warning occurs (will elaborate more after deeper research upon need).
About masking other warnings, the fact that there is retry mechanism in kubevirt and that e2e already have an option to ignore them, and that those warnings are transient, means imho, that we shouldn't be blocked in this very moment for presenting a new mechanism that filter specific warnings.
We will do it, but after the current effort is done if possible please.
@phoracek @EdDev @qinqon
Wdyt please, can we postpone the mechanism for a following PR, and focus on the critical path (and the right one imho) ?

Its just a warning, it should not fail the test, a better approach maybe is just to show the events in case the tests fails (unless it already shows the events) imo.

Other tests ignore warnings as well, and sriov meant to test SRIOV, not the VM spinning.
Moreover we are doing some relaxations such as etcd in ramFs, because its kind.
I think this change is also legit, at least at phase 1.
Doing phase 2 in the right time will allow us to come more prepared for it usually.

EDIT:
Moreover warnings should not fail the tests, its not an error.
Something that worth investing imo, is to add a mechanism to print the events, unless there is one already
and then in case a warning occurred and the test failed eventually, the warning will be visible there.
Update:
We do have the warning already in artifacts events file

           "reason": "SyncFailed",
            "message": "unknown error encountered sending command SyncVMI: rpc error: code = DeadlineExceeded desc = context deadline exceeded",
            "source": {
                "component": "virt-handler",
                "host": "sriov-worker"
            },

The mechanism to ignoreWarnings existed before this PR, and used in several e2e test files.
WaitForSuccessfulVMIStartIgnoreWarnings
https://github.com/doc22940/kubevirt/search?q=WaitForSuccessfulVMIStartIgnoreWarnings
The mechanism:
https://github.com/doc22940/kubevirt/blob/ad8b5db41598834539be5ab83c12c0df099b4714/tests/utils.go#L2367

This PR suggest to ignore the warning, but it will still log it, and have it in the events (it checks for warning in parallel if requested else it just log the warning, and assert for errors, only thing that is worthy imo is to double check that its indeed in parallel).
https://github.com/doc22940/kubevirt/blob/ad8b5db41598834539be5ab83c12c0df099b4714/tests/utils.go#L335

[oshoval]

Addressed comments

[oshoval]

/hold
We can still review please, but i will release the hold once its checked with new code of multi sriov on CI.
(i did check locally but with injection)

[oshoval]

/hold cancel
tested on CI

[qinqon]

first pass, is good

[oshoval]

Addressed the resolved comments, (with little additional refactor)
Thanks

[oshoval]

/retest

[oshoval]

/retest

[qinqon]

All good now

[oshoval]

Thanks @qinqon @EdDev

[oshoval]

/retest

[oshoval]

/retest

[oshoval]

/assign @enp0s3

[enp0s3]

Thank you! looks good, please see my comments below.
A general concern is that we don't have unit tests for our test tooling and we are adding more
functionality, but let's leave this task for a separate PR.

[oshoval]

Rebased

[enp0s3]

Thank you
/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• tests/OWNERS [enp0s3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubevirt-commenter-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[kubevirt-commenter-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[kubevirt-commenter-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[kubevirt-commenter-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.