-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flaky test fire VMStorageClassWarning when rxbounce is disabled #1122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Nice work!
tests/monitoring_test.go
Outdated
vm.Spec.Template.ObjectMeta.Annotations = map[string]string{ | ||
"vm.kubevirt.io/os": "windows-10", | ||
} | ||
vm.Spec.Running = ptr.To(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use RunStrategy instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 because there is a plan to deprecate vm.Spec.Running
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests are failing because NewVirtualMachine
is setting the vm.Spec.Running
to false under the hood, and when I set the RunStrategy
it errors with
Type: "FieldValueInvalid",
Message: "Running and RunStrategy are mutually exclusive",
Field: "spec.running",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I revert to use vm.Spec.Running
for now until the NewVirtualMachine
is also changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in NewVirtualMachine
- Running: &running,
+ RunStrategy: ptr.To(kubevirtv1.RunStrategyHalted),
tests/monitoring_test.go
Outdated
var volumes []kubevirtv1.Volume | ||
|
||
if createDataVolume { | ||
createPVCAndPV(vmName, rxbounceEnabled) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
dde15db
to
6cca8e0
Compare
tests/monitoring_test.go
Outdated
var always = kubevirtv1.RunStrategyAlways | ||
vm.Spec.RunStrategy = &always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to do this in a single line? E.g.,
vm.Spec.RunStrategy = ptr.To(kubevirtv1.RunStrategyAlways)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
6cca8e0
to
605e22c
Compare
Thanks @machadovilaca! |
605e22c
to
58ae7e4
Compare
Signed-off-by: João Vilaça <[email protected]>
58ae7e4
to
79e2887
Compare
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akrejcir 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Fix flaky test: [test_id:10549] Should fire VMStorageClassWarning when rxbounce is disabled
jira-ticket: https://issues.redhat.com/browse/CNV-47434
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: