opennebula inventory: add VM id and VM host to data

[aioue]

SUMMARY

To enable greater use of the inventory, add the ID of the VM, and the hostname of the host the VM is running on to the inventory output

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

opennebula.py

ADDITIONAL INFORMATION

                "host": "foo23.host",
                "id": 1234,

[ansibullbot]

cc @feldsam
click here for bot help

[aioue]

Not sure why the tests are failing as I'm not adding a new import for the pyone library...?

[feldsam]

Hi, problem is, that you added new method which is calling pyone package. To pass test, you need to also update tests to reflect this.

Anyway, for such infromation - host on which is VM running, there is easier method to get this. It is already in VM object, in history records, you just need to get last history record of VM to get host. So no need to do aditional api call and searching for host.

[aioue]

@feldsam updated PR to reflect the new information you provided. It's a very simple PR now!

I'm afraid I don't understand how to fix the tests.

[felixfontein]

This data structure looks very different from what the code is actually using. Here HISTORY_RECORDS is a list of dictionaries with a key HISTORY in them, and that in turn gives a dictionary with a key HOSTNAME in it.

The code expects HISTORY_RECORDS to be an object with an attribute HISTORY, which is a list, and contains objects that have an attribute called HOSTNAME.

[aioue]

I'm afraid I'm completely lost, this is way beyond my ability. Hoping the original author @feldsam can help.

[russoz]

Maybe something like:

Suggested change

	'HISTORY_RECORDS': [
	{
	'HISTORY': OrderedDict({
	'OID': '42',
	'SEQ': '384',
	'HOSTNAME': 'sam-691-sam',
	'HID': '10',
	'CID': '0',
	'DS_ID': '100',
	'VM_MAD': 'kvm',
	'TM_MAD': '3par',
	'ACTION': '0'
	}),
	}
	],
	'HISTORY_RECORDS': {
	'HISTORY' : [
	{
	'OID': '42',
	'SEQ': '384',
	'HOSTNAME': 'sam-691-sam',
	'HID': '10',
	'CID': '0',
	'DS_ID': '100',
	'VM_MAD': 'kvm',
	'TM_MAD': '3par',
	'ACTION': '0',
	},
	],
	},

By the way, this construct OrderedDict({ ... }) that appears throughout this test code is likely redundant, they could all be replaced with the dict content {...}.

[aioue]

Thanks @russoz. Since my opennebula.py inventory change does work when run against OpenNebula servers, I am reasonably sure that changing HISTORY_RECORDS back to a {} rather than a [] will break the test, which means it doesn't reflect the actual data structure that ON returns.

If the code is working and the tests are currently passing, is this PR 'good enough' now?

I'm quite burnt out on it and don't want to make myself dislike adding features as I'll be using Open Nebula for the next year plus!

[russoz]

Thanks @russoz. Since my opennebula.py inventory change does work when run against OpenNebula servers, I am reasonably sure that changing HISTORY_RECORDS back to a {} rather than a [] will break the test, which means it doesn't reflect the actual data structure that ON returns.

If the code is working and the tests are currently passing, is this PR 'good enough' now?

Well, the concern is that the piece of code that seems to parse that data goes like this:

            if hasattr(vm.HISTORY_RECORDS, 'HISTORY'):
                if isinstance(vm.HISTORY_RECORDS.HISTORY, Sequence) and len(vm.HISTORY_RECORDS.HISTORY) > 0:
                    if hasattr(vm.HISTORY_RECORDS.HISTORY[-1], 'HOSTNAME'):
                        server['host'] = vm.HISTORY_RECORDS.HISTORY[-1].HOSTNAME

That seems to indicate that HISTORY_RECORDS has an attribute HISTORY, which in its turn is a Sequence (with at least one element), from which you select the last element, and the attribute HOSTNAME out of it. That looks not coherent with the statement that HISTORY_RECORDS is a list. Could you please explain why that difference?

People other than you (me, Felix, @feldsam and future maintainers of the collection) will need to provide support to this code for potentially a long time and we cannot explain it. Either we are missing some piece of the puzzle, or the test might doing the wrong thing.

So, answering your question, to be 'good enough' we need to understand what's happening. Hope you don't take this badly, but the long-term health of the code is more important than short-term results.

I'm quite burnt out on it and don't want to make myself dislike adding features as I'll be using Open Nebula for the next year plus!

Sorry to hear that, hope you can find some time to decompress. There is no pressure from our side to solve this quickly, so take your time.

All the best.

[felixfontein]

Something like that should produce the data structure that the inventory plugin actually expects:

class HistoryEntry(object):
    def __init__(self):
        self.SEQ = '384'
        self.HOSTNAME = 'sam-691-sam'
        self.HID = '10'
        self.CID = '0'
        self.DS_ID = '100'
        self.VM_MAD = 'kvm'
        self.TM_MAD = '3par'
        self.ACTION = '0'

class HistoryRecords(object):
    def __init__(self):
        self.HISTORY = [HistoryEntry()]

def get_vm_pool():
    ...
        'HISTORY_RECORDS': HistoryRecords(),
    ...