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

Virtual grain in Windows #50015

Closed
basseed opened this issue Oct 12, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@basseed
Copy link
Contributor

commented Oct 12, 2018

Hi there, we check if our hosts are virtual as such in our pillars:

{% elif 'host' in grains['is_virtual'] %}

and we have a problem on Windows where a physical host won't have a grain key for virtual and jinja errors out, because in grains/core.py the windows grain defaults to an empty dictionary rather than ''physical":

grains/core.py

def _windows_virtual(osdata):
    '''
    Returns what type of virtual hardware is under the hood, kvm or physical
    '''
    # Provides:
    #   virtual
    #   virtual_subtype
    grains = dict()
    if osdata['kernel'] != 'Windows':
        return grains

def _virtual(osdata):
    '''
    Returns what type of virtual hardware is under the hood, kvm or physical
    '''
    # This is going to be a monster, if you are running a vm you can test this
    # grain with please submit patches!
    # Provides:
    #   virtual
    #   virtual_subtype
    **grains = {'virtual': 'physical'}**

We worked around that creating a custom grain 'is_virtual' and wrapping around virtual as such:

import salt.config
import salt.loader
import platform
import os

def is_virtual():
    ## we put the only necessary dummy keys for loader.py
    ## if we used an actual config file, it would try to retrieve itself and enter an endless loop
    __opts__ = {}
    __opts__['cachedir'] = 'dummy'
    __opts__['extension_modules'] = 'dummy'
    ## Load core grains only
    __grains__ = salt.loader.grains(__opts__)
    try:
        virtual = __grains__['virtual']
    except KeyError, e:
        grains = {'is_virtual': 'physical'}
        return grains
    grains = {'is_virtual': virtual}
    return grains

But shouldn't the windows "virtual" default to "pyhisical" after the os check like this?

def _windows_virtual(osdata):
    '''
    Returns what type of virtual hardware is under the hood, kvm or physical
    '''
    # Provides:
    #   virtual
    #   virtual_subtype
    grains = dict()
    if osdata['kernel'] != 'Windows':
        return grains
    grains = {'virtual': 'physical'}

We are on 2017.7.7 but I can see the code is the same in the newer versions,

thanks

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

That seems reasonable to me.

Would you mind submitting a pr to the 2017.7 branch to fix this?

https://docs.saltstack.com/en/latest/topics/development/contributing.html

Thanks!
Daniel

@gtmanfred gtmanfred added the Bug label Oct 12, 2018

@gtmanfred gtmanfred added this to the Approved milestone Oct 12, 2018

basseed added a commit to basseed/salt that referenced this issue Oct 13, 2018

Issue number saltstack#50015
On Windows a key the grain defaults and returns an empty dictionary if the machine is physical or an unrecognized vm type.
This casues errors if the "virtual" grain is used in a multi-platform environment that includes Windows.
We match the unix "virtual" grain behavior and after the OS check we default to "physical".
@basseed

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

done but not sure why it's failing the windows tests?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

That was submitted against the develop branch, not 2017.7. bug fixes should be submitted against the oldest active branch that they apply too.

cachedout pushed a commit that referenced this issue Oct 16, 2018

Mike Place
Merge pull request #50028 from basseed/fix-windows-virtual-grain
Fix #50015, Windows grain defaults to physical as unix grain

@rallytime rallytime closed this in 82fa866 Oct 23, 2018

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.