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

New grains fetching detailed hypervisor info for Xen domains #46459

Merged
merged 8 commits into from May 29, 2018

Conversation

Projects
None yet
7 participants
@ixs
Copy link
Contributor

commented Mar 9, 2018

What does this PR do?

Add new grains to parse hypervisor details

New Behavior

Xen exposes a lot of detail about the hypervisor in /sys/hypervisor.

Parse the hypervisor version and the capabilities and expose these
as grains.

This allows for detailed targeting and appropriate actions, e.g. when
running on XenServer install the XenServer guest-tools.

Sample output:

virtual_hv_features:
    000020f0
virtual_hv_features_list:
    - pae_pgdir_above_4gb
    - mmu_pt_update_preserve_ad
    - gnttab_map_avail_bits
    - memory_op_vnode_supported
virtual_hv_version:
    4.6.6-2.el7
virtual_hv_version_info:
    - 4
    - 6
    - .6-2.el7

Tests written?

No. It looked like the whole _virtual part has no tests at all.

Commits signed with GPG?

No

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

Should these grains be documented? Is there any page/doco describing grains?

@ixs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

@damon-atkins I do not believe there is any documentation.

If you look at https://docs.saltstack.com/en/latest/topics/grains/, it links straight to https://github.com/saltstack/salt/blob/develop/salt/grains/core.py for "documentation". And otherwise it suggests to use the grains module to discover what is available.

@rallytime rallytime requested a review from garethgreenaway Mar 12, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

@ixs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

@rallytime done.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

@ixs Looks like there are still some errors. You can run the lint check yourself if you like by following these instructions.

@rallytime
Copy link
Contributor

left a comment

I have a couple of requested changes here, too.

try:
version = {}
for file in ('major', 'minor', 'extra'):
with salt.utils.fopen('/sys/hypervisor/version/{}'.format(file), 'r') as fhr:

This comment has been minimized.

Copy link
@rallytime

rallytime Mar 15, 2018

Contributor

Can you use the salt.utils.files.fopen path here, instead of the salt.utils.fopen? We've split out the utils init file into separate files in 2018.3 and develop.

This comment has been minimized.

Copy link
@ixs

ixs Mar 17, 2018

Author Contributor

Right. In that case you might also want to update your linter messages as that referes to salt.utils.fopen()

# Try to get the exact hypervisor version from sysfs
try:
version = {}
for file in ('major', 'minor', 'extra'):

This comment has been minimized.

Copy link
@rallytime

rallytime Mar 15, 2018

Contributor

file is a reserved keyword in Python. Can you change this to another variable name?

This comment has been minimized.

Copy link
@ixs

ixs Mar 17, 2018

Author Contributor

Done.

13: 'memory_op_vnode_supported',
14: 'ARM_SMCCC_supported'}
try:
with salt.utils.fopen('/sys/hypervisor/properties/features', 'r') as fhr:

This comment has been minimized.

Copy link
@rallytime

rallytime Mar 15, 2018

Contributor

Same comment here about the utils path. :)

This comment has been minimized.

Copy link
@ixs

ixs Mar 17, 2018

Author Contributor

Done

ixs and others added some commits Mar 9, 2018

New grains fetching detailed hypervisor info for Xen domains
Xen exposes a lot of detail about the hypervisor in /sys/hypervisor.

Parse the hypervisor version and the capabilities and expose these
as grains.

This allows for detailed targeting and appropriate actions, e.g. when
running on XenServer install the XenServer guest-tools.

Sample output:

    virtual_hv_features:
        000020f0
    virtual_hv_features_list:
        - pae_pgdir_above_4gb
        - mmu_pt_update_preserve_ad
        - gnttab_map_avail_bits
        - memory_op_vnode_supported
    virtual_hv_version:
        4.6.6-2.el7
    virtual_hv_version_info:
        - 4
        - 6
        - .6-2.el7

@ixs ixs requested a review from saltstack/team-core as a code owner Mar 17, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-cloud Mar 17, 2018

version[fn] = fhr.read().strip()
grains['virtual_hv_version'] = '{}.{}{}'.format(version['major'], version['minor'], version['extra'])
grains['virtual_hv_version_info'] = (version['major'], version['minor'], version['extra'])
except:

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

Bare excepts are bad, at minimum except Exception should be used. But when you know the potenial exceptions, you should use them. In this case, except (IOError, OSError, KeyError):

with salt.utils.files.fopen('/sys/hypervisor/version/{}'.format(fn), 'r') as fhr:
version[fn] = fhr.read().strip()
grains['virtual_hv_version'] = '{}.{}{}'.format(version['major'], version['minor'], version['extra'])
grains['virtual_hv_version_info'] = (version['major'], version['minor'], version['extra'])

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

We should be using a list instead of a tuple here, for a couple reasons:

  1. The tuple will be lost (converted to list) anyway if/when the data is serialized using msgpack, json, etc.
  2. We allow grains to be added via YAML, which does not support tuples.
version = {}
for fn in ('major', 'minor', 'extra'):
with salt.utils.files.fopen('/sys/hypervisor/version/{}'.format(fn), 'r') as fhr:
version[fn] = fhr.read().strip()

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

This needs to be decoded to unicode using:

version[fn] = salt.utils.stringutils.to_unicode(fhr.read().strip())

This may also require that salt.utils.stringutils is added to the imports.

14: 'ARM_SMCCC_supported'}
try:
with salt.utils.files.fopen('/sys/hypervisor/properties/features', 'r') as fhr:
features = fhr.read().strip()

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

Same as above with decoding to unicode.

with salt.utils.files.fopen('/sys/hypervisor/properties/features', 'r') as fhr:
features = fhr.read().strip()
enabled_features = []
for bit, feat in xen_feature_table.items():

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

Instead of xen_feature_table.items(), please use six.iteritems(xen_feature_table). This ensures that Python 2 uses a generator over the key/value pairs.

enabled_features.append(feat)
grains['virtual_hv_features'] = features
grains['virtual_hv_features_list'] = enabled_features
except:

This comment has been minimized.

Copy link
@terminalmage

terminalmage Mar 20, 2018

Contributor

Same as above with the bare exception. Try to narrow this down to the exceptions you'd expect to be raised.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2018

@ixs Are you planning to implement the review feedback on this?

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2018

@ixs We did not hear back from you on the review feedback so we are going to close this. When you're ready to come back to it, please leave a comment here and we will happily re-open it. Thanks!

@cachedout cachedout closed this Apr 9, 2018

@ixs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

@cachedout thanks for the review and the suggestions. Finally gotten around to finish this. Please reopen.

@rallytime rallytime reopened this Apr 17, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

@ixs I have re-opened this. Looking forward to seeing your updates. :)

@ixs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

@rallytime Thanks. The new update is already up on github and I believe this PR has the latest changes with all requested work done.

@rallytime rallytime requested a review from terminalmage Apr 17, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

Hi @ixs - This change is causing some test failures in the test_core.py file. Can you take a look?

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-cent7-py3/4121/#showFailuresLink

Handle the case the osdata dict doesn't contain any virtual key.
This would happen if someone runs on bare metal, an admittedly
increasingly rare situation...
@ixs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@rallytime Ohh, that's new. Fixed.
Turns out, the virtual grain is of course not set when running not in a virtual environment... But who would ever do such a thing, right?

@rallytime rallytime merged commit 5e13488 into saltstack:develop May 29, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5300 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10271 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19356 — ABORTED
Details
codeclimate 4 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23234 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25490 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17574 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22204 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.