-
Notifications
You must be signed in to change notification settings - Fork 199
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
Ansible 2.10 + Collections support #715
Conversation
…modulemixin, need to tweak how this works
…ugh because of empty submodules
Hmm. Fascinating.
In ansible/ansible#52194 there was a line that prefixed modules with This is a concerning comment "preventing controller-side code injection during collection loading"... I wonder if that's what's breaking this. # this loader implements key functionality for Ansible collections
# * implicit distributed namespace packages for the root Ansible namespace (no pkgutil.extend_path hackery reqd)
# * implicit package support for Python 2.7 (no need for __init__.py in collections, except to use standard Py2.7 tooling)
# * preventing controller-side code injection during collection loading
# * (default loader would execute arbitrary package code from all __init__.py's) I'll keep looking for how |
Here's where it gets prepended: They alias to ansible's modules and build |
so https://github.com/ansible/ansible/pull/52194/files#diff-279e661b06196c21d04f8226e65ee458R27 specifically is why we see the error about |
Ok wat. I'm able to dynamically load things now from sys.modules that are collections, but in this example: https://github.com/alikins/collection_inspect/tree/master/plugins only action and modules appear in sys.modules:
so I'm gonna have to go the full file path route now possibly, or I'll try one of the other *method classes used to detect source code! :D challenge_accepted |
So, that collection has:
Both action and module_utils are special cases in Ah. I suspect the only way Here's a significant change that adjusted how AnsiballZ collects files to be able to load module_utils with relative imports from collections into the AnsiballZ wrapper: ansible/ansible#61196 |
Oh, and there's a |
Thanks for the info 👍 |
FYI: you might not want to go too crazy here just yet- 2.10 changes the internals on this stuff quite a bit for 2.9 backwards compatibility with collectionized content that's no longer in Ansible core. See ansible/ansible#67684 which is just about to land in devel. Off the top of my head, I don't see anything here that 67684 should break, but the child_module calculation may be incomplete, since our loader's |
Thanks for the heads up! |
Does that mean that mitogen should only start supporting collections with 2.10? Plus, with mitogen's philosophy of supporting very old python, and very old ansible versions, what does this mean for ansible_mitogen supporting ansible 2.8 (w/ tech preview of collections) or 2.9 (w/ support for collections)? |
@cognifloyd Not sure if you're asking me or others on this PR. I've got no opinion- someone on our team pointed this PR out to me since I'm doing most of the work on the core engine's collections support (which is the part you're most likely to be impacted by). I will say that 2.10 will be the first release where everyone gets exposure to collections. Only the bleeding-edge folks and partners developing collections were doing much with them in 2.8/2.9, but the 2.10 community distributions of Ansible will all be collection-ized internally (ansible-base + all the collections required to maintain 2.9 compatibility "installed" to |
@nitzmahone I was talking more about the path forward for mitogen. Given that ansible mitogen advertises support for ansible 2.3 to 2.8 (see here), do we need to worry about not supporting collections with ansible 2.8 or 2.9? At this point, the answer is probably "no" we don't need to support collections there. We'll need to document that and get the website updated (is that in this repo? if it is, I can submit a PR). I understand that there are a lot of changes with the 2.10 release. I'd really like to try out mitogen with collections. |
Website gets updated in this way in the code: #714 Using I don't know what to do about the backwards compat issue @cognifloyd. We already have quite a few places in the code that do hackish things to support super old Ansible... I wonder if it makes sense to only support ansible 2.10+ with mitogen 0.3.0+ and tell folks to use mitogen < 0.3 if they want <= ansible 2.9, depending on the kinds of changes that 2.10 brings? We could do a 0.2.10 release with everything leading up to 2.10 support (like ansible_python_interpreter detection) and then do a version change to 0.3 for ansible 2.10+ perhaps 🤔 WDYT? |
I guess we need @dw to weigh in and say whether this would work for him and how he's using it in Operon. From everything I've read, backwards compat is super important for him. Personally, I'd be fine with 2.10+ in 0.3 |
Agreed 👍 |
@cognifloyd David is cool with bumping the version number for collections support :) |
By bumping the version number, you mean bumping the minimum ansible version, right? |
Thanks for tracking this down. Small addition: When executing a playbook, ansible reported a slightly different error that I was able to circumvent by adding |
@moreati @dw are you cool with the updates? If so, I'd like to finalize the PR, add some documentation, and tag a release. Big finalization question is based on the changes required, does it make sense to break backwards compat. I've I vote doing 2 releases: 1 with everything in |
Mostly. I've updated my review comments and I think two still need addressing. Additionally I see there are 5 remaining subtasks
I see the following blockers to releasing current master
|
@moreati thanks for updating your comments, I've responded to both 👍
I updated the subtasks, now all that's left is to remove the
There's a commit somewhere in this PR that fixes Travis tests iirc. I'll find it and open a new PR with it.
There's been multiple github tickets filed of people trying to fix our README with adding |
tar xvf sshpass-1.05.tar.gz && \ | ||
cd sshpass-1.05 && \ | ||
./configure && \ | ||
sudo make install") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be one of the test fixes that is split out from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old sshpass version doesn't work anymore on the test setup, so I'm fine splitting it out but then that PR would need to land before this one does. WDYT?
@@ -18,75 +18,64 @@ cache: | |||
|
|||
install: | |||
- grep -Erl git-lfs\|couchdb /etc/apt | sudo xargs rm -v | |||
- pip install -U pip==20.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip >= 20.2
is what triggers the UnicodeDecodeError you had to work around in ansible_install.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, nice find! I'm in favor of passing the encoding on command install rather than pinning to an older version of pip, so the fix isn't lost in the future. I'm imagining a scenario where a new pip version comes along that we want, and then we try and upgrade to it and run into the potential UnicodeDecodeError again, but forget how to resolve it. For that reason, I'm inclined to keep the encoding as-is in ansible-install.py. WDYT?
Hi :) I just encountered an issue trying this branch. Here is my setup:
I did run my playbook like this: ANSIBLE_STRATEGY_PLUGINS=/home/user/git/mitogen/ansible_mitogen/plugins/strategy ANSIBLE_STRATEGY=mitogen_linear ansible-playbook -i inventory playbook.yml -l "$HOST$" This is the error message:
The task where the error occurred is a Thanks for your work so far and let me know if you need more information! EDIT: setting |
@geigi thanks for testing! :) Your issue happens because Ansible itself threw an system exit, the relevant code is here: Setting Also, I made a different ticket here because I think if possible, Mitogen should error nicer and say Ansible 💥 instead of throwing a massive traceback. |
Thanks for the explanation 😊 Running the same playbook without mitogen and without the EDIT: I've tested it multiple times to make sure it wasn't a fluke |
From what I can tell, looks like Since |
Good to know that the default for |
@moreati I made changes/commented on your latest review. I'm going to make an PSA: travis.org is being EOL EOY, which will affect Mitogen since our |
New releases made: TL;DR of this PR thread: Mitogen has 2 releases now due to a bunch of API-breaking changes Ansible 2.10 made, 1 with Ansible < 2.10 and one Ansible 2.10+. Please tag any Github issues related to either release candidate with the new labels |
Thank you for you hard work! It’s great to see mitogen on the verge of supporting Ansible v2.10! 👍 |
Fixes #652
Fixes #731
Summary of changes:
master
)ActionModuleMixin
base class because collection support didn't have an ActionBase for some reason. This hopefully can be removed because I initially added it in when I was still on Ansible's devel branch, pre-2.10.master
where they are via itssys.path
so it can load them later when it looks for where dependencies live__synthetic__
thing that Ansible added to collections modulesTODO:
[ ] Synchronize module is broken, it moved from an ansible action plugin here: https://github.com/ansible/ansible/blob/v2.9.13/lib/ansible/plugins/action/synchronize.py to being in the builtin ansible collection-> Ansible 2.10 + Collections support #715 (comment)posix
:redirecting (type: action) ansible.builtin.synchronize to ansible.posix.synchronize
mitogen_linear
andlinear
strategies isn't working, something about Ansible 2.10 changed how strategy is loaded. Doing so in the same play file in differenthosts
blocks is failing (https://github.com/dw/mitogen/blob/master/tests/ansible/integration/strategy/mixed_vanilla_mitogen.yml#L2 this test, which can be reproduced locally as well).Not sure if this is a bug with Ansible 2.10 or with Mitogen specifically yet.It's an issue with how Mitogen overrides theconnection
obj (this works in Ansible 2.10 when I tried switching fromlinear
tofree
and back again), here's the traceback:possibly broken from here?: ansible/ansible@8c2754e ,
self._connection
is set to the old Mitogen connection obj, despitestrategy
being set tolinear
pip install ansible==2.10.0
) instead of viaansible-galaxy collection install
NOTE
sections in the code where backwards compat breaks, since it's been decided it's gonna break[ ] Add more test cases around different common collection usagestest cases use installed collections in them, so I think we're covered here[ ] Anything else that Ansible 2.10 might have brokenfixed everything that's been reported so far, and will fix future things in future releases[ ] get debops working: Ansible 2.10 + Collections support #715 (comment) it might not be 2.10 compatible just yet for our use case-> disabling debops because ran into Fatal error in pki-role: lookup plugin (task_src) not found debops/debops#1521