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

Fix vmware vm uuid parsing and add DeleteFrom and DetachFrom #566

Merged
merged 3 commits into from Aug 13, 2018

Conversation

harsh-px
Copy link
Contributor

@harsh-px harsh-px commented Aug 10, 2018

Signed-off-by: Harsh Desai harsh@portworx.com

What this PR does / why we need it:

  1. User doesn't need to give vsphere vmuuid from environment. It's optional.
  2. Add DeleteFrom and DetachFrom API's to detach and delete if invoked from a different node

Signed-off-by: Harsh Desai <harsh@portworx.com>
@harsh-px harsh-px self-assigned this Aug 10, 2018
Signed-off-by: Harsh Desai <harsh@portworx.com>
@harsh-px harsh-px changed the title Don't enforce vmware vm uuid in environment Fix vmware vm uuid parsing and add DeleteFrom and DeleteFrom Aug 13, 2018
@lpabon
Copy link
Member

lpabon commented Aug 13, 2018

FYI: title has add DeleteFrom and DeleteFrom

@harsh-px harsh-px changed the title Fix vmware vm uuid parsing and add DeleteFrom and DeleteFrom Fix vmware vm uuid parsing and add DeleteFrom and DetachFrom Aug 13, 2018
if err != nil {
return nil, err
}
cfg.VMUUID, _ = storageops.GetEnvValueStrict("VSPHERE_VM_UUID")
Copy link
Member

Choose a reason for hiding this comment

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

curious why this change is needed?

Copy link
Contributor Author

@harsh-px harsh-px Aug 13, 2018

Choose a reason for hiding this comment

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

VMUUID is required to be fetched from the env variableVSPHERE_VM_UUID only if one is running unit tests for dev.

When running in a real vSphere environment, Portworx (porx) will fetch this from a cloudprovider providerID field that the vmware cloud provider in k8s populates in the node object. For e.g

- apiVersion: v1
  kind: Node
  metadata:
    creationTimestamp: 2018-08-13T03:46:12Z
  spec:
    externalID: 421076c2-e509-7897-e64f-84b0bff79cbe
    providerID: vsphere://421076c2-e509-7897-e64f-84b0bff79cbe

So we don't need the env variable.

And in a non-k8s environment, this can be fetched from a /sys/fs file where vmware puts the vm uuid.

@sangleganesh
Copy link
Member

@harsh-px just for my info, is there a reason this code is a part of libopenstorage ?

@harsh-px
Copy link
Contributor Author

@harsh-px just for my info, is there a reason this code is a part of libopenstorage ?

This is the storageops interface ASG/cloud drive code in porx (AWS, GCE, vSphere) uses. For e.g https://github.com/portworx/porx/blob/master/storage/hal/provider/aws/aws_storage.go#L50 is where aws uses it.

Copy link
Contributor

@adityadani adityadani left a comment

Choose a reason for hiding this comment

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

the changes look good.
Please add a UT esp. for DetachFrom for completeness.

@sangleganesh
Copy link
Member

@harsh-px iirc the reason storage ops of AWS was put in libopenstorage was to have AWS volume driver code reuse the storage ops. gke/vmware don't have that dependency (does not have volume driver implementation for osd). is there any interface/implementation required for osd itself from either gke/vmware ?

Signed-off-by: Harsh Desai <harsh@portworx.com>
@harsh-px
Copy link
Contributor Author

Please add a UT esp. for DetachFrom for completeness.

@adityadani added

@harsh-px iirc the reason storage ops of AWS was put in libopenstorage was to have AWS volume driver code reuse the storage ops. gke/vmware don't have that dependency (does not have volume driver implementation for osd). is there any interface/implementation required for osd itself from either gke/vmware ?

Nothing in osd is using gke/vmware. Currently it's only porx. Project rico in libopenstorage was going to use all of these storage ops (https://github.com/libopenstorage/rico/tree/master/pkg/cloudprovider).

@harsh-px harsh-px merged commit 7cd3e0e into libopenstorage:master Aug 13, 2018
@harsh-px harsh-px deleted the fix-vm branch August 13, 2018 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants