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

Add engine relaying libvirt events #46461

Merged
merged 3 commits into from Apr 5, 2018
Merged

Add engine relaying libvirt events #46461

merged 3 commits into from Apr 5, 2018

Conversation

@cbosdo
Copy link
Collaborator

@cbosdo cbosdo commented Mar 9, 2018

What does this PR do?

Libvirt API offers clients to register callbacks for various events.
libvirt_events engine will listen on a libvirt URI (local or remote)
for events and send them to the salt event bus.

What issues does this PR fix or reference?

Nothing that is in the issue tracker to my knowledge

Tests written?

No: those would require a running libvirt daemon
Yes

Commits signed with GPG?

Yes

@rallytime rallytime requested a review from garethgreenaway Mar 9, 2018
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 12, 2018

I can't change the callback signature: codeclimate would need to be silenced for those functions that have more than 4 arguments.

I don't really know what to do with the docstrings of the callbacks: either pylint is complaining not having them or code climate complains because their are too similar. Honestly, considering how useless those would be, I'ld just let pylint complain.

Codeclimate complains that the file is too long... but is it possible at all to have the code of an engine split into several files? Ideally I would separate the callbacks from the rest of the engine, not sure if that is possible / wanted.

Copy link
Contributor

@isbm isbm left a comment

Can we also rename callbacks from dom_<something> to domain_...? Because this is confusing with DOM (document object model). The callbacks can be also "hidden", i.e. starts from the underscore: def _something.

('pmsuspended', ("memory",
"disk")),
('crashed', ("panicked"))
)
Copy link
Contributor

@isbm isbm Mar 12, 2018

I would refactor all this into a dictionary. The def nth is not needed anywhere, because the dictionary has .get(...) function.

Copy link
Collaborator Author

@cbosdo cbosdo Mar 13, 2018

Found a way to get rid of these lists by introspecting libvirt module.

@isbm
Copy link
Contributor

@isbm isbm commented Mar 12, 2018

@cbosdo Unit tests do not need to have libvirt installed, only integration tests. So you can just mock.MagicMock all the output from the libvirt part and unit-test everything, since we do not really need to test the libvirt itself.

@cbosdo cbosdo force-pushed the develop branch 2 times, most recently from 7c1524c to 1b0daf9 Mar 13, 2018

libvirt_events.start(uri, prefix)

assert call.call_list() != []
Copy link
Contributor

@isbm isbm Mar 14, 2018

@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 14, 2018

unit tests added now

@cbosdo cbosdo force-pushed the develop branch 2 times, most recently from ba50b0e to 7ea9446 Mar 14, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 15, 2018

@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 15, 2018

Fixed the pylint errors now

Copy link
Contributor

@rallytime rallytime left a comment

@cbosdo I have a couple of comments here that need to be addressed. Thank you so much for writing the tests, too!

@@ -0,0 +1,617 @@
# coding=utf-8
Copy link
Contributor

@rallytime rallytime Mar 15, 2018

I know this is picky, but can you make this encoding line the standard version?

# -*- coding: utf-8 -*-

# coding=utf-8

'''
An engine that listens for libvirt events and resend them to the salt event bus.
Copy link
Contributor

@rallytime rallytime Mar 15, 2018

resend --> resends

Copy link
Contributor

@rallytime rallytime Mar 15, 2018

Can you also add a version directive to this?

.. versionadded:: Fluorine


# pylint: disable=invalid-name

msg = ''
Copy link
Contributor

@rallytime rallytime Mar 15, 2018

This type of msg handling and version checking should be done in the __virtual__() function.

Copy link
Contributor

@isbm isbm Mar 16, 2018

@cbosdo actually @rallytime is right. So just move checks to the __virtual__ itself:

try:
    import libvirt
except ImportError:
    libvirt = None

def __virtual__():
    '''
    Only load if libvirt python binding is present
    '''
    if libvirt is None:
        msg = 'libvirt module not found'
    elif libvirt.getVersion() < 1000000:
        msg = 'libvirt >= 1.0.0 required'
    else:
        msg = ''

    return bool(libvirt), msg

OK, basically copypaste this. 😉

Copy link
Collaborator Author

@cbosdo cbosdo Mar 16, 2018

This makes pylint complain on the if libvirt is None: line:
[used-before-assignment] Using variable 'libvirt' before assignment

And on the libvirt = None line that you didn't move:
[redefined-outer-name] Redefining name 'libvirt' from outer scope

Copy link
Contributor

@isbm isbm Mar 16, 2018

If checks has to be in __virtual__, then just mute Lint in this case.

'''
Convenience function adding common data to the event and sending it
on the salt event bus.
'''
Copy link
Contributor

@rallytime rallytime Mar 15, 2018

Can you document all of the arguments here? And provide an example of how they should be used?

def salt_send_domain_event(opaque, conn, domain, event, event_data):
'''
Helper function send a salt event for a libvirt domain
'''
Copy link
Contributor

@rallytime rallytime Mar 15, 2018

Same comment here about docs.

Copy link
Contributor

@rallytime rallytime Mar 15, 2018

Rather than commenting in many places repeatedly, I'll just say this here: Any public facing function needs docs for all args/kwargs and some examples of usage. If you could update all of those below, that would be great.

Copy link
Collaborator Author

@cbosdo cbosdo Mar 16, 2018

All these helper functions aren't intended to be used by external code, I'ld rather hide them, but adding those comments can't harm

Copy link
Contributor

@isbm isbm Mar 16, 2018

@cbosdo even hidden usually should at least document what it does. But yes, I would underscore them in front, since they are all internal.

@cbosdo cbosdo force-pushed the develop branch 7 times, most recently from fb4ec0b to 53cb886 Mar 22, 2018
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 28, 2018

any update on that PR?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 28, 2018

Libvirt API offers clients to register callbacks for various events.
libvirt_events engine will listen on a libvirt URI (local or remote)
for events and send them to the salt event bus.

Special thanks to @isbm for the code cleanup help
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 28, 2018

@rallytime right! fixed all the pylint errors now.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 28, 2018

@cbosdo Awesome! Thanks! We're working on getting some testing issues resolved here, so we'll restart them when that's resolved.

In the mean time, @garethgreenaway can you swing by here and review this when you get a moment?

@garethgreenaway
Copy link
Member

@garethgreenaway garethgreenaway commented Mar 28, 2018

Looks good!

@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Mar 28, 2018

hum... looks like the branch would need a rebase before merging... should I do it now that it has been approved or you can still do it at merge time?

@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Apr 3, 2018

@rallytime is it me or the changes you requested (which have been implemented by the time) are blocking this PR?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Apr 4, 2018

re-run py2

@rallytime
Copy link
Contributor

@rallytime rallytime commented Apr 4, 2018

@cbosdo Apologies for the delay. Some of the test results timed out so I am re-running some. Once those come back we can get this in. Thanks for the bump!

isbm
isbm approved these changes Apr 5, 2018
@rallytime rallytime merged commit 9ab8c26 into saltstack:develop Apr 5, 2018
5 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants