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

explicitly skip cd-rom drives during migration #5020

Merged
merged 1 commit into from Apr 9, 2021
Merged

explicitly skip cd-rom drives during migration #5020

merged 1 commit into from Apr 9, 2021

Conversation

vladikr
Copy link
Member

@vladikr vladikr commented Feb 12, 2021

What this PR does / why we need it:
We should explicitly skip cdrom drives during migration. Adding a functional tests as well to cover this scenario.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Should address: https://bugzilla.redhat.com/show_bug.cgi?id=1927378

Release note:

NONE

@vladikr vladikr requested a review from rmohr February 12, 2021 04:59
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Feb 12, 2021
rmohr
rmohr reviewed Feb 12, 2021
View reviewed changes
pkg/virt-launcher/virtwrap/manager.go
// explicitly skip cd-rom drives
if disk.Device == "cdrom" {
continue
}
if disk.ReadOnly != nil && !migrationVols.isGeneratedVolume(disk.Alias.GetName()) {
Copy link
Member

Choose a reason for hiding this comment

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

What I find interesting: I see here disk.ReadOnly: Do we have an explanation why the readonly cdrom drive was not skipped by this?

@sgarbour
Copy link
Contributor

/retest

1 similar comment
@sgarbour
Copy link
Contributor

/retest

dhiller
dhiller approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

@kubevirt-bot kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 16, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2021
@stu-gott
Copy link
Member

Why is this PR on hold? I can't tell from the discussion why it should be.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021
@stu-gott
Copy link
Member

Ping? Can the hold be removed from this PR? @dhiller? @rmohr?

@vladikr this PR has merge conflicts now.

@dhiller
Copy link
Contributor

dhiller commented Mar 25, 2021

/unhols

@dhiller
Copy link
Contributor

dhiller commented Mar 25, 2021

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2021
rmohr
rmohr reviewed Mar 26, 2021
View reviewed changes
tests/migration_test.go Outdated
tests.ConfirmVMIPostMigration(virtClient, vmi, migrationUID)

// delete VMI
By("Deleting the VMI")
Copy link
Member

Choose a reason for hiding this comment

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

The cleanup is done automatically. The VMi does not have to be extra deleted and waited for to disappear.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, done

tests/migration_test.go Outdated
tests.WaitForVirtualMachineToDisappearWithTimeout(vmi, 240)

},
table.Entry("Should successfully migrate vmi with a cd-rom on a sata bus", "sata", ""),
Copy link
Member

Choose a reason for hiding this comment

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

We could also just add both cdrom drives to a single VMI.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@kubevirt-bot kubevirt-bot added size/S and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M labels Apr 1, 2021
@vladikr
Copy link
Member Author

vladikr commented Apr 2, 2021

/retest

1 similar comment
@vladikr
Copy link
Member Author

vladikr commented Apr 6, 2021

/retest

@vladikr vladikr force-pushed the migrate_with_cdrom branch 2 times, most recently from 5dae2bf to d3fce19 Compare April 7, 2021 01:20
@vladikr
Copy link
Member Author

vladikr commented Apr 7, 2021

/retest

@vladikr
explicitly skip cd-rom drives during migration
51da37c 
Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Apr 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vladikr
Copy link
Member Author

vladikr commented Apr 7, 2021

/retest

stu-gott
stu-gott reviewed Apr 8, 2021
View reviewed changes
Copy link
Member

@stu-gott stu-gott left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021
@vladikr
Copy link
Member Author

vladikr commented Apr 8, 2021

/retest

1 similar comment
@vladikr
Copy link
Member Author

vladikr commented Apr 8, 2021

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 8, 2021 •
edited

@vladikr: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubevirt-check-tests-for-flakes 51da37c link /test pull-kubevirt-check-tests-for-flakes

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/test-infra repository. I understand the commands that are listed here.

@kubevirt-bot kubevirt-bot merged commit e740040 into kubevirt:master Apr 9, 2021
@vladikr
Copy link
Member Author

vladikr commented Apr 19, 2021

/cherrypick release-0.36

@kubevirt-bot
Copy link
Contributor

@vladikr: #5020 failed to apply on top of branch "release-0.36":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/virt-launcher/virtwrap/manager.go
M	tests/migration_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/migration_test.go
CONFLICT (content): Merge conflict in tests/migration_test.go
Auto-merging pkg/virt-launcher/virtwrap/manager.go
Patch failed at 0001 explicitly skip cd-rom drives during migration

In response to this:

/cherrypick release-0.36

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/test-infra repository.

@vladikr vladikr mentioned this pull request Apr 20, 2021
Merged
@rmohr rmohr mentioned this pull request Nov 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr left review comments

@stu-gott stu-gott stu-gott left review comments

@dhiller dhiller dhiller approved these changes

@rnetser rnetser Awaiting requested review from rnetser

Assignees

@dhiller dhiller

@stu-gott stu-gott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vladikr @sgarbour @kubevirt-bot @stu-gott @dhiller @rmohr