Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Add support for block volumes #347

Merged
merged 1 commit into from Jul 27, 2020
Merged

Add support for block volumes #347

merged 1 commit into from Jul 27, 2020

Conversation

machacekondra
Copy link
Contributor

@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2020
@kubevirt-bot
Copy link

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 dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L labels Jul 21, 2020
@machacekondra
Copy link
Contributor Author

/test all

@machacekondra machacekondra force-pushed the blockvolumesupport branch 2 times, most recently from 1bae132 to 7c8e173 Compare July 22, 2020 11:47
@machacekondra
Copy link
Contributor Author

/test all

@machacekondra machacekondra marked this pull request as ready for review July 22, 2020 12:31
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020
@machacekondra machacekondra force-pushed the blockvolumesupport branch 4 times, most recently from 5095cc1 to 661f1c3 Compare July 23, 2020 13:41
jakub-dzon
jakub-dzon suggested changes Jul 27, 2020
View reviewed changes
pkg/operator/resources/operator/operator.go Show resolved Hide resolved
pkg/operator/resources/operator/operator.go Show resolved Hide resolved
pkg/operator/resources/operator/operator.go Show resolved Hide resolved
pkg/operator/resources/operator/operator.go Show resolved Hide resolved
pkg/providers/ovirt/mapper/mapper.go Outdated
@@ -261,6 +301,7 @@ func (o *OvirtMapper) MapDataVolumes(targetVMName *string) (map[string]cdiv1.Dat
}
quantity, _ := resource.ParseQuantity(diskSizeConverted)
accessMode := o.getAccessMode(diskAttachment)
volumeMode := o.getVolumeMode(disk, o.mappings)
Copy link
Contributor

Choose a reason for hiding this comment

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

To detect the volume mode we're iterating over the same mappings slice for the second time (first - for the storage class in getStorageClassForDisk). In both cases we should get exactly the same mapping instance as a base for retrieving both storage class and volume mode. What do you think about finding the mapping first and than passing it to getVolumeMode and getStorageClassForDisk to extract the needed parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks.

pkg/providers/ovirt/mappings/merger.go
@@ -38,16 +38,62 @@ func extractMappings(externalMappingSpec *v2vv1alpha1.ResourceMappingSpec, crMap
return &primaryMappings, &secondaryMappings
}

func mergeMappings(primaryMappings *[]v2vv1alpha1.ResourceMappingItem, secondaryMappings *[]v2vv1alpha1.ResourceMappingItem) *[]v2vv1alpha1.ResourceMappingItem {
var mapping []v2vv1alpha1.ResourceMappingItem
func mergeNetworkMappings(primaryMappings *[]v2vv1alpha1.NetworkResourceMappingItem, secondaryMappings *[]v2vv1alpha1.NetworkResourceMappingItem) *[]v2vv1alpha1.NetworkResourceMappingItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer both mergeNetworkMappings and mergeStorageMappings to use the same common code, but without common interface or generics it's unlikely that it can be done cleanly. Definitely a good candidate for trying them when they're finally supported.;)

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, I was trying to solve it with reflection, but it was even worse IMHO, so I left the code duplication.

pkg/providers/ovirt/mapper/mapper_test.go Show resolved Hide resolved
@pkliczewski pkliczewski added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 27, 2020
@kubevirt-bot kubevirt-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 27, 2020
@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jakub-dzon, machacekondra, pkliczewski
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

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

@pkliczewski
Copy link
Contributor

@machacekondra I see 2 conflicting files. Would you mind to rebase?

@machacekondra
Add support for block volumes
6e7bf11 
Signed-off-by: Ondra Machacek <omachace@redhat.com>
@machacekondra
Copy link
Contributor Author

@machacekondra I see 2 conflicting files. Would you mind to rebase?

Done.

@pkliczewski pkliczewski merged commit 4bf666c into kubevirt:master Jul 27, 2020
@yaacov yaacov mentioned this pull request Aug 25, 2020
Merged
@ghost ghost mentioned this pull request Nov 17, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkliczewski pkliczewski pkliczewski approved these changes

@jakub-dzon jakub-dzon jakub-dzon approved these changes

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note-none Denotes a PR that doesn't merit a release note. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@machacekondra @kubevirt-bot @pkliczewski @jakub-dzon