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

[integration][vsphere] Add optional vm include parameter #2459

Merged
merged 2 commits into from May 18, 2016

Conversation

JohnLZeller
Copy link
Contributor

This adds the ability to set a custom field from the vSphere PowerCLI which acts as a marker for which VMs you would like to have monitored by the agent. In order to add this custom field, a user can use this command: Get-VM <MyVMName> | Set-CustomField -Name "DatadogMonitored" -Value "DatadogMonitored"

The ideal would have been to use the new tagging feature brought in vSphere 6.0 (usable also through the vSphere Web Client), but the vSphere API (at least in vCenter 5.5) does not expose these tags. There is an attribute on the managed object for VirtualMachine, called tag. Unfortunately, this is actually not the same tag set as the ones set in the vSphere Web Client (at least in vCenter 5.5). The tags set in the web client are instead stored in the Inventory Service.

Additionally, Custom Attributes could have been an option, which can be set via PowerCLI or Web Client (I think), but Custom Attributes were deprecated in exchange for Tags in vSphere 6.0.

Alas, this fix gives the ability for all. When vSphere releases another update that makes it easy to access the tags set on VirtualMachine managed objects, then we can add a feature to use tags.

@JohnLZeller
Copy link
Contributor Author

I've tested this. I had the agent running with include_only_marked: false, then I set it to true, and then I added the custom field to the japan vm. As you can see from this screenshot, the metrics coming in, reported correctly. I've included a host in this screenshot as proof that the host itself didn't also stop reporting.

Note: Staging and japan are the VMs.

screen shot 2016-05-02 at 6 18 53 pm

@JohnLZeller JohnLZeller added this to the 5.9.0 milestone May 2, 2016
@@ -633,9 +649,10 @@ def _cache_morlist_raw(self, instance):
'host_include': instance.get('host_include_only_regex'),
'vm_include': instance.get('vm_include_only_regex')
}
include_only_marked = instance.get('include_only_marked')
Copy link
Member

Choose a reason for hiding this comment

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

Should probably default to False:
include_only_marked = instance.get('include_only_marked', False)

we should also probably also use is_affirmative() so the user can be a little more loose in the way they specify if the flag is enabled.

@truthbk
Copy link
Member

truthbk commented May 3, 2016

Looks good other than the nitpick.

@JohnLZeller
Copy link
Contributor Author

@truthbk thanks for the note! I took your suggestions, and made the changes. How's it look now?

@@ -602,6 +607,18 @@ def _cache_morlist_raw_atomic(self, i_key, obj_type, obj, tags, regexes=None):
if not match:
self.log.debug(u"Filtered out VM {0} because of vm_include_only_regex".format(obj.name))
return
# Also, if include_only_marked is true, then check if there exists a
# custom field with the value DatadogMonitored
if _is_affirmative(include_only_marked):
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, but feel free to ignore: I'd probably put is_affirmative() here:

include_only_marked = _is_affrmative(instance.get('include_only_marked', False)) on line 653

@truthbk
Copy link
Member

truthbk commented May 18, 2016

👍 squash and merge as soon as CI passes! Thanks!

@JohnLZeller JohnLZeller merged commit a8a0416 into master May 18, 2016
@JohnLZeller JohnLZeller deleted the zeller/vsphere-tags branch May 18, 2016 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants