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.

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

Add support for passing disk devices to vsphere machines #512

Merged
merged 1 commit into from Apr 2, 2020

Conversation

alexander-demicev
Copy link
Contributor

Add support for passing disk devices to vsphere machines. The code is based on upstream one https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/master/pkg/services/govmomi/vcenter/clone.go#L74-L85

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 11, 2020
@@ -428,6 +464,21 @@ func clone(s *machineScope) error {
return nil
}

func getDiskSpec(s *machineScope, devices object.VirtualDeviceList) (types.BaseVirtualDeviceConfigSpec, error) {
Copy link
Member

Choose a reason for hiding this comment

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

can we include a unit for this? similar to TestGetNetworkDevices

@enxebre
Copy link
Member

enxebre commented Mar 11, 2020

overall this looks great! PTAL @bison
cc @jcpowermac

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 11, 2020
const (
// FullClone indicates a VM will have no relationship to the source of the
// clone operation once the operation is complete. This is the safest clone
// mode, but it is also the fastest.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description and the one below claim to be the fastest clones 😅 I suspect this one is actually not the fastest? Can you double check and update the comments appropriately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to just leave it then 😅 Didn't realise that was a copy/pasta from upstream

Copy link
Member

Choose a reason for hiding this comment

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

@alexander-demicev
Copy link
Contributor Author

/retest

},
},
{
name: "Succefully get disk spec",
Copy link
Member

Choose a reason for hiding this comment

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

duplicate name with case above

// FullClone indicates a VM will have no relationship to the source of the
// clone operation once the operation is complete. This is the safest clone
// mode, but it is also the fastest.
FullClone CloneMode = "fullClone"
Copy link
Member

Choose a reason for hiding this comment

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

I can't see where FullClone is used at all

// to FullClone.
// When LinkedClone mode is enabled the DiskGiB field is ignored as it is
// not possible to expand disks of linked clones.
// Defaults to LinkedClone, but fails gracefully to FullClone if the source
Copy link
Member

Choose a reason for hiding this comment

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

I can't really see where this "fails gracefully to FullClone", seems we are missing some logic in reconciler line 344

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we set the diskMoveType based on the above
e.g

fullCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsMoveAllDiskBackingsAndConsolidate
	linkCloneDiskMoveType = types.VirtualMachineRelocateDiskMoveOptionsCreateNewChildDiskBacking

See https://github.com/openshift/machine-api-operator/pull/512/files#r394223024

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@alexander-demicev
Copy link
Contributor Author

/retest

@enxebre
Copy link
Member

enxebre commented Mar 25, 2020

/retest
/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 25, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2020
@alexander-demicev
Copy link
Contributor Author

/retest

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Added a couple of questions and suggestions for possible improvements

// CloneMode specifies the type of clone operation.
// The LinkedClone mode is only support for templates that have at least
// one snapshot. If the template has no snapshots, then CloneMode defaults
// to FullClone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it not default to LinkedClone based on

if s.providerSpec.CloneMode == "" || s.providerSpec.CloneMode == vspherev1.LinkedClone {
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deviceSpecs := []types.BaseVirtualDeviceConfigSpec{}

// Only non-linked clones may expand the size of the template's disk.
if snapshotRef == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If the comment above this line is correct, I would expect this if statement to check the diskMoveType == fullCloneDIskMoveType rather than checking the snapshotRef which is kind of a side effect, WDYT? Behaviour should be functionally equivalent if I've understood this correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it should be the same. I took this check from upstream https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/master/pkg/services/govmomi/vcenter/clone.go#L123 , maybe checking for snapshot reference is safer because the next line will fail otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the next line fail is this is not nil? I don't think snapshotRef is used in there is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion, the next line doesn't fail. The clone task fails if diskSpec from L460 is appended and SnapshotRef is nil.

@@ -467,6 +513,21 @@ func clone(s *machineScope) (string, error) {
return task.Reference().Value, nil
}

func getDiskSpec(s *machineScope, devices object.VirtualDeviceList) (types.BaseVirtualDeviceConfigSpec, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, should add a comment for the function to explain what is does

machineScope := &machineScope{
Context: context.TODO(),
providerSpec: &vsphereapi.VSphereMachineProviderSpec{
DiskGiB: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could possibly add this field to the testCases and try a couple of different values to make this test suite a little more thorough? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not getting this, you suggest trying different disk sizes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the disk size (and expected disk size) could come from the test case and then you could try 1, 2, 3 as values in different cases and check the calculation of the size is correct, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, more tests never hurt :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoelSpeed done

@openshift-ci-robot
Copy link
Contributor

@alexander-demichev: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 ec0e536 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-azure ec0e536 link /test e2e-azure
ci/prow/e2e-azure-operator ec0e536 link /test e2e-azure-operator

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 322089a into openshift:master Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants