19:00:51 <sdoran> #startmeeting Ansible Core public IRC meeting https://github.com/ansible/community/issues/584
19:00:51 <zodbot> Meeting started Tue Jan 12 19:00:51 2021 UTC.
19:00:51 <zodbot> This meeting is logged and archived in a public location.
19:00:51 <zodbot> The chair is sdoran. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:51 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:00:51 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting_https://github.com/ansible/community/issues/584'
19:01:00 <Shrews> o/
19:01:05 <felixfontein> \o
19:01:11 <nitzmahone> yo
19:01:26 <sdoran> #chair Shrews felixfontein nitzmahone
19:01:26 <zodbot> Current chairs: Shrews felixfontein nitzmahone sdoran
19:01:41 <sdoran> #topic hash_merge behavior
19:01:47 <jerrac> o/ Is that how we say present? :)
19:01:55 <sdoran> That'll work.
19:01:57 <felixfontein> jerrac: it's one way :)
19:02:00 <sdoran> #chair jerrac
19:02:00 <zodbot> Current chairs: Shrews felixfontein jerrac nitzmahone sdoran
19:02:00 <felixfontein> #chair jerrac
19:02:00 <zodbot> Current chairs: Shrews felixfontein jerrac nitzmahone sdoran
19:02:13 <sdoran> jerrac: What would you like to discuss about hash_merge behavior?
19:03:13 <jerrac> So, `hash_behavior=merge` is something saves me, and my coworkers, a LOT of time.
19:04:31 <jerrac> Deprecating it will make a lot of extra work for us. Which is one thing when it makes sense, but so far I have not seen any arguments against it backed by evidence.
19:05:29 <jerrac> I have covered most of what I'd want to say in the github issues. What more would you like from me?
19:07:00 <bcoca> well, i see 3 issues, a) the deprecation itself b) if we do go forward, a migation path for users c) documenting this path
19:08:01 <bcoca> b) can further be divided into 'path that preserves teh funcionality in other ways' OR 'force users to change their data to conform with new reality'
19:08:37 <felixfontein> I'm not sure how a patch that preserves the functionality in other ways would look like. at least not without deeper modifications of ansible-core itself
19:08:50 <felixfontein> s/patch/path/
19:09:18 <sdoran> While is good is some situations, more often it is confusing and makes is very difficult to fully understand the final merged result of complex data sets.
19:09:23 <bcoca> there is full compatiblity and partial, there are alredy 2 things for partial (vars plugin and update to include_vars)
19:10:15 <felixfontein> bcoca: I guess everything that's not full compatibiltiy requires rewriting and checking large chunks of roles/playbooks for affected users, since uses could be hiding anywhere
19:10:17 <bcoca> to enumerate the reasons against it: a) makes plays/roles that rely on it non portable b) its another branch of rarely used code to maintain (but i can say this of many things in ansilbe)
19:10:57 <bcoca> felixfontein:   option={{varthatisdupeinsources}} <= there is really no way to know if they rely on ovwrite or merge
19:11:17 <jerrac> sdoran: Are you talking about for users who create lots of config, or in the code of Ansible itself?
19:11:32 <sdoran> And when Ansible doesn't "merge things correctly" we get asked to "fix" the merge behavior, which is asking us to play with very unpredictable fire.
19:11:38 <felixfontein> bcoca: yes, not without tracing back every way this expression can be evaluated. so it's essentially rewriting, to be on the safe side.
19:12:44 <bcoca> specially when you add extra vars into mix
19:12:47 <sdoran> We prefer things to be less magic and more straightforward. Too much internal magic makes troubleshooting difficult.
19:13:02 <bcoca> sdoran: yet we are adding 'reflections'
19:13:11 <sdoran> s/straightforward/explicit
19:13:11 <felixfontein> sdoran: do you know how much this impacts paid Support? for community questions, one can simply say "well you chose to use this, now suffer", but I guess that's not really an option for paid Support.
19:13:13 <Shrews> jerrac: i'm curious as to what your use case is that is saving you and your coworkers a lot of time?
19:13:39 <bcoca> felixfontein: no, i dont think anyone asked paid support about this decision
19:13:55 <jerrac> How does it "makes plays/roles that rely on it non portable"? Every role I've written could be used without it if you just copy/pasted a ton of config around...
19:14:00 <felixfontein> bcoca: at least something :)
19:14:10 <nitzmahone> It's eliminating the game of whack-a-mole that inevitably occurs every time someone touches any code around vars...
19:14:53 <nitzmahone> My personal favorite argument against it is from the guy that originally wrote it: https://www.reddit.com/r/ansible/comments/a7x92q/in_which_case_hash_behaviourmerge_can_become/
19:15:05 <bcoca> jerrac:   packages: a, b c ; packages: e,f,d . roles that use packages will in one case install 6 in the other 3, if rest of role depends on full list...
19:15:18 <bcoca> nitzmahone: that is already linked in the original pr
19:15:26 <jerrac> Installing packages on most systems, but supporting that one snowflake that can't have a certain package. Without merge, I'd have to paste the whole list of packages into that one servers config, then remember to update it every time I added/removed a package from the list.
19:15:41 <bcoca> felixfontein: just asked, a few complained, but most won't until it is actually remove and stuff breaks
19:16:20 <felixfontein> bcoca: hmm, that will probably be 'fun' as well.
19:16:39 <bcoca> also  firewall rules (where i've seen this most commonly used) role a adds 10 rules, role b adds 5 and role C uses same var to create all rules
19:16:45 <bcoca> felixfontein: yeppers
19:16:47 <jerrac> Another place would be my apache config, I can use a set of defaults on all my apache servers, then add or override as needed depending on what I'm hosting. All without having to duplicate config very much.
19:17:06 <bcoca> ^ any form of composite config
19:17:15 <nitzmahone> There are still lots of ways to accomplish reuse without relying on a global merge behavior
19:17:19 <felixfontein> bcoca: I guess paying customers won't like "rewrite your playbooks/roles"
19:17:29 <bcoca> nitzmahone: true, the problem is existing users, not new imho
19:17:42 <bcoca> felixfontein: they tend to YELL on that response
19:19:16 <felixfontein> how about not removing the feature, but making it mandatory that users add i_will_never_ask_support_about_hash_merge=true to their ansible.cfg to be able to use it?
19:19:59 <bcoca> its not like we respond to support of it now
19:20:43 <jerrac> nitzmahone: I believe felixfontein mentioned a few ways in one of the issues, I checked, and they'd only work part of the time. Also, it seemed like the preferred method was implementing complicated filters. And that syntax is not something I find easy to parse.
19:21:58 <jerrac> I'll also just say that all my Ansible config relies on it. I'd guess I'd be spending about 4 to 5 more times as much time on configuration if I didn't have it.
19:26:34 <felixfontein> I think the main problem of keeping the deprecation is that it is very frustrating to current users of the feature, and the big question is whether this weights more or less than the pain caused by abusing that feature
19:27:25 <jerrac> Could you show some examples of the pain caused by abusing it? So far I have been unable to find any.
19:27:38 <felixfontein> while I don't really like the feature myself and its removal won't have any effect on me, I think it would be better to keep it, but probably add some more (explicit / big) warnings to deter new users from starting to use it
19:28:29 <nitzmahone> The bigger problem is that it's never had adequate test coverage, and is likely broken all over the place already- just folks haven't noticed
19:28:46 <nitzmahone> (because the common use cases for it are fairly narrow)
19:29:13 <nitzmahone> But it's also a cause of future performance issues, since any merged value can't "stop" looking at the first found value in the hierarchy
19:29:58 <felixfontein> nitzmahone: is it a performance issue for all code-paths, or only for code-paths where this feature is enabled?
19:30:33 <bcoca> nitzmahone: we always look at first found, independant of this feature
19:30:38 <nitzmahone> It'd only be an issue when the feature's enabled, but there's extra conditional stuff required anyplace we're looking things up
19:30:41 <bcoca> even removing it does not remove the merge we do
19:30:56 <bcoca> the feature does add overhead (merging is more expensive than overwriting)
19:31:03 <bcoca> but only on that codepath
19:31:52 <nitzmahone> (sorry, was talking about performance issues on the nested immutable vars thing we've kicked around)
19:35:46 <sdoran> We call `combine_vars()` in a lot of places in the stack, which calls `merg_hash()`. And it's recursive.
19:36:12 <sdoran> It has cyclomatic complexity of 14. I highly doubt it is adequately tested (as nitzmahone  pointed out earlier).
19:36:49 <bcoca> sdoran: again, we can apply that to a large part of our codebase and we are not deprecating all of those
19:37:08 <felixfontein> sdoran: that's mainly because of the extra options which are not used by combine_vars
19:37:09 <sdoran> I know, but I'm just picking on this one for now. ;)
19:37:49 <bcoca> if we had comprehensive plan to deal with those issues in general is one thing, picking on this one for very common issues in our code, seems unfair
19:38:07 <bcoca> also, that is implementation, not because of teh feature itself
19:38:46 <nitzmahone> Right, but the feature itself is actively harmful to a number of things we try to promote (reuse, transparency)
19:39:16 <nitzmahone> Again, not that we've gotten everything perfect in those areas, but we're trying to move in better directions
19:39:20 <bcoca> nitzmahone: again, many features are like that ... roles ...
19:39:46 <bcoca> nitzmahone: and if we had comprehensive plan, easier to justify, but not picking on the single feature for reasons we should deprecate 1/2 of existing ones
19:39:47 <felixfontein> vars precedence ;)
19:39:58 <jerrac> nitzmahone: How is it harmful to reuse and transparency?
19:40:01 <nitzmahone> ... and if they weren't so widely used, there've been loud voices trying to kill those too that we've managed to quell... :D
19:40:06 <bcoca> so i think should narrow down to reasons specific to this feature
19:40:20 <bcoca> felixfontein: he .. dontgetmestarted
19:40:44 <nitzmahone> Content that requires hash merging doesn't work for people that don't have it turned on, and content that doesn't account for hash merging can break in "interesting" ways.
19:41:06 <nitzmahone> (eg, pretty much anything on Galaxy)
19:41:14 <bcoca> nitzmahone: yes, that is reason a) i gave above,  makes it incompatible with sharing
19:41:20 <bcoca> and gave several examples
19:41:55 <nitzmahone> Transparency is affected by the fact that just setting a var somewhere doesn't give you the full picture of what that var's value is anymore
19:42:14 <jerrac> bcoca: Sorry, I can't quite get your example to make sense... :\
19:42:18 <bcoca> that is true with or without the setting
19:42:18 <nitzmahone> (even more so than already is)
19:43:10 <bcoca> jerrac:  you have a firewall role that uses a var to set rules, role A and role B use it as dependency, role A expect 'merge' behaviour , role B doesn't , role B will get all its rule's implemented, role A only the 'last update to var'
19:43:37 <jerrac> For transparency, that seems like something users need to be responsible for. You should structure your project in a manner that lets you keep track of what is what.
19:43:42 <bcoca> so role A is not 'shareable'
19:43:48 <jerrac> merging helps me with that a lot.
19:44:49 <jerrac> bcoca: Ah, ok, I can see that.
19:44:49 <jerrac> How many roles actually depend on each other like that, though?
19:45:23 <sdoran> More than 0. 😉
19:45:49 <nitzmahone> People abuse role hierarchies plenty- I've seen 3-5 layers pretty commonly
19:45:49 <felixfontein> definitely. and usually not shareable ones :)
19:45:52 <bcoca> jerrac: actually, many, luckily most 'shared roles' avoid merge behaviour, but there have been people tripped by it
19:46:47 <sdoran> The main goal of removing this is to remove the ability to create something obscure and not easily understood from reading the config. It's a higher level goal, though I was trying to come up with some lower level technical ones.
19:47:10 <bcoca> one basic problem: we dont know how many use/rely on this, since we dont get feedback for every ansible run, so moslty we guess a) this is in use by small portion of user base b) that use can be changed and the data structures it relies on are not required
19:47:19 <felixfontein> life would be a lot simpler if roles would be more encapsulated, so they can only get input and output through very specific variable names, and cannot leak state in other ways. but that's not how roles work with now.
19:47:32 <jerrac> And I'd argue that removeing it would make it harder to understand the config....
19:47:35 <sdoran> We have _plenty_ of ways to write complicated Ansible without hash merging.
19:47:46 <bcoca> - "Some users prefer that variables that are hashes (aka 'dictionaries' in Python terms) are merged.
19:47:47 <bcoca> This setting is called 'merge'. This is not the default behavior and it does not affect variables whose values are scalars
19:47:49 <bcoca> (integers, strings) or arrays.  We generally recommend not using this setting unless you think you have an absolute need for it,
19:47:50 <bcoca> and playbooks in the official examples repos do not use this setting"
19:47:53 <bcoca> ^ part of the docs of the entry
19:48:00 <bcoca> we can add more WARNING signs there
19:48:27 <felixfontein> +1 for more warnings. also in bold, please :)
19:48:41 <sdoran> jerrac: I think you may have one of the only clean uses if this feature. 😄
19:48:52 <bcoca> felixfontein: no bold, but CAPS MIGHT WORK
19:49:00 <jerrac> Also document concrete examples of how it can go wrong.
19:49:11 <felixfontein> sdoran: there could be a lot more. usually you don't hear from them, because they don't complain or ask questions :)
19:49:23 <bcoca> examples in config entry .. dont wor4k well, maybe FAQ entry
19:50:00 <felixfontein> as bcoca said, it will mostly become visible once the feature is actually removed and suddenly things stop working. then people will start yelling. the question is how many of them.
19:50:22 <bcoca> and how loud
19:50:24 <jerrac> The most frustrating issue with all of this has been that no one has done that. The best I've seen so far is what bcoca said earlier. To
19:50:25 <nitzmahone> (and can we figure out a better replacement in the meantime ;) )
19:50:34 <jerrac> s/S/
19:50:47 <jerrac> my poor irc skills show up...
19:50:48 <felixfontein> nitzmahone: usually it's a good idea to have a better replacement before deprecating things :)
19:50:55 <bcoca> jerrac: maybe as the devs and having supported ansible for long time its hard for us to realize its not that obvious to people
19:51:05 <bcoca> felixfontein: or at least a migration tool/path
19:51:34 <jerrac> bcoca: yep.. I have to remember that whenever I'm helping my parents with their computers....
19:51:41 <bcoca> i'll admit the way we did it, with basically no more info than the original requestor saying 'my bad' ... is far from optimal
19:51:53 <jerrac> Thanks. :)
19:52:10 <bcoca> jerrac: we all have a context, its hard to remember its not always the same one
19:52:17 <nitzmahone> dictionary vars are becoming increasingly common for a lot of things, so as time goes on, the likelihood for this to break for common use cases increases as well
19:52:39 <tadeboro> Maybe a blog post such as the one about Windows not being supported as control node would help? So one can point angry people somewhere?
19:52:54 * nitzmahone wrote the last one of those- NOT IT ;)
19:53:43 <bcoca> nitzmahone: increasingly? ... i would argue they've been common since 0.2
19:53:55 <sdoran> We have about seven minutes left in our meeting for today. Do we want to move to another topic with our remaining time or just run out the clock?
19:54:02 * bcoca abused dictionary variables as a user with 0.4
19:54:10 <nitzmahone> Right, but things like cloud/networking/resource stuff all use dict vars heavily, where most older modules do not
19:54:20 <bcoca> do we want to vote? do we have specific options we want to follow?
19:54:32 <bcoca> nitzmahone: yes, they have, for years now
19:54:36 <sdoran> Tons and tons of dicts in those contexts, for sure.
19:54:48 * bcoca pokes at 'parameters' option in old cloud/networking moduels
19:55:17 <bcoca> also reason why args_spec has 'suboptions'
19:55:46 <nitzmahone> (which it didn't for most of Ansible's life)
19:55:51 <nitzmahone> networking added it for real
19:56:03 <bcoca> we are still 'adding it'
19:56:08 * felixfontein likes suboptions :)
19:56:29 <felixfontein> maintaining docker_container would be even more a PITA without it
19:56:45 <bcoca> but its been there for a while already, my point is that we dont have a sudden surge .. we are waay past the start of the surge of dict var usage
19:56:55 <nitzmahone> I don't know where we go from here- if folks want to explore more targeted ways of getting merge behavior, we've got ~ 18mo before this thing dies
19:57:50 <nitzmahone> But it seems like it can't be replaced with a more targeted option without a pretty deeply-embedded core behavior
19:57:58 <bcoca> i see several options a) rescind teh deprecation for now, review when new options become available, b) keep deprecation, but create docs on migration path and possibly helper tools
19:58:09 <nitzmahone> (since most things can't see the entirety of unmerged vars to simulate that behavior)
19:58:27 <bcoca> c) create varsmanager plugins
19:59:16 <felixfontein> while c) sounds interesting, it is probably a lot of work and will make parts of core a lot harder to adjust in the future
19:59:24 <nitzmahone> ayup
19:59:55 <bcoca> 'c' was a joke, basically describing what nitzmahone said above and if someone wants  to do it, i'll take 2-5 yr sabatical and come back after the fire is out
20:00:23 * nitzmahone has to duck to another mtg
20:00:25 <sdoran> We're at time for today.
20:00:38 <sdoran> Thank you everyone for attending and participating in the discussion.
20:00:40 <sdoran> #endmeeting