19:00:26 #startmeeting Ansible Core IRC meeting 19:00:26 Meeting started Tue Feb 5 19:00:26 2019 UTC. 19:00:26 This meeting is logged and archived in a public location. 19:00:26 The chair is gundalow. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:26 Useful Commands: #action #agreed #halp #info #idea #link #topic. 19:00:26 The meeting name has been set to 'ansible_core_irc_meeting' 19:00:32 .hello2 19:00:32 * gundalow waves 19:00:33 maxamillion: maxamillion 'Adam Miller' 19:00:35 Who's here 19:01:04 * dw waves 19:01:56 * felixfontein too 19:02:15 #chair cyberpear felixfontein tima maxamillion dw jimi|ansible nitzmahone 19:02:15 Current chairs: cyberpear dw felixfontein gundalow jimi|ansible maxamillion nitzmahone tima 19:02:34 o/ 19:02:46 #info Agenda https://github.com/ansible/community/labels/core 19:03:08 \o 19:03:50 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 #chair sdoran 19:03:53 Current chairs: cyberpear dw felixfontein gundalow jimi|ansible maxamillion nitzmahone sdoran tima 19:04:19 Dag's at cfgmgmtcamp and doesn't appear to be online at the moment, so that takes us to two 19:04:34 #topic standardize connection vars to be `user` and `password` #51757 19:04:39 #info https://github.com/ansible/ansible/issues/51757 19:05:12 > For password connection variables, var names differ whether they end in `_pass` or in `_password` 19:05:12 > 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 hi 19:05:18 +1 to that; it was just incomplete when it happened in 2.0 19:05:32 #chair jtanner dag 19:05:32 Current chairs: cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima 19:05:32 o/ 19:05:43 sorry, rhel7 laptop resume issues :-/ 19:05:47 but the aliases will need to be deprecated (or not) 19:05:48 * alongchamps waves 19:05:55 nitzmahone: `We should also standardize which "wins" when both variants are specified.` Is their a winning here, or just aliases`? 19:06:15 +1 to what this is doing, just want to check about any caveats 19:06:22 what happens (or should happen) when both variants are specified with different values? 19:06:42 whichever is last usually wins 19:07:04 IIRC they're kept in the order spec'd in config, and "more specific" is preferred over generic 19:07:44 so "old form" should be lower in precedence IMO 19:07:51 +1 19:08:24 #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 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 any opinions on which form should be chosen as the standard? 19:09:28 ansible_user and ansible_password were what was chosen for 2.0 as standard 19:09:29 Does deprecating give us anything? 19:09:29 I'm wondering if we just standardise on formX (update docs, examples, etc) then that's enough 19:09:51 sounds like enough to me, plus dev guide for future plugins 19:09:56 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 #info `ansible_user` and `ansible_password` were what was chosen for 2.0 as standard 19:11:16 So I think this comment says it: https://github.com/ansible/ansible/issues/51757#issuecomment-460752538 19:12:16 I can open a PR to add those 4 vars. 19:12:38 deprecate old names or not? Either way, in the porting guide we can be clear what the "correct" vars are 19:13:37 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 +1 for consistency - gives module developers some "constants" to work with 19:14:06 Yeah, it's also been a source of weird bugs in the past (intersection with other "magic" behavior) 19:14:12 the disparate names were a tripping hazard -- the reason I brought this up 19:14:37 nitzmahone: You voting to deprecate old names? 19:14:56 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 Do connection plugins have access to the deprecation helper functions? 19:15:33 hum, maybe that's implementation detail 19:15:38 (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 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 gundalow: yeah, we can make that work 19:16:34 @gundalow Yes. The `deprecated()` method comes from `Display`, which can be used outside modules. 19:16:40 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 sdoran: nitzmahone ace, thanks for confirming 19:17:13 I'm for `*_user` and `*_password` and having those override the `*_username` and `*_pass` variants. 19:17:35 @nitzmahone yes, I find the plugin-specific vars useful in certain cases, so keep them please. 19:17:51 So how about 19:17:51 PR1: consistent naming, docs, examples, porting guide 19:17:51 PR2: (after PR1 merged) if needed use `deprecated()` on older names 19:17:51 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 (maybe we vote if PR2 is needed next week) 19:18:48 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 +1 19:19:23 (but +1 for consistent name for it) 19:21:55 OK, think we are sorted, so in summary 19:21:55 ## PR1 19:21:55 * +1 to doing this 19:21:55 * grep for any references in the `docs/docsite` and update 19:21:55 * ansible_user` and `ansible_password` were what was chosen for 2.0 as standard 19:21:55 * Even if we don't deprecate we should add this to the porting guide 19:21:55 * 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 PR1: consistent naming, docs, examples, porting guide 19:21:56 PR2: (Vote next meeting if needed) if needed use `deprecated()` on older names 19:21:57 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 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 That sound right? 19:22:54 sounds good to me 19:22:57 The plugins should be delegating all that to the config anyway, so PR4 shouldn't be necessary 19:23:25 nitzmahone: ack, updated, thanks 19:23:36 #agreed https://github.com/ansible/ansible/issues/51757#issuecomment-460769276 19:23:40 Thanks, next 19:24:15 So Dag's kindly offered to bump his items to next time, so we can move on to 19:24:20 #topic Mitogen 19:24:40 should i start? 19:24:41 For those that haven't met, we have dw with us today, the brains behind Mitogen 19:24:56 unsurprisingly I must say, my kindness :p 19:25:30 dw: welcome! 19:25:50 dw: I guess a summary of where we are today. No need to focus on the past 19:26:12 it is my understanding that ansible-connection is the preferred way to get some form of mitogen upstream 19:26:54 i believe that would be the consensus on the core team, yes 19:26:57 That is correct. 19:27:08 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 you mention some issues around the executor with that, could you elaborate on those? 19:28:32 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 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 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 (we're currently doing the former in PSRP, but also have a need for this) 19:30:51 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 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 ok i think i see what you mean, because ansible-connection basically is just sent strings to execute via a shell 19:31:46 yep 19:32:11 ok, so yeah i'm fine with adding a new method for that specifically 19:32:52 It's probably not just a single method though 19:33:04 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 *ideally* it's an entirely different interaction pattern between the connection and the executor 19:34:25 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 *our 19:35:00 agreed. i wouldn't want a ton of framework just for mitogen, it needs to be generally useful 19:35:08 but i notice a modularized executor helps remove special cases for windows too 19:35:52 It absolutely does- building the Windows infrastructure around the existing idioms has been, shall we say, an interesting challenge... 19:36:09 it may even find use cases in networking, but i have no clue about that stuff 19:36:43 nitzmahone: :-) 19:37:56 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 umm, those parts have already been "teased" apart... 19:39:00 unless you mean a different strategy 19:39:34 Actually in this case, it's more about the intersection of the module build 19:39:41 and the shell/connection layers 19:40:02 (but strategy comes in as well in a few places when I whiteboarded this out awhile back) 19:40:44 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 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 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 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 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 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 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 (similar to the Windows case; pipelining is forced on and only disallowed to support things like KEEP_REMOTE_FILES) 19:45:06 connection can always import anything from ansible to do the execution more directly than simply handling the connection plugin 19:45:31 the difficulty will be in transferring state over to ansible-connection 19:46:03 So we do want to have Mitogen (in whatever form) added to Ansible as a connection method ? 19:46:27 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 i thought that was part of having it be run through ansible-connection? 19:46:36 it has to be a connection plugin for that 19:47:17 jimi|ansible: ok, I wasn't sure this was the plan 19:47:56 because things seemed to have died down, so I wonder how we can get this thing on track again 19:48:02 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 yep! the plugin path search list must be shared with -connection (i think) for each new task 19:48:48 (there was a lot of interest in Mitogen at cfgmgmtcamp and a lot of uncertainty of where it was going) 19:49:25 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 (people are using Mitogen in production) 19:49:54 yes, many people on networks where i'd prefer they didn't run it :p 19:50:13 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 also the rebase i did today against devel did NOT go smoothly... 19:50:26 jimi|ansible: \o/ 19:51:00 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 module fixups? 19:52:18 module loader is not thread-safe, nor e.g. are things that touch stdio 19:52:42 (the context is both jimi|ansible's work and the strategy reimplementation i have both are heavily threaded) 19:53:10 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 ah, yep. :) 19:54:29 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 ok, so are there any outstanding questions here, or are we clear on a path forward? 19:55:40 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 :) 19:56:33 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 deal! 19:57:01 * jborean93 would be great to do something with that with psrp 19:57:12 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 action plugin, currently its action plugins doing that work 19:57:38 kind of like how we short-circuit TE for include* now 19:57:49 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 nitzmahone: does that work for you in regards to your windows stuff too? 19:58:20 Conceptually, yes, in general, though I suspect we'll actually want to deconstruct the process in the future 19:58:33 we can burn that bridge when we come to it :) 19:58:36 This is a good way to isolate the required changes for now without blowing up the world 19:58:46 you pacifist! 19:58:47 * maxamillion gets matches and gasoline 19:59:02 dw: yes a proposal is always good just to make sure we're in agreenment on the technical aspects 19:59:10 and it gives us something to refer to in future meetings 19:59:27 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 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 te doesnt execute module, action plugin does, that is where you need the override 19:59:55 te always calls action plugin as 'action handler' 20:00:17 yeah that might be better because it will reduce duplication in all the other TE-level control stuff like retries, etc. 20:00:24 yep 20:00:26 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 gundalow: baby steps.. i'd suggest /just/ the execute_module() step. then rinse, repeat with another chunk, perhaps 20:01:01 ActionBase has execute_module, but it also calls 'configure_module' which lives in executor/module_common, that might also need changing 20:01:01 gundalow: no proposal for this IMO 20:01:13 oh, sorry, I must have miss understood, carry on 20:01:29 Just trying to make sure that any actions get captured 20:01:35 (though this IRC meeting is logged) 20:01:37 use #action 20:01:39 1) adding a new verb named "execute_module" 20:01:39 2) adding support to short-circuit execution to defer the module execution to the connection plugin 20:01:39 3) adding the mitogen connection plugin and any bits required to get it working properly with ansible-connection 20:01:41 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 configure_module (and it's children) are the part that needs to be deconstructed in the future 20:01:49 bcoca: weren't you on PTO ? 20:01:55 i came to clean up the bodies 20:01:59 :-D 20:02:01 / kick bcoca ;) 20:02:02 did i miss anything? 20:02:09 dw: the issue is that 'async' is also implemented in action plugin 20:02:13 its not as simple as that 20:02:19 jimi|ansible: as a first volley, this looks good 20:02:26 among other things like 'temp dirs', 'interpreter location' ,etc 20:02:32 #action 1) adding a new verb named "execute_module" 20:02:43 #action 2) adding support to short-circuit execution to defer the module execution to the connection plugin 20:02:45 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 #action 3) adding the mitogen connection plugin and any bits required to get it working properly with ansible-connection 20:02:56 #chair bcoca 20:02:56 Current chairs: bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima 20:02:57 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 jimi|ansible: among other things 20:03:00 but maybe not, don't know at this point 20:03:16 i have some general questions aside from the work 20:03:23 nitzmahone: also 'script' , both it and raw use 'low_level-execute' directly 20:03:31 (and reboot, and many others) 20:03:35 mitogen connection just has to implement low_level_execute 20:03:40 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 exactly 20:03:55 bcoca: i don't think so, LLE is at the action layer 20:04:13 jimi|ansible: it calls connection to actually exec 20:04:14 correct, action has LLE, and LLE points at connectionplugin.execute 20:04:14 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 its simple wrapper 20:04:21 that calls 'become' when needed 20:04:27 s/simple// 20:04:27 yes, it basically just calls conn.execute() 20:04:47 but like i said we can conditionally control what it's doing if it's not a python module 20:05:03 i.e not using 'normal' action plugin? 20:05:18 more if it's not a "new-style" module 20:05:23 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 jimi|ansible: we might need to refactor a lot from actionbase (that is where we detect module style) 20:05:53 can't remember if you implemented any filtering to prevent it from trying to fetch code impls from arbitrary namespaces 20:05:56 nitzmahone: do you know when this was tried? 2.x<->3.x support has improved immensely over time 20:06:18 yes, the extension has importer blacklisting since very early -- prerelease IIRC 20:06:20 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 I think I remember thinking in our case it needed to be whitelisting 20:06:48 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 fact gathering ... 20:07:27 that's what "free" mode was supposed to help address 20:07:28 jtanner: indeed 20:07:31 we also have memory ballooning from A) nested includes B) receiving lots of data from lots of hosts all at once 20:07:41 fact gathering ... 20:07:42 Doing it as a generic import hook may also cause problems with py2/3 namespace moves, conditional import behaviors, etc 20:08:03 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 not that i'm aware ... binary modules? 20:08:39 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 when I rewrote ansible in go, it seemed like the biggest problem was python interpreter startup time on the remote side 20:08:50 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 jimi|ansible: you rewrote Ansible in go ?? 20:09:08 it would be very nice to remove that overhead 20:09:13 bcoca: that's often not true. https://mitogen.readthedocs.io/en/dmw/ansible.html#debops-uk-to-india 20:09:14 dag: maybe :) 20:09:16 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 dag: the executor 20:09:39 jimi|ansible: Any pointers ? :) We were thinking about that at one point, also run go modules.. 20:09:55 dag: problem with go modules is binary differences between the machines 20:10:03 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 but yeah that'd be fast :) 20:10:10 jimi|ansible: a problem that can be overcome, surely 20:10:16 we also already support running go modules (that are compiled) 20:10:29 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 yes we do, that was actually added after i did the engine bits 20:10:38 dw: im talking about actual complaints we get, plenty of examples that show we have more overhead than we should 20:10:48 dw: right, it's where threading runs into a big performance hit 20:11:06 if the module returns a big chunk of data, all those threads end up processing it on one CPU 20:11:17 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 plus it's trying to manage threads on that same CPU 20:11:39 so you just get more CPU contention doing everything on a single CPU 20:11:52 or core if you will 20:11:55 One of the reasons I've been pushing for 3.5+ controller, but RHEL7... :( 20:12:06 which you need to do to keep the data consistent (either serialize or lock .. which serializes) 20:12:25 at least Python 3.6 is in EPEL now, but that's a no-go for a lot of cases 20:12:33 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 or have a 'if PY3: ' 20:13:18 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 per last year, the new strategy to legitimately (no wastage) saturate 16 CPUs 20:13:41 *to=can 20:14:49 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 ^ gundalow ? 20:15:13 you might want to postpone to next meeting, though today we have a rare 'full compliment' 20:15:36 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 dw: You got what you need to progress? 20:15:53 im happy to skip any remaining discussions -- there have been real decisions made here that help with the -connection approach 20:16:01 gundalow: we can review that other item on the agenda 20:16:11 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 #agreed proposal for `execute_module()` changes only. 20:16:44 dw: the proposal should include the 1-3 points I posted above I think 20:16:50 jimi|ansible: yep :) great 20:16:52 since it will need to tie in with other bits 20:16:56 #undo 20:16:56 Removing item from minutes: AGREED by gundalow at 20:16:39 : proposal for `execute_module()` changes only. 20:17:05 #action 4) proposal for `execute_module()` changes only. 20:17:13 there, fixed the OCD 20:17:20 ok, NEXT 20:17:32 dag: is one of your higher priority? 20:18:05 #topic ansible/ansible#42528 -- New 'forks' directive, per-play, per-block, per-task 20:18:20 #info https://github.com/ansible/ansib 20:18:23 #undo 20:18:23 Removing item from minutes: INFO by gundalow at 20:18:20 : https://github.com/ansible/ansib 20:18:31 #info https://github.com/ansible/ansible/pull/42528 20:18:52 > 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 dag: How can we help with this? 20:19:46 Yeah, so this used to be called `max_concurrency:`, but was renamed to `forks:` 20:20:17 gundalow: well, there was some discussion and I want to know if this is something that we can merge ? 20:20:43 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 the change is very straight-forward, and the functionality fixes various user problems (or at least avoid ugly workarounds) 20:21:38 So ignoring the name, do people think this is a good thing? 20:21:58 I see maxamillion is happy with it 20:22:10 I think bcoca was the one that raised issues with it. But he's on PTO. 20:22:26 gundalow: you can *see* maxamillion ? :) 20:22:26 without tests, there's very little to assert we won't break it every day. 20:22:41 abadger1999: :) 20:23:03 jtanner: if you can tell me how we could test this, I would be happy to ensure we have integration tests 20:23:17 dag: if he can, that'd be a cool trick 20:23:23 creepy, but impressive 20:23:32 the runme.sh gives you limitless possibilities 20:23:37 I see maxamillion *is happy with it* 20:23:39 runme.sh pattern* 20:24:01 maxamillion: well, I was thinking of adding something that counts the running forks 20:24:37 that information is in the debug logs 20:24:40 jtanner: ok, I'll see if we can add something 20:25:25 #info If this is accepted we will need docs. 20:25:39 Q: Where should this be documented 20:25:48 #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 Anything else on this one? 20:26:09 gundalow: yup, docs too, I'll get it done, thanks 20:27:19 dag: shall we move onto your next item? 20:28:17 sure 20:28:22 thanks 20:28:38 #topic Add the "probe" filter to dig into an array or a hash #50706 20:28:45 #info https://github.com/ansible/ansible/pull/50706 20:28:58 > This PR adds the `probe` filter in native filters. 20:28:59 So originally this filter was named "dig", but obviously that's quite confusing 20:29:16 heeh, yes that would have been confusing 20:29:31 so putting naming things aside 20:29:44 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 Is the idea sound and solving a useful problem 20:29:54 s/heard/hard 20:30:52 you mean you don't like to write a series of 4 nested |default() statements? 20:31:18 #chair agaffney 20:31:18 Current chairs: agaffney bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran tima 20:31:23 json_query can currently be used to do something similar 20:31:48 but that requires another dep to be installed on the controller 20:31:52 I get the problem that it's trying to solve, but something just "feels wrong" about it to me 20:32:55 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 (likely the error is before it ever gets to default()) 20:33:40 agaffney: exactly 20:33:46 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 . syntax would be nice. sivel has a simple implementation https://gist.github.com/sivel/f76b62b548631f06b95532e24ec7d538 20:34:19 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 so if someone else proposes the same thing, there's a need 20:35:43 could we somehow define Undefined[anything] to also be Undefined? 20:35:45 https://github.com/ansible/ansible/issues/35992 20:35:46 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 has an example of how to do it w/ json_query 20:36:27 a custom Undefined class should be trivial to implement 20:37:02 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 we discussed in a previous conversation that this "probe" (formerly dig) needed to implement dot syntax, instead of discreete arg syntax 20:37:58 agaffney: I didn't think we could do that, but sure, if nobody objects I guess we can ;-) 20:38:00 and I see shertel mentions something similar above 20:38:17 sivel: where was this previous conversation ? 20:38:39 dag: we discussed it in our internal core triage meeting 20:38:56 sivel: ok, just ignore me feeling left out :-D 20:38:57 I believe shertel was going to perform a full review indicating that requirement in the near future 20:39:07 sorry we haven't gotten around to it yet 20:39:18 but maybe agaffney's solution is better ? Changing the behaviour ? 20:39:23 http://jinja.pocoo.org/docs/2.10/api/#undefined-types 20:39:32 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 jinja offers a few default Undefined implementations and allows you to provide your own 20:39:57 agaffney: we are using Undefined() like that, but from a filter 20:40:08 I cannot necessarily say without seeing how it works. I've never tried to influence the undefined behavior in that manner 20:40:16 #chair sivel 20:40:16 Current chairs: agaffney bcoca cyberpear dag dw felixfontein gundalow jimi|ansible jtanner maxamillion nitzmahone sdoran sivel tima 20:40:25 agaffney: the question is whether we want to change the existing Jinja behaviour in this case 20:40:32 I'd think we could just subclass jinja2.Undefined and override __getitem__() and __getattr__() 20:40:56 dag: afaik, it shouldn't break any existing stuff 20:40:57 +1 20:41:10 (no need to be sorry, I was only half joking feeling left out ;-)) 20:41:47 agaffney: if nobody is depending on that specific behavior, it's hard to tell 20:42:12 agaffney: this would be my preferred solution to be honest. 20:42:16 dag: I doubt they are, since exceptions from jinja aren't catchable from within a template 20:42:38 ignore_errors ? 20:42:42 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 it would require also understanding how it interacts with error_on_undefined_vars 20:43:32 you would still get an Undefined object in the end, so I wouldn't think anything would change there 20:43:48 it would just change the behavior where an exception is thrown for nested undefined values 20:44:44 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 we are already overriding the default jinja undefined behavior by having it use jinja2.StrictUndefined 20:45:21 agaffney: best thing is to try it out and see what it brings 20:45:32 yeah, it should be pretty easy to test 20:45:51 let's try it as an alternative 20:48:03 I'm putting together a branch to test it now 20:48:20 agaffney: ace, thanks 20:48:37 * gundalow realises we are at 108 minutes 20:50:38 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 nice 20:51:04 Shall we continue discussion on PR/#ansible-devel and #endmeeting? 20:51:30 #info https://github.com/ansible/ansible/pull/51768 [WIP] Custom jinja Undefined class 20:51:47 Yup :) 20:52:01 Great meeting, thank you everybody for your time 20:52:06 #endmeeting