[vsphere] check revamp addressing several issues #3055
Conversation
|
|
||
| server_instance = self._get_server_instance(instance) | ||
| root_folder = server_instance.content.rootFolder | ||
| for resource_type in RESOURCE_TYPE_MAP: |
There was a problem hiding this comment.
Why can't we keep
if i_key in self.morlist_raw and len(self.morlist_raw[i_key]) > 0:
?
Vsphere objects in self.morlist_raw[i_key][resource_type] are populated at the same time as the resource keys in self.morlist_raw[i_key].
There was a problem hiding this comment.
Well, I did that because now we pop() the elements per resource type in _cache_morlist_process(), before we knew when the list was empty, we were done. Now we may have processed all ComputeResources, or Datacenters, but not the VM's or whatever, so just checking for the length of the dictionary wouldn't be enough. Besides, we pop from the lists, not the keys. self.morlist_raw[i_key][resource_type] dicts are created the first time around, but they then exist through-out, I believe, so the len(self.morlist_raw[i_key]) == len(RESOURCE_TYPE_MAP) in subsequent iterations.
| u"job_atomic: Exploring MOR %s: name=%s, class=%s", | ||
| obj, obj.name, obj.__class__ | ||
| ) | ||
| def _get_all_objs(content, vimtype, regexes=None, include_only_marked=False): |
There was a problem hiding this comment.
Do you think we should move _get_all_objs and build_resource_registry out of _discover_mor body?
| @@ -582,15 +584,19 @@ def _discover_mor(self, instance, tags, regexes=None, include_only_marked=False) | |||
| If it's a node we want to query metric for, queue it in `self.morlist_raw` that | |||
| will be processed by another job. | |||
| """ | |||
| def _get_all_objs(content, vimtype, regexes=None, include_only_marked=False): | |||
| def _get_all_objs(content, vimtype, regexes=None, include_only_marked=False, tags=[]): | |||
There was a problem hiding this comment.
Let's avoid the mutable tags=[] as a default argument. It's easy to forget it and modify the value later on.
| for c in container.view: | ||
| if not self._is_excluded(c, regexes, include_only_marked): | ||
| obj_list.append(c) | ||
| obj_list.append(dict(mor_type=vimtype, mor=c, hostname=c.name, tags=tags)) |
There was a problem hiding this comment.
Nitpick: NamedTuple for that. It's easier to use as we know that all keys exist, i.e.
vsphere_object.mtype
instead of
vsphere_object['mtype']
which is more ambiguous and can quickly become
vsphere_object.get('mtype')
|
|
||
| mor['metrics'] = self._compute_needed_metrics(instance, available_metrics) | ||
| mor_name = str(mor['mor']) | ||
| if available_metrics: |
There was a problem hiding this comment.
What happens if we don't check that? We were not doing it before.
There was a problem hiding this comment.
You're right, we probably don't need it, going to try with passing around the empty list.
| interval = REAL_TIME_INTERVAL | ||
| else: | ||
| interval = None | ||
|
|
There was a problem hiding this comment.
Nitpick: we have some small code duplication, I wonder if it'd be nicer to have the interval stored once for all in mor, i.e.
dict(mor_type=vimtype, mor=c, hostname=c.name, tags=tags, interval=interval)
| "vsphere.%s" % metric_name, | ||
| value, | ||
| hostname=mor['hostname'], | ||
| tags=['instance:%s' % instance_name] | ||
| tags=['instance:%s' % instance_name] + mor['tags'] |
There was a problem hiding this comment.
I don't think this is necessary. more['tags'] should be host tags, not metric tags.
What does this PR do?
This PR aims to address several issues with the check. Mostly focusing on the following points:
Motivation
A general need to improve our vsphere check to better address customer requirements.