19:00:26 <gundalow> #startmeeting Ansible Core IRC meeting
19:00:26 <zodbot> Meeting started Tue Feb  5 19:00:26 2019 UTC.
19:00:26 <zodbot> This meeting is logged and archived in a public location.
19:00:26 <zodbot> The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:26 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:00:26 <zodbot> The meeting name has been set to 'ansible_core_irc_meeting'
19:00:32 <maxamillion> .hello2
19:00:32 * gundalow waves
19:00:33 <zodbot> maxamillion: maxamillion 'Adam Miller' <maxamillion@gmail.com>
19:00:35 <gundalow> Who's here
19:01:04 * dw waves
19:01:56 * felixfontein too
19:02:15 <gundalow> #chair cyberpear felixfontein tima maxamillion dw jimi|ansible nitzmahone
19:02:15 <zodbot> Current chairs: cyberpear dw felixfontein gundalow jimi|ansible maxamillion nitzmahone tima
19:02:34 <jimi|ansible> o/
19:02:46 <gundalow> #info Agenda https://github.com/ansible/community/labels/core
19:03:08 <sdoran> \o
19:03:50 <gundalow> So we have four things on the agenda today (forks, probe filter, Mitogen, and standardize connection vars to be `user` and `password`)
19:03:53 <gundalow> #chair sdoran
19:03:53 <zodbot> Current chairs: cyberpear dw felixfontein gundalow jimi|ansible maxamillion nitzmahone sdoran tima
19:04:19 <gundalow> Dag's at cfgmgmtcamp and doesn't appear to be online at the moment, so that takes us to two
19:04:34 <gundalow> #topic standardize connection vars to be `user` and `password` #51757
19:04:39 <gundalow> #info https://github.com/ansible/ansible/issues/51757
19:05:12 <gundalow> > For password connection variables, var names differ whether they end in `_pass` or in `_password`
19:05:12 <gundalow> > For some vars, both variants are supported, but for others only one of them is. We should standardize on one variant or the other and make sure that variant is supported everywhere. We should do the same for variants of `_user` vs `_username`. The chosen variant should be the only one used throughout the docs.
19:05:16 <jtanner> hi
19:05:18 <nitzmahone> +1 to that; it was just incomplete when it happened in 2.0
19:05:32 <gundalow> #chair jtanner dag
19:05:32 <zodbot> Current chairs: cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima
19:05:32 <dag> o/
19:05:43 <dag> sorry, rhel7 laptop resume issues :-/
19:05:47 <nitzmahone> but the aliases will need to be deprecated (or not)
19:05:48 * alongchamps waves
19:05:55 <gundalow> nitzmahone: `We should also standardize which "wins" when both variants are specified.` Is their a winning here, or just aliases`?
19:06:15 <gundalow> +1 to what this is doing, just want to check about any caveats
19:06:22 <cyberpear> what happens (or should happen) when both variants are specified with different values?
19:06:42 <jimi|ansible> whichever is last usually wins
19:07:04 <nitzmahone> IIRC they're kept in the order spec'd in config, and "more specific" is preferred over generic
19:07:44 <nitzmahone> so "old form" should be lower in precedence IMO
19:07:51 <cyberpear> +1
19:08:24 <gundalow> #info IIRC they're kept in the order spec'd in config, and "more specific" is preferred over generic. So "old form" should be lower in precedence IMO
19:08:35 <nitzmahone> We could consider deprecating them as well, but we'd have to either extend config to support that declaratively or move all the resolution code into the various plugins (ick)]
19:09:03 <cyberpear> any opinions on which form should be chosen as the standard?
19:09:28 <nitzmahone> ansible_user and ansible_password were what was chosen for 2.0 as standard
19:09:29 <gundalow> Does deprecating give us anything?
19:09:29 <gundalow> I'm wondering if we just standardise on formX (update docs, examples, etc) then that's enough
19:09:51 <cyberpear> sounds like enough to me, plus dev guide for future plugins
19:09:56 <nitzmahone> gundalow: it's mostly just a trip hazard for people that are updating content using the old forms, but much better if the docs are out there...
19:10:53 <gundalow> #info `ansible_user` and `ansible_password` were what was chosen for 2.0 as standard
19:11:16 <nitzmahone> So I think this comment says it: https://github.com/ansible/ansible/issues/51757#issuecomment-460752538
19:12:16 <cyberpear> I can open a PR to add those 4 vars.
19:12:38 <gundalow> deprecate old names or not? Either way, in the porting guide we can be clear what the "correct" vars are
19:13:37 <jimi|ansible> it's probably time we got around to deprecating them, it has at times caused confusion (though I thought that was mostly in the 1.x -> 2.x days)
19:14:01 <alongchamps> +1 for consistency - gives module developers some "constants" to work with
19:14:06 <nitzmahone> Yeah, it's also been a source of weird bugs in the past (intersection with other "magic" behavior)
19:14:12 <cyberpear> the disparate names were a tripping hazard -- the reason I brought this up
19:14:37 <gundalow> nitzmahone: You voting to deprecate old names?
19:14:56 <nitzmahone> TBC: we're still keeping the plugin-specific values, just fixing the suffixes to be consistent (and docs to consistently use the generic values)?
19:15:25 <gundalow> Do connection plugins have access to the deprecation helper functions?
19:15:33 <gundalow> hum, maybe that's implementation detail
19:15:38 <cyberpear> (would also be good to also document that user/password passed on command line via --ask-pass, etc are lower precedence than all other vars)
19:15:43 <nitzmahone> There are very limited circumstances where the plugin-specific keys are important (eg, connection switching behavior, and we could even work around that if we want with task vars)
19:16:11 <nitzmahone> gundalow: yeah, we can make that work
19:16:34 <sdoran> @gundalow Yes. The `deprecated()` method comes from `Display`, which can be used outside modules.
19:16:40 <nitzmahone> gundalow: I'm +0.5 for deprecating, but +1000 for make it consistent and just use the generic vars in the docs unless we're talking about one of the weird cases
19:16:43 <gundalow> sdoran: nitzmahone ace, thanks for confirming
19:17:13 <sdoran> I'm for `*_user` and `*_password` and having those override the `*_username` and `*_pass` variants.
19:17:35 <cyberpear> @nitzmahone yes, I find the plugin-specific vars useful in certain cases, so keep them please.
19:17:51 <gundalow> So how about
19:17:51 <gundalow> PR1: consistent naming, docs, examples, porting guide
19:17:51 <gundalow> PR2: (after PR1 merged) if needed use `deprecated()` on older names
19:17:51 <gundalow> PR3: Start a new page under https://docs.ansible.com/ansible/devel/dev_guide/ where we define these preferred terms, `validate_certs` being another example.
19:18:10 <gundalow> (maybe we vote if PR2 is needed next week)
19:18:48 <nitzmahone> PS: there's a reason I made validation a string not a bool, so don't go ripping that one out just yet
19:19:07 <cyberpear> +1
19:19:23 <nitzmahone> (but +1 for consistent name for it)
19:21:55 <gundalow> OK, think we are sorted, so in summary
19:21:55 <gundalow> ## PR1
19:21:55 <gundalow> * +1 to doing this
19:21:55 <gundalow> * grep for any references in the `docs/docsite` and update
19:21:55 <gundalow> * ansible_user` and `ansible_password` were what was chosen for 2.0 as standard
19:21:55 <gundalow> * Even if we don't deprecate we should add this to the porting guide
19:21:55 <gundalow> * would also be good to also document that user/password passed on command line via --ask-pass, etc are lower precedence than all other vars
19:21:56 <gundalow> PR1: consistent naming, docs, examples, porting guide
19:21:56 <gundalow> PR2: (Vote next meeting if needed) if needed use `deprecated()` on older names
19:21:57 <gundalow> PR3: Start a new page under https://docs.ansible.com/ansible/devel/dev_guide/ where we define these preferred terms, `validate_certs` being another example.
19:21:57 <gundalow> PR4: (Bonus) Also can we add CI test using the regexp in the issue to spot for connection plugins using incorrect names?
19:21:58 <gundalow> That sound right?
19:22:54 <cyberpear> sounds good to me
19:22:57 <nitzmahone> The plugins should be delegating all that to the config anyway, so PR4 shouldn't be necessary
19:23:25 <gundalow> nitzmahone: ack, updated, thanks
19:23:36 <gundalow> #agreed https://github.com/ansible/ansible/issues/51757#issuecomment-460769276
19:23:40 <gundalow> Thanks, next
19:24:15 <gundalow> So Dag's kindly offered to bump his items to next time, so we can move on to
19:24:20 <gundalow> #topic Mitogen
19:24:40 <dw> should i start?
19:24:41 <gundalow> For those that haven't met, we have dw with us today, the brains behind Mitogen
19:24:56 <dag> unsurprisingly I must say, my kindness :p
19:25:30 <maxamillion> dw: welcome!
19:25:50 <gundalow> dw: I guess a summary of where we are today. No need to focus on the past
19:26:12 <dw> it is my understanding that ansible-connection is the preferred way to get some form of mitogen upstream
19:26:54 <jimi|ansible> i believe that would be the consensus on the core team, yes
19:26:57 <sdoran> That is correct.
19:27:08 <dw> to approach anything like a PR for that, we must first define what functionality the ansible-connection approach will support. for example, if it uses mitogen's module executor, it means the existing -connection must be extended to support an "execute module" verb, and state such as the module loader path must be forwarded to -connection as it changes
19:27:19 <jimi|ansible> you mention some issues around the executor with that, could you elaborate on those?
19:28:32 <dw> there must also be some agreed-upon method where previously task-layer configurables like the configured python interpreter are available to -connection as it runs, and whether or not a change in such a variable causes a new -connection to exist, or if one -connection can handle all varieties of configuration for a particular host (or all hosts)
19:29:03 <nitzmahone> IIUC the "execute module" verb issue is pretty similar to the same problems we had with WinRM->PSRP; everything is done in terms of shell commands, so executing a module in a persistent interpreter would require reverse-engineering those shell commands or sending a "less-chewed" format.
19:29:43 <jimi|ansible> dw: also, why does there need to be an execute_module verb when there's execute? it's how we execute modules today
19:29:48 <nitzmahone> (we're currently doing the former in PSRP, but also have a need for this)
19:30:51 <dw> jimi|ansible: that's the question. if there is no 'execute module' verb, the connection layer (aka. -connection) does not have sufficient information to run a module while handling code loading/caching, etc. it just becomes another ssh-like connection method
19:31:18 <dw> in this case we would benefit only from much faster file transfer, and slightly fewer round-trips to run code provided by the existing module executor
19:31:41 <jimi|ansible> ok i think i see what you mean, because ansible-connection basically is just sent strings to execute via a shell
19:31:46 <dw> yep
19:32:11 <jimi|ansible> ok, so yeah i'm fine with adding a new method for that specifically
19:32:52 <nitzmahone> It's probably not just a single method though
19:33:04 <dw> that raises the next problem -- the hard-wiring of the existing executor, a change to modularize it was explicitly rejected in https://github.com/ansible/ansible/pull/47417 . how do we add a new 'execute module' verb in this acse?
19:33:08 <nitzmahone> *ideally* it's an entirely different interaction pattern between the connection and the executor
19:34:25 <nitzmahone> But that comes back to needing an intermediate solution that provides performance wins without blowing up or prematurely creating whole new plugin/abstraction layers that may or may not match out future needs in other areas.
19:34:35 <nitzmahone> *our
19:35:00 <dw> agreed. i wouldn't want a ton of framework just for mitogen, it needs to be generally useful
19:35:08 <dw> but i notice a modularized executor helps remove special cases for windows too
19:35:52 <nitzmahone> It absolutely does- building the Windows infrastructure around the existing idioms has been, shall we say, an interesting challenge...
19:36:09 <dw> it may even find use cases in networking, but i have no clue about that stuff
19:36:43 <dag> nitzmahone: :-)
19:37:56 <nitzmahone> The trick there being teasing apart the strategy and executor bits to support the necessary abstractions without breaking existing stuff or painting ourselves into any corners.
19:38:37 <jimi|ansible> umm, those parts have already been "teased" apart...
19:39:00 <jimi|ansible> unless you mean a different strategy
19:39:34 <nitzmahone> Actually in this case, it's more about the intersection of the module build
19:39:41 <nitzmahone> and the shell/connection layers
19:40:02 <nitzmahone> (but strategy comes in as well in a few places when I whiteboarded this out awhile back)
19:40:44 <nitzmahone> eg keeping it so that the strategy can still have the control it needs to over the execution process
19:41:12 * nitzmahone details foggy, been a long time since I thought about this in the big picture
19:41:16 <dw> with action monkey-patches removed, mitogen's strategy bits are almost pointless, they're just to make it easier to run a playbook under mitogen without changing lots of vars. -connection's existing framework handles the "spin up a persistent process" parts
19:41:59 <dw> the monkey-patches are 99% about module execution. the other stuff has very little performance advantage, it's just nice to not call out to shell where it's no longer necessary
19:43:08 <dw> some of the action monkey patch removed round-trips due to tempfile management, that is a big speedup in some scenarios, but it is not mandatory
19:43:49 <dw> so the focus i think is in "how do we teach -connection to know everything it needs to know to execute a module", and "how does -connection get to know strange per-task vars that effect the interpreter config"
19:44:09 <nitzmahone> I assumed mitogen would be operating in a pipelined mode by default, so tempfile/tempdir management should only be an issue for unoptimized template/copy cases and maybe async (for today's POSIX impl of it anyway)...
19:44:57 <dw> nitzmahone: tempfile stuff is pipelined :) but it is the different between "run this 'rmdir' shell command please" and "this is a temp file removal, you dont need to wait for it to complete. capture of semantic information, just like module execution
19:44:59 <nitzmahone> (similar to the Windows case; pipelining is forced on and only disallowed to support things like KEEP_REMOTE_FILES)
19:45:06 <jimi|ansible> connection can always import anything from ansible to do the execution more directly than simply handling the connection plugin
19:45:31 <jimi|ansible> the difficulty will be in transferring state over to ansible-connection
19:46:03 <dag> So we do want to have Mitogen (in whatever form) added to Ansible as a connection method ?
19:46:27 <dw> jimi|ansible: for some idea of the state involved, here is a reasonably current RPC it makes: https://gist.github.com/dw/3755015863492746433238691187b3cc
19:46:29 <jimi|ansible> i thought that was part of having it be run through ansible-connection?
19:46:36 <jimi|ansible> it has to be a connection plugin for that
19:47:17 <dag> jimi|ansible: ok, I wasn't sure this was the plan
19:47:56 <dag> because things seemed to have died down, so I wonder how we can get this thing on track again
19:48:02 <jimi|ansible> sure, but the thing i'm thinking of specifically is the plugin loader - when someone runs a role for instance, it may add new plugins so those objects get changed - this was something i originally had problems with in 2.0 when moving more things to the post-fork side
19:48:42 <dw> yep! the plugin path search list must be shared with -connection (i think) for each new task
19:48:48 <dag> (there was a lot of interest in Mitogen at cfgmgmtcamp and a lot of uncertainty of where it was going)
19:49:25 <dw> dag: if -connection is a necessary first step, i am happy to try and make that happen, but architecturally it is unfit for all the same reasons forking is unfit. that's a discussion for another time, though
19:49:26 <dag> (people are using Mitogen in production)
19:49:54 <dw> yes, many people on networks where i'd prefer they didn't run it :p
19:50:13 <jimi|ansible> dw: i'm trying to get threading done, need to get testing sorted out to make sure i haven't broken anything
19:50:23 <jimi|ansible> also the rebase i did today against devel did NOT go smoothly...
19:50:26 <dag> jimi|ansible: \o/
19:51:00 <dw> hah :) jimi|ansible even just the module fixups you found.. i found the same ones. if they are landed (and they look safe to land), the big chunk of code i have sitting here might be able to live in the extension. ("rinse, repeat")
19:52:02 <maxamillion> module fixups?
19:52:18 <dw> module loader is not thread-safe, nor e.g. are things that touch stdio
19:52:42 <dw> (the context is both jimi|ansible's work and the strategy reimplementation i have both are heavily threaded)
19:53:10 <jimi|ansible> i think module loader is, it's using a flock (with threading I can use a normal threading.Lock() though), but plugin loader most definitely was not thread safe
19:53:30 <dw> ah, yep. :)
19:54:29 <jimi|ansible> that's actually what broke on the rebase, I believe we fixed some bugs there and it was right in the spot i had added the lock acquire/release bits so it is not merging cleanly anymore
19:55:31 <jimi|ansible> ok, so are there any outstanding questions here, or are we clear on a path forward?
19:55:40 <dw> so regarding outcomes, how do we reach agreement on what needs to happen to make -connection viable? did we all agree something needs to be done about modular executor?
19:55:47 <dw> :)
19:56:33 <jimi|ansible> i think we can agree to add execute_module as a verb to ConnectionBase, and in that method you can do whatever you want inside ansible-connection right?
19:56:45 <dw> deal!
19:57:01 * jborean93 would be great to do something with that with psrp
19:57:12 <jimi|ansible> we can tie in something to TaskExecutor to basically hand-off everything to the connection plugin when that connection supports execute_module
19:57:31 <bcoca> action plugin, currently its action plugins doing that work
19:57:38 <jimi|ansible> kind of like how we short-circuit TE for include* now
19:57:49 <dw> should i send another PR, or perhaps draft a proposal for that change? it is kinda basically the target plugins PR, but scaled back a bit
19:57:53 <jimi|ansible> nitzmahone: does that work for you in regards to your windows stuff too?
19:58:20 <nitzmahone> Conceptually, yes, in general, though I suspect we'll actually want to deconstruct the process in the future
19:58:33 <jimi|ansible> we can burn that bridge when we come to it :)
19:58:36 <nitzmahone> This is a good way to isolate the required changes for now without blowing up the world
19:58:46 <bcoca> you pacifist!
19:58:47 * maxamillion gets matches and gasoline
19:59:02 <jimi|ansible> dw: yes a proposal is always good just to make sure we're in agreenment on the technical aspects
19:59:10 <jimi|ansible> and it gives us something to refer to in future meetings
19:59:27 <dw> yes, i agree this is a good first step. i assume extending ansible-connection with the new verb should be straightforward -- except that the controller-side connection plugin must somehow know that the connection plugin that runs inside -connection can handle execute_module()
19:59:29 <nitzmahone> I'm also thinking it might need to be a short-circuit in the action base though too, not the TE, but there might be good reasons for either
19:59:44 <bcoca> te doesnt execute module, action plugin does, that is where you need the override
19:59:55 <bcoca> te always calls action plugin as 'action handler'
20:00:17 <jimi|ansible> yeah that might be better because it will reduce duplication in all the other TE-level control stuff like retries, etc.
20:00:24 <nitzmahone> yep
20:00:26 <gundalow> So if we are going down the proposal route, what specifically needs to be covered in it?
20:00:43 * nitzmahone tries to think if this is going to cause problems with collection module loading- will also jump off that bridge when we come to it
20:00:49 <dw> gundalow: baby steps.. i'd suggest /just/ the execute_module() step. then rinse, repeat with another chunk, perhaps
20:01:01 <bcoca> ActionBase has execute_module, but it also calls 'configure_module' which lives in executor/module_common, that might also need changing
20:01:01 <nitzmahone> gundalow: no proposal for this IMO
20:01:13 <gundalow> oh, sorry, I must have miss understood, carry on
20:01:29 <gundalow> Just trying to make sure that any actions get captured
20:01:35 <gundalow> (though this IRC meeting is logged)
20:01:37 <bcoca> use #action
20:01:39 <jimi|ansible> 1) adding a new verb named "execute_module"
20:01:39 <jimi|ansible> 2) adding support to short-circuit execution to defer the module execution to the connection plugin
20:01:39 <jimi|ansible> 3) adding the mitogen connection plugin and any bits required to get it working properly with ansible-connection
20:01:41 <dw> bcoca: the conversation above was basically to agree on a short-circuit of that logic if the connection plugin supports it. ~2-line if statement in ActionBase
20:01:43 <nitzmahone> configure_module (and it's children) are the part that needs to be deconstructed in the future
20:01:49 <dag> bcoca: weren't you on PTO ?
20:01:55 <bcoca> i came to clean up the bodies
20:01:59 <dag> :-D
20:02:01 <nitzmahone> / kick bcoca ;)
20:02:02 <jimi|ansible> did i miss anything?
20:02:09 <bcoca> dw:  the issue is that 'async' is also implemented in action plugin
20:02:13 <bcoca> its not as simple as that
20:02:19 <dw> jimi|ansible: as a first volley, this looks good
20:02:26 <bcoca> among other things like 'temp dirs', 'interpreter location' ,etc
20:02:32 <gundalow> #action 1) adding a new verb named "execute_module"
20:02:43 <gundalow> #action 2) adding support to short-circuit execution to defer the module execution to the connection plugin
20:02:45 <jimi|ansible> bcoca: most likely we'll have to build some kind of async support into mitogen so it handles that outside of the normal async bits
20:02:53 <gundalow> #action 3) adding the mitogen connection plugin and any bits required to get it working properly with ansible-connection
20:02:56 <gundalow> #chair bcoca
20:02:56 <zodbot> Current chairs: bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima
20:02:57 <nitzmahone> dw: one little thing: what will `raw:` do against mitogen if all the guts are in execute_module? Just some shell fallback, fail, or ?
20:02:57 <bcoca> jimi|ansible: among other things
20:03:00 <jimi|ansible> but maybe not, don't know at this point
20:03:16 <dw> i have some general questions aside from the work
20:03:23 <bcoca> nitzmahone: also 'script' , both it and raw use 'low_level-execute' directly
20:03:31 <nitzmahone> (and reboot, and many others)
20:03:35 <bcoca> mitogen connection just has to implement low_level_execute
20:03:40 <jimi|ansible> nitzmahone: yeah i think the nice thing there is we can make the short-circuit conditional and not have it do unsupported stuff like anything that's non-python via the normal path
20:03:46 <nitzmahone> exactly
20:03:55 <jimi|ansible> bcoca: i don't think so, LLE is at the action layer
20:04:13 <bcoca> jimi|ansible: it calls connection to actually exec
20:04:14 <jtanner> correct, action has LLE, and LLE points at connectionplugin.execute
20:04:14 <dw> i've heard from multiple sources by now that the team have paid close attention to the plugin, and also been told that there are cases where it doesn't help, or problems that it creates. aside from "must have python installed" (e.g. broken raw:), are you aware of other scenarios where there is either a) no speedup, or b) something is broken
20:04:16 <bcoca> its simple wrapper
20:04:21 <bcoca> that calls 'become' when needed
20:04:27 <bcoca> s/simple//
20:04:27 <jimi|ansible> yes, it basically just calls conn.execute()
20:04:47 <jimi|ansible> but like i said we can conditionally control what it's doing if it's not a python module
20:05:03 <bcoca> i.e not using 'normal' action plugin?
20:05:18 <jimi|ansible> more if it's not a "new-style" module
20:05:23 <nitzmahone> dw: IIRC the generic "I need code" Python fetch could cause problems in heterogenous Python envs (eg, ansible controller is 3.6, target is 2.7)
20:05:53 <bcoca> jimi|ansible: we might need to refactor a lot from actionbase (that is where we detect module style)
20:05:53 <nitzmahone> can't remember if you implemented any filtering to prevent it from trying to fetch code impls from arbitrary namespaces
20:05:56 <dw> nitzmahone: do you know when this was tried? 2.x<->3.x support has improved immensely over time
20:06:18 <dw> yes, the extension has importer blacklisting since very early -- prerelease IIRC
20:06:20 <nitzmahone> It's been a long time since I played with it- I just remember thinking it would be a problem as it was several months ago
20:06:45 <nitzmahone> I think I remember thinking in our case it needed to be whitelisting
20:06:48 <jtanner> dw: most of the "slow" playbooks i've worked on today have been typically slow execution of a command on one host in the batch causing the whole task the suffer. Things like yum getting locked, or a D state or some $random nonsense on the remote.
20:07:03 <bcoca> fact gathering ...
20:07:27 <jimi|ansible> that's what "free" mode was supposed to help address
20:07:28 <dag> jtanner: indeed
20:07:31 <jtanner> we also have memory ballooning from A) nested includes B) receiving lots of data from lots of hosts all at once
20:07:41 <bcoca> fact gathering ...
20:07:42 <nitzmahone> Doing it as a generic import hook may also cause problems with py2/3 namespace moves, conditional import behaviors, etc
20:08:03 <dw> jtanner: these make sense. i'm more interested in where the lack of priority comes from -- is there some 'aha!' case i may not know about that was uncovered during the team's testing
20:08:36 <jtanner> not that i'm aware ... binary modules?
20:08:39 <dw> jtanner: many users i talk to consider the extension essential to their workflow. so the lack of priority to investigating this work seems quite contradictory, and i don't understand where the difference lies
20:08:44 <jimi|ansible> when I rewrote ansible in go, it seemed like the biggest problem was python interpreter startup time on the remote side
20:08:50 <bcoca> dw:  in most cases 'normal' speed is acceptable, most of the performance issues don't have to do with task connection/module start, but with the actual task the module itself does
20:09:02 <dag> jimi|ansible: you rewrote Ansible in go ??
20:09:08 <bcoca> it would be very nice to remove that overhead
20:09:13 <dw> bcoca: that's often not true. https://mitogen.readthedocs.io/en/dmw/ansible.html#debops-uk-to-india
20:09:14 <jimi|ansible> dag: maybe :)
20:09:16 <sivel> I cannot speak to mitogen specifically, but as an FYI we haven't really seen any perf gains from the threading work. In fact in a number of scenarios, threading has shown to be a bit slower thank forking
20:09:18 <bcoca> dag: the executor
20:09:39 <dag> jimi|ansible: Any pointers ? :) We were thinking about that at one point, also run go modules..
20:09:55 <jimi|ansible> dag: problem with go modules is binary differences between the machines
20:10:03 <dw> sivel / jimi|ansible, re threading, you should check out the hacks mitogen does. CPU affinity pinning and GIL tick interval in combination are a _huge_ speed increase. ~33% for GIL interval, ~20% for cpu affinity
20:10:06 <jimi|ansible> but yeah that'd be fast :)
20:10:10 <dag> jimi|ansible: a problem that can be overcome, surely
20:10:16 <jtanner> we also already support running go modules (that are compiled)
20:10:29 <dw> jimi|ansible: key insight: threads or not, the GIL means a python app is fundamentally single-threaded (more or less). so running those threads on competing CPUs almost never makes sense
20:10:32 <jimi|ansible> yes we do, that was actually added after i did the engine bits
20:10:38 <bcoca> dw: im talking about actual complaints we get, plenty of examples that show we have more overhead than we should
20:10:48 <jimi|ansible> dw: right, it's where threading runs into a big performance hit
20:11:06 <jimi|ansible> if the module returns a big chunk of data, all those threads end up processing it on one CPU
20:11:17 <dw> jimi|ansible: python's threading performs very well if you treat it more like a coroutines implementation than an actual threads implementation ;)
20:11:20 <sivel> plus it's trying to manage threads on that same CPU
20:11:39 <sivel> so you just get more CPU contention doing everything on a single CPU
20:11:52 <sivel> or core if you will
20:11:55 <nitzmahone> One of the reasons I've been pushing for 3.5+ controller, but RHEL7... :(
20:12:06 <bcoca> which you need to do to keep the data consistent (either serialize or lock .. which serializes)
20:12:25 <nitzmahone> at least Python 3.6 is in EPEL now, but that's a no-go for a lot of cases
20:12:33 <jimi|ansible> yes but the good thing about my threading work is it basically turns the threading/forking model into a plugin that could add a coroutine method in the future for 3.5+ implementations
20:12:56 <bcoca> or have a 'if PY3: '
20:13:18 <dw> jimi|ansible: i'm excited about your branch, but it's missing a lot of work to actually paralllize the run across cpus. that code exists, i just have no vehicle to release it yet
20:13:36 <dw> per last year, the new strategy to legitimately (no wastage) saturate 16 CPUs
20:13:41 <dw> *to=can
20:14:49 <jimi|ansible> ok, so i think we're good on this discussion, and we're past our hour but there's more on the agenda I believe?
20:14:52 <jimi|ansible> ^ gundalow ?
20:15:13 <bcoca> you might want to postpone to next meeting, though today we have a rare 'full compliment'
20:15:36 <gundalow> jimi|ansible: we are pushing other topics to next session (dag was happy with that), remaining topic is just a module update
20:15:48 <gundalow> dw: You got what you need to progress?
20:15:53 <dw> im happy to skip any remaining discussions -- there have been real decisions made here that help with the -connection approach
20:16:01 <dag> gundalow: we can review that other item on the agenda
20:16:11 <dw> gundalow: i believe so. proposal for execute_module() changes only. does everyone agree with that?
20:16:27 * bcoca goes back to buying folding crates, real reason he turned his pc on
20:16:39 <gundalow> #agreed proposal for `execute_module()` changes only.
20:16:44 <jimi|ansible> dw: the proposal should include the 1-3 points I posted above I think
20:16:50 <dw> jimi|ansible: yep :) great
20:16:52 <jimi|ansible> since it will need to tie in with other bits
20:16:56 <gundalow> #undo
20:16:56 <zodbot> Removing item from minutes: AGREED by gundalow at 20:16:39 : proposal for `execute_module()` changes only.
20:17:05 <gundalow> #action 4) proposal for `execute_module()` changes only.
20:17:13 <gundalow> there, fixed the OCD
20:17:20 <gundalow> ok, NEXT
20:17:32 <gundalow> dag: is one of your higher priority?
20:18:05 <gundalow> #topic ansible/ansible#42528 -- New 'forks' directive, per-play, per-block, per-task
20:18:20 <gundalow> #info https://github.com/ansible/ansib
20:18:23 <gundalow> #undo
20:18:23 <zodbot> Removing item from minutes: INFO by gundalow at 20:18:20 : https://github.com/ansible/ansib
20:18:31 <gundalow> #info https://github.com/ansible/ansible/pull/42528
20:18:52 <gundalow> > With this patch it is possible to limit the number of concurrent task runs for the task where it has been added. This is similar to forks, but affects only one task. It can be used to serialize one or more tasks.
20:19:26 <gundalow> dag: How can we help with this?
20:19:46 <dag> Yeah, so this used to be called `max_concurrency:`, but was renamed to `forks:`
20:20:17 <dag> gundalow: well, there was some discussion and I want to know if this is something that we can merge ?
20:20:43 <dag> gundalow: at least 2 persons tested it and it seems to work fine, but it is hard to create integration tests for this
20:21:35 <dag> the change is very straight-forward, and the functionality fixes various user problems (or at least avoid ugly workarounds)
20:21:38 <gundalow> So ignoring the name, do people think this is a good thing?
20:21:58 <gundalow> I see maxamillion is happy with it
20:22:10 <abadger1999> I think bcoca was the one that raised issues with it.  But he's on PTO.
20:22:26 <dag> gundalow: you can *see* maxamillion ? :)
20:22:26 <jtanner> without tests, there's very little to assert we won't break it every day.
20:22:41 <dag> abadger1999: :)
20:23:03 <dag> jtanner: if you can tell me how we could test this, I would be happy to ensure we have integration tests
20:23:17 <maxamillion> dag: if he can, that'd be a cool trick
20:23:23 <maxamillion> creepy, but impressive
20:23:32 <jtanner> the runme.sh gives you limitless possibilities
20:23:37 <gundalow> I see maxamillion *is happy with it*
20:23:39 <jtanner> runme.sh pattern*
20:24:01 <dag> maxamillion: well, I was thinking of adding something that counts the running forks
20:24:37 <jtanner> that information is in the debug logs
20:24:40 <dag> jtanner: ok, I'll see if we can add something
20:25:25 <gundalow> #info If this is accepted we will need docs.
20:25:39 <gundalow> Q: Where should this be documented
20:25:48 <gundalow> #info Could test it via `runme.sh` to run a Playbook and count the number of forks found in the debug logs
20:25:51 <gundalow> Anything else on this one?
20:26:09 <dag> gundalow: yup, docs too, I'll get it done, thanks
20:27:19 <gundalow> dag: shall we move onto your next item?
20:28:17 <dag> sure
20:28:22 <gundalow> thanks
20:28:38 <gundalow> #topic Add the "probe" filter to dig into an array or a hash #50706
20:28:45 <gundalow> #info https://github.com/ansible/ansible/pull/50706
20:28:58 <gundalow> > This PR adds the `probe` filter in native filters.
20:28:59 <dag> So originally this filter was named "dig", but obviously that's quite confusing
20:29:16 <gundalow> heeh, yes that would have been confusing
20:29:31 <gundalow> so putting naming things aside
20:29:44 <dag> currently it is heard to get some value out of a complex datastructure, unless you know beforehand it exists, or you have to jump through hoops to ensure it doesn't break
20:29:49 <gundalow> Is the idea sound and solving a useful problem
20:29:54 <dag> s/heard/hard
20:30:52 <agaffney> you mean you don't like to write a series of 4 nested |default() statements?
20:31:18 <gundalow> #chair agaffney
20:31:18 <zodbot> Current chairs: agaffney bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima
20:31:23 <cyberpear> json_query can currently be used to do something similar
20:31:48 <cyberpear> but that requires another dep to be installed on the controller
20:31:52 <agaffney> I get the problem that it's trying to solve, but something just "feels wrong" about it to me
20:32:55 <cyberpear> would be better if default() didn't error out, or had an option to avoid the error, but I'm unaware of the implementation details
20:33:21 <cyberpear> (likely the error is before it ever gets to default())
20:33:40 <dag> agaffney: exactly
20:33:46 <agaffney> cyberpear: jinja returns an Undefined for a non-existent var or key, but if you have two undefined in a row, the access of anything on the first Undefined throws an exception that |default() can't catch
20:34:12 <shertel> . syntax would be nice. sivel has a simple implementation https://gist.github.com/sivel/f76b62b548631f06b95532e24ec7d538
20:34:19 <agaffney> the |default() filter just checks for a jinja2.Undefined object being passed to it
20:34:48 * dag had something similar written before for a customer, never found anything better than this
20:35:02 <dag> so if someone else proposes the same thing, there's a need
20:35:43 <cyberpear> could we somehow define Undefined[anything] to also be Undefined?
20:35:45 <cyberpear> https://github.com/ansible/ansible/issues/35992
20:35:46 <agaffney> I agree there's a need, but I feel this could be better solved by a custom Undefined class that just keeps returning Undefined rather than an AttributeError exception (or whatever it does)
20:35:54 <cyberpear> has an example of how to do it w/ json_query
20:36:27 <agaffney> a custom Undefined class should be trivial to implement
20:37:02 <agaffney> it would deviate from standard jinja2 behavior, but that's probably not a big deal since jinja2 itself provides multiple choices for Undefined behavior
20:37:42 <sivel> we discussed in a previous conversation that this "probe" (formerly dig) needed to implement dot syntax, instead of  discreete arg syntax
20:37:58 <dag> agaffney: I didn't think we could do that, but sure, if nobody objects I guess we can ;-)
20:38:00 <sivel> and I see shertel mentions something similar above
20:38:17 <dag> sivel: where was this previous conversation ?
20:38:39 <sivel> dag: we discussed it in our internal core triage meeting
20:38:56 <dag> sivel: ok, just ignore me feeling left out :-D
20:38:57 <sivel> I believe shertel was going to perform a full review indicating that requirement in the near future
20:39:07 <sivel> sorry we haven't gotten around to it yet
20:39:18 <dag> but maybe agaffney's solution is better ? Changing the behaviour ?
20:39:23 <agaffney> http://jinja.pocoo.org/docs/2.10/api/#undefined-types
20:39:32 <shertel> Sorry, it's been slipping for other things. I like it and it solves some of the issues I was trying to fix with https://github.com/ansible/ansible/pull/27298
20:39:39 <agaffney> jinja offers a few default Undefined implementations and allows you to provide your own
20:39:57 <dag> agaffney: we are using Undefined() like that, but from a filter
20:40:08 <sivel> I cannot necessarily say without seeing how it works. I've never tried to influence the undefined behavior in that manner
20:40:16 <gundalow> #chair sivel
20:40:16 <zodbot> Current chairs: agaffney bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran sivel tima
20:40:25 <dag> agaffney: the question is whether we want to change the existing Jinja behaviour in this case
20:40:32 <agaffney> I'd think we could just subclass jinja2.Undefined and override __getitem__() and __getattr__()
20:40:56 <agaffney> dag: afaik, it shouldn't break any existing stuff
20:40:57 <cyberpear> +1
20:41:10 <dag> (no need to be sorry, I was only half joking feeling left out ;-))
20:41:47 <dag> agaffney: if nobody is depending on that specific behavior, it's hard to tell
20:42:12 <dag> agaffney: this would be my preferred solution to be honest.
20:42:16 <agaffney> dag: I doubt they are, since exceptions from jinja aren't catchable from within a template
20:42:38 <dag> ignore_errors ?
20:42:42 <agaffney> so anyone now is either doing a bunch of 'foo is defined and foo.bar is defined and ...' or nested |default(), or getting an exception
20:43:01 <sivel> it would require also understanding how it interacts with error_on_undefined_vars
20:43:32 <agaffney> you would still get an Undefined object in the end, so I wouldn't think anything would change there
20:43:48 <agaffney> it would just change the behavior where an exception is thrown for nested undefined values
20:44:44 <cyberpear> the currently-thrown exceptions are caught with something like block-rescue construct, though I don't have any playbooks that do that
20:44:48 <agaffney> we are already overriding the default jinja undefined behavior by having it use jinja2.StrictUndefined
20:45:21 <dag> agaffney: best thing is to try it out and see what it brings
20:45:32 <agaffney> yeah, it should be pretty easy to test
20:45:51 <dag> let's try it as an alternative
20:48:03 <agaffney> I'm putting together a branch to test it now
20:48:20 <gundalow> agaffney: ace, thanks
20:48:37 * gundalow realises we are at 108 minutes
20:50:38 <agaffney> I created a WIP PR that just wraps jinja2.StrictUndefined with a class that just contains 'pass' to make sure that tests still pass, and then I'll actually add the desired functionality
20:50:43 <gundalow> nice
20:51:04 <gundalow> Shall we continue discussion on PR/#ansible-devel and #endmeeting?
20:51:30 <gundalow> #info https://github.com/ansible/ansible/pull/51768 [WIP] Custom jinja Undefined class
20:51:47 <dag> Yup :)
20:52:01 <gundalow> Great meeting, thank you everybody for your time
20:52:06 <gundalow> #endmeeting