Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] persistent reservation tests: run in ordered container #12434

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[WIP] persistent reservation tests: run in ordered container #12434

wants to merge 2 commits into from

Conversation

akalenyu
Copy link
Contributor

What this PR does

Refactor the long persistent reservation suite to only perform teardown once after it's done.

This allows us to avoid re-rolling out the feature gate change for each
test. BeforeAll and AfterAll readout: https://onsi.github.io/ginkgo/#setup-in-ordered-containers-beforeall-and-afterall

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note

NONE

@akalenyu
persistent reservation tests: run in ordered container
98ca353 
this allows us to avoid re-rolling out the feature gate change for each
test. BeforeAll and AfterAll readout: x

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@akalenyu
tests: let some tests restore the original kubevirt state themselves
e3099c0 
some tests (especially with before/afterall) will want to not
have the kubevirt CR reverted after each tests, and instead do so
themselves at the appropriate time.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 22, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign alonakaplan for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@akalenyu
Copy link
Contributor Author

/cc @alicefr @xpivarc @mhenriks
tbh I am not so happy with this myself, and I don't see it widely adopted in kubernetes/kubernetes.
but it does shave off a good amount of time off these tests which previously took around 300seconds each

$ cat ./junit.functest_sigstorage_analysis.xml | grep reservation
  <testcase name="[sig-storage] [Serial]SCSI persistent reservation Use LUN disk with presistent reservation Should successfully start a VM with persistent reservation" classname="Tests Suite" status="" time="322.449534721"></testcase>
  <testcase name="[sig-storage] [Serial]SCSI persistent reservation Use LUN disk with presistent reservation Should successfully start 2 VMs with persistent reservation on the same LUN" classname="Tests Suite" status="" time="344.812896486"></testcase>
  <testcase name="[sig-storage] [Serial]SCSI persistent reservation with PersistentReservation feature gate toggled should delete and recreate virt-handler" classname="Tests Suite" status="" time="52.7413015"></testcase>

wdyt?

@akalenyu
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.30-sig-storage

@alicefr
Copy link
Member

alicefr commented Jul 22, 2024

@akalenyu that's a great improvement, many thanks!

xpivarc
xpivarc reviewed Jul 22, 2024
View reviewed changes
tests/tests_suite_test.go
@@ -166,5 +166,10 @@ func resetToDefaultConfig() {
// we can just skip the restore step.
return
}
if ok, _ := CurrentSpecReport().MatchesLabelFilter("kvmanualreset"); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me that we probably don't want to blindly skip the serial case but assert the config have not changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it did though. with my changes you'll never get equality here
(this is an aftereach as opposed to afterall doing the teardown)

tests/storage/reservation.go
@@ -39,7 +39,7 @@ import (
// feature gate PersistentReservation. The enablement/disablement of this
// feature gate redeploys virt-handler pod, and this might interfer with other
// tests.
var _ = SIGDescribe("[Serial]SCSI persistent reservation", Serial, func() {
var _ = SIGDescribe("[Serial]SCSI persistent reservation", Label("kvmanualreset"), Serial, Ordered, func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the biggest time consumer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rerolling the kubevirt installation after enabling/disabling the persistent reservation feature gate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would be inclined to enable the feature gate for Kubevirt testsuite as whole and remove the enabling here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah me too.. talking to the ginkgo maintainer this "ordered" container thing is not really advised

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would be inclined to enable the feature gate for Kubevirt testsuite as whole and remove the enabling here.

with your blessing I will pivot to doing that @alicefr @xpivarc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xpivarc we still need to add a configuration for this feature, but in general persistent reservation should always been off by default since it deploys an additional privileged container. Right now, we are wrongly using the fg for enabling/disabling the feature

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2024
@kubevirt-bot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@awels awels Awaiting requested review from awels

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@alicefr alicefr Awaiting requested review from alicefr

@mhenriks mhenriks Awaiting requested review from mhenriks

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/storage size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akalenyu @kubevirt-bot @alicefr @xpivarc