add fleet_facts module which gathers information about CoreOS clusters #928

Closed
wants to merge 2 commits into
from

Projects

None yet

9 participants

@coderfi
coderfi commented Sep 7, 2015

No description provided.

@coderfi
coderfi commented Sep 7, 2015

This pull obsoletes #91
Per discussion, I have split facts gathering into its own module.

Also see related PR: #929

@robynbergeron
Contributor

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@robynbergeron
Contributor

@jeanmertz @jalev @ahjohannessen @veverjak @johnraz @moperacz -- Some of you had feedback for this in the original PR, #91 -- please note that with the new and improved extras PR review process (detailed in the previous comment), your help in reviewing this can help get it in more quickly :)

Also -- hey, @brianredbeard or @kelseyhightower -- do you guys know anyone who might be a good candidate for reviewing this? :) If so, can you nudge them this way? Thanks! :)

@gregdek gregdek removed the core_review label Oct 5, 2015
@brianredbeard

Hey there,

So looking through this there are a few questions. So i'm not sure how well the python-fleet module is maintained. Also, I don't see a readme as a part of the PR (and thus it's unclear how much this is inheriting from python-fleet) but I also don't see a reference to the file socket, only a TCP socket. Does this require the TCP socket to be listening (by default it does not)?:

core@redbeard-2gb-3-6 ~ $ systemctl  cat fleet.socket 
# /usr/lib64/systemd/system/fleet.socket
[Unit]
Description=Fleet API Socket
PartOf=fleet.service

[Socket]
ListenStream=/var/run/fleet.sock

I'd recommend @jonboulle or @mischief for further commentary.

@jonboulle

my Python and Ansible is rusty, but this looks reasonable to me.
I might have a concern re: performance - how frequently would it be expected that this would run?

@robynbergeron
Contributor

Hi @brianredbeard :D and @jonboulle -- thanks for your comments / questions :)

@coderfi -- can you address the comments / questions above? Thanks!

And a friendly note to anyone else passing by: This new module is open for reviews to have it included in Extras, as explained above in this comment -- #928 (comment) -- so feel free to do so :)

@coderfi
coderfi commented Jan 16, 2016

Oh wow, I have not checked up on this PR for so long, sorry about that.

@brianredbeard I'm also not sure about how well python-fleet is maintained, but it's immortalized on pypi and the functionality was sufficient for this PR, so that's good enough for me. My original PR had the code doing some stdout scraping to achieve the same results. Although very portable, the original reason was because it was discouraged to have external library dependencies, but that was before we had ansible-module-extras.
Besides, something like this wouldn't need to change often.

At the time, I believe python-fleet only supported fleet TCP sockets. However, in the past few months, it looks like they now support ssh tunneling and sockets, though I would think there would be difficulty on exposing a file socket, given that this library would be running on a remote, administrative machine (that has python and ansible, among other things).

It looks like a small update to the argument spec will allow specifying the ssh tunnel, which would be better from a security standpoint. Would you like me to update?
Also, not sure about providing a README, the other modules in ansible-modules-extras didn't typically have a standalone README. However, I can clarify a few things in the module documentation.

@jonboulle Thank you for reviewing my (simple) code. How often it runs depends entirely on the use case! In either case, the inner workings just issues an RPC over the fleet socket, and CoreOS/Fleet has this information readily available. Other than the latency between the admin server and the host, this should be basically as fast as any other gather facts call.

@cnelson
cnelson commented Jan 16, 2016

python-fleet is maintained. The only thing it's currently missing is unix socket forwarding (on ssh >= 6.7) because paramiko doesn't support it (paramiko/paramiko#544).

@coderfi
coderfi commented Jan 16, 2016

Yes, I noticed your updates in the recent months, great library, btw! It simplified my stdout scraping hacks significantly. :)

@brianredbeard I think waiting for unix socket forwarding is really just scope creep. I see no reason to delay this PR because of that. We can simply make another PR to include that once unblocked by paramiko.

@gregdek
Contributor
gregdek commented Mar 21, 2016

@coderfi this fails Travis; can you check that out? Feel free to put into ready_for_review when you've done so.

@sivel
Member
sivel commented Mar 21, 2016

Some additional issues that need to be resolved:

============================================================================
fleet_facts.py
============================================================================
ERROR: version_added should be 2.1. Currently 2.0
ERROR: No RETURN documentation provided
@coderfi
coderfi commented Mar 24, 2016

Thank you for your comments. Will try to get back to this as soon as I can.

@gregdek
Contributor
gregdek commented Apr 6, 2016

@coderfi A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes.

[This message brought to you by your friendly Ansibull-bot.]

@coderfi
coderfi commented Apr 9, 2016

Similar to my other Pull #929 , seems to be a build error on a file that doesn't exist in my fork.

My branch is thousands of commits behind the upstream. Maybe syncing it up will fix this build failure?

@gregdek
Contributor
gregdek commented Apr 21, 2016

@coderfi Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@coderfi
coderfi commented Apr 24, 2016

Fro #929, I apparently need to do a rebase. Unfortunately, doesn't seem to be anything 'easy' I could do since it's been months since I forked the repo. Any rebase I try is met with numerous conflicts.
Unless some GIT Ninja can recommend otherwise, I'll just fork the latest devel and submit a new pull request on that.

@bcoca
Member
bcoca commented Apr 25, 2016

I recommend to checkout a new devel and cherry pick your changes, but new clone should also work.

In both cases, you can force push to the same github branch and it will update this PR.

@gregdek
Contributor
gregdek commented May 7, 2016

@coderfi A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek
Contributor
gregdek commented May 22, 2016

@coderfi Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@coderfi coderfi closed this May 30, 2016
@coderfi coderfi deleted the coderfi:fleet branch May 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment