Skip to content
This repository has been archived by the owner on Mar 2, 2019. It is now read-only.

Include metadata as part of create_vm and create_disk #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nehagjain15
Copy link

No description provided.

@cfdreddbot
Copy link

Hey nehagjain15!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@voelzmo
Copy link
Contributor

voelzmo commented Oct 18, 2018

I'm not sure I fully understand the motivation behind this. So far, we have ensured that after creating a VM and after creating a disk the Director calls the method to set the metadata. This sets all IaaS specific tags on the infrastructures which support it.

  • Using metadata to determine the VM name is already possible now, the AWS and OpenStack CPI are for example doing this already. The discussion you linked with Issue #1163 asks for users to be able to define their own template to determine how the VM name should be composed. This seems somewhat orthogonal to this proposal to me.
  • I have no idea about vCenter. It seems your proposal is influenced by the specific needs for the vSphere CPI. Could you elaborate more about the concrete use-cases that you want to enable? Did I mis-interpret this proposal and you have other use-cases for other IaaSes as well?

@yeshwantbabar
Copy link

Hi @voelzmo -

Sorry about the delay on getting back to you.

As Neha mentioned there are similar requests for the other IaaSes as well.

What we are looking for is the ability to accept a prefix/suffix strings that can be specified in the VMTypes / Deployment manifest so that all the VMs that belong to a particular Instance Group get a certain prefix or suffix added to their names. There are several usecases for these -

  1. Providing some context for what the VM does for admins.
  2. Abiding by the existing standards for what the VM names should look like.
  3. Being able create folder structure based on deployments with the folder hierarchy having the name that's human readable.
  4. These names can be plugged into the automation frameworks like powercli.

Please let me know your thoughts.

cc: @mfine30

@dpb587-pivotal
Copy link
Contributor

@friegger @beyhan @voelzmo - forgot to mention this earlier today. Do you have any objections around this potential CPI change to provide VM metadata as part of the initial create_vm call? This came up again in some recent discussions.

A related issue was raised in the AWS CPI as an example of how it could be used in other IaaSes. cloudfoundry/bosh-aws-cpi-release#93 refers to locking down EC2 operations based on tags (and those tags would need to be present on the initial create_vm/RunInstances call).

@voelzmo mentions that cloudfoundry/bosh#1163 is orthogonal, which I'd mostly agree. I think there's the caveat that some IaaSes might not support renaming things after the resource is created, and this would help that situation. I think this is the case in vCenter for VM names where they can't be changed after creation. (Whether or not that property should be a director-level property is a different topic)

Anyways, I'd rather consider implementing this change before CPIs start relying on the groups field of the API which is not an officially ordered list of metadata. That field could technically change in the future, and I don't want director to be tightly tied to keeping the exact values.

@nehagjain15
Copy link
Author

@voelzmo if we get metadata as part of create_vm call we could

  1. Name VM's appropriately instead of using uuid
  2. We could also create folder based structure and group VM's based on deployment_name/ instance_group name.

@beyhan
Copy link

beyhan commented Feb 6, 2019

I don't have any. I would like to have VM creation and tagging possible with one call on BOSH side. CPIs could decide whether they want/can support this or not.

@voelzmo
Copy link
Contributor

voelzmo commented Feb 6, 2019

@dpb587-pivotal:

Do you have any objections around this potential CPI change to provide VM metadata as part of the initial create_vm call?

Sounds good to me. OpenStack does allow for setting metadata on create_vm and create_volume, so this should also reduce calls on the OpenStack CPI side.

With the questions above I just wanted to understand where the request is coming from: Which problem are we solving and for whom? I'm perfectly fine with helping vSphere pick better names for VMs and folders and also help lock down security on AWS. This would most likely also help with better VM names for GCP: cloudfoundry/bosh-google-cpi-release#217 (comment). Thanks for following up on this and helping me understand!

proposals/include-metadata.md Outdated Show resolved Hide resolved

# Details

Metadata will include basic information about director, deployment and instance_group. E.g
Copy link
Contributor

Choose a reason for hiding this comment

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

And also all user-defined tags from manifest and runtime config? Or will those still be set with a later call to set_vm_metadata?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect create_vm to receive any tags that the immediate followup set_vm_metadata call would send.


# Unresolved questions

What other information can be added as part of this set?
Copy link
Contributor

Choose a reason for hiding this comment

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

Potention other question: The director currently calls set_vm_metadata and set_disk_metadata. How does the director determine to not call these anymore? Based on CPI API version returned by the info method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it bad to still call them? It'd effectively be a no-op. One thing to keep in mind is that it's possible for some metadata to change, and I think CPIs should still support that. It does call out that we should probably have a recommendation on which metadata keys are used in resource-permanent fields that cannot be changed later (to avoid confusing users if the values change).

For example, compilation VMs will have their metadata updated to reflect what they're currently compiling (in the case of reuse_compilation_vms).

I don't think disk metadata is currently updated at any point, but I think we should support it. For example, I think disk metadata can get out of sync with manual attach-disk calls; might be something director wants to fix/change at some point.

@nehagjain15
Copy link
Author

nehagjain15 commented Feb 13, 2019

@voelzmo & @dpb587-pivotal I updated the proposal based on your conversation. What would be the next steps to get this added to tracker so that the change can be implemented?


# Unresolved questions

How should set_vm_metadata and set_disk_metadat methods be updated?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think they should (or need) to be updated. I'd expect them to remain the same and be called in the same places as they currently are. I think this is cheapest to support both older/newer CPIs and avoid director dealing with complex versioning behaviors, all with minimal impact to the actual IaaS calls. Happy to talk more about that if anyone has concerns or objections with that approach.

I think the general recommendation for CPIs should be "avoid making unnecessary metadata changes" to help cover scenarios where its going to be a no-op in the traditional create_vm + set_vm_metadata case. Although there's probably not much difference between querying for current tags and sending changed ones vs sending the full list of tags to the IaaS every time.

@dpb587-pivotal
Copy link
Contributor

Reposting from internal Slack where I didn't hear any response...

From my perspective, I think there's enough of an understanding of what changes are necessary and desired for it, and I think it makes sense. Conceptually, I think the two pieces are:

  1. implement on director for create_vm, create_disk, create_snapshots; the API looks like it'll be backwards compatible for at least ruby+go CPIs (but we'll want to verify that during implementation).
  2. individual CPIs could then implement it. Depending on who is implementing the director changes, the AWS/vSphere would probably be implemented alongside it for testing.

@nehagjain15 @yeshwantbabar, I know you're interested in the vSphere CPI support, at least. Are you also interested in implementing the director-side stuff as well? Otherwise, I'd recommend you and @mfine30 chat more about where it'd fall in prioritization between your teams, here or in your next sync.

@voelzmo
Copy link
Contributor

voelzmo commented Feb 15, 2019

@dpb587-pivotal I'd still be interested in a concept on how to avoid unnecessary calls from director to CPI (and therefore to the IaaS API). This would also help with other issues e.g. rate limiting on AWS.

@dpb587-pivotal
Copy link
Contributor

@voelzmo seems reasonable. Want to document your thoughts here, or discuss it elsewhere? And do you want it to be a blocker on any initial release of CPI+director for this functionality, or would you consider it something that can be improved on later?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants