16:02:23 <akasurde> #startmeeting Ansible VMware Working Group Meeting
16:02:23 <zodbot> Meeting started Mon Feb 25 16:02:23 2019 UTC.
16:02:23 <zodbot> This meeting is logged and archived in a public location.
16:02:23 <zodbot> The chair is akasurde. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:02:23 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
16:02:23 <zodbot> The meeting name has been set to 'ansible_vmware_working_group_meeting'
16:02:27 <akasurde> Hello Everyone
16:02:37 <alongchamps> hi akasurde
16:03:27 <dericcrago> hi
16:03:35 <akasurde> #chair dericcrago
16:03:35 <zodbot> Current chairs: akasurde dericcrago
16:03:41 <akasurde> #chair alongchamps
16:03:41 <zodbot> Current chairs: akasurde alongchamps dericcrago
16:03:44 <akasurde> hi alongchamps
16:03:46 <akasurde> hi dericcrago
16:03:53 <akasurde> dericcrago, long time
16:04:11 <akasurde> just saw your updates on vmware_tools connection plugins
16:04:14 <akasurde> PR
16:04:26 <dericcrago> yeah, I had missed the previous comments from a couple of weeks ago
16:04:50 <moshloop_> hi
16:05:07 <dericcrago> I hadn't intended to abandon the PR, it works, just need more guidance to get it across the finish line I guess
16:05:23 <alongchamps> what's the link to the PR?
16:05:41 <dericcrago> https://github.com/ansible/ansible/pull/47072
16:06:06 <alongchamps> ty
16:06:30 <akasurde> #chair moshloop_
16:06:30 <zodbot> Current chairs: akasurde alongchamps dericcrago moshloop_
16:06:34 <akasurde> hi moshloop_
16:11:11 <akasurde> dericcrago, let me know if you need anything from my side
16:11:27 <akasurde> I would love to see this PR in upstream <3
16:11:41 <moshloop_> Can you have a look at my PR's and let me know what I still need todo ? https://github.com/ansible/ansible/pulls/moshloop
16:12:34 <dericcrago> thanks akasurde, yeah, you're not the only one :)
16:12:36 <akasurde> moshloop_, yes, sure
16:13:55 <akasurde> moshloop_, sorry I have a bunch of things on my plate so could not provide time to review your PRs, but worry not I will take a look
16:14:48 <akasurde> moshloop_, https://github.com/ansible/ansible/pull/44885 For this pdellaert provides some feedback
16:17:21 <akasurde> dag, are you around
16:18:01 <dag> I am
16:18:05 <moshloop_> for 44885 - will do
16:20:01 <akasurde> dag, can you please provide little insight for https://github.com/ansible/ansible/pull/44832 so that moshloop_ will be proceed
16:20:16 <akasurde> *will proceed
16:21:25 <dag> akasurde: what is no clear from my last comment ?
16:21:34 <dag> https://github.com/ansible/ansible/pull/44832/files#r218247626
16:21:47 <akasurde> it is confusing to me :)
16:22:40 <akasurde> nevermind I get it now
16:23:10 <dag> they replace fetch_url() by open_url()
16:23:31 <dag> and now they get issues when they get `except Exception`
16:23:48 <dag> so in essence, they get something back, don't know what it is, do some stuff with it, and now that seems to fail
16:24:02 <dag> and they want to encapsulate that in another-try-block
16:24:29 <dag> which makes it on big chunk of sh*t, no one knows exactly what has happened, and why
16:24:54 <akasurde> ok
16:25:01 <dag> so we have to bring it back to the basics, only catch known exceptions, and handle the known exceptions as they should be handled
16:25:01 <moshloop_> I think there are 2 issues: 1) swallowing errors - which is an easy fix
16:25:19 <moshloop_> 2) refactoring the code to handle errors correctly
16:25:32 <akasurde> moshloop_, I would go with 2
16:25:34 <dag> moshloop: I would do away with the exception handling code
16:25:48 <dag> because I think that is no longer necessary once we know the exception
16:25:52 <moshloop_> I need to prioritize my time
16:26:07 <dag> I think that code is the result of a myriad of exceptions that do not return the same error information
16:26:24 <dag> so someone made it work with his exception too
16:26:27 <moshloop_> and I just can't justify make a few lines of code a little bit neater
16:26:34 <moshloop_> of something I did not write
16:27:03 <dag> moshloop, remove the except Exception: completely, and try to make this code fail
16:27:04 <moshloop_> I am just trying to make something bad, a little less bad
16:28:01 <dag> I don't agree that you make it less bad ;-) it depends on the viewpoint of what is correct
16:28:08 <dag> it's further away from how it ought to be
16:28:14 <moshloop_> What do you mean by try and make this code fail? Write integration tests for failure scenarios ? manually test failure conditions ?
16:28:16 <dag> but it will behave in your use-case better :)
16:28:31 <dag> test for possible failure conditions
16:28:38 <moshloop_> I don't see how swallowing an error can ever be considered better
16:28:52 <dag> (e.g. target an unknown host, wrong port, different service, etc.)
16:29:13 <dag> swallowing an error ?
16:29:42 <moshloop_> That code swallows errors if you have any except a KeyError
16:29:46 <dag> if the "except Exception" is removed, you don't swallow it, you make it fail with a traceback
16:30:14 <dag> I want to get rid of the stuff before that
16:30:25 <moshloop_> I am happy to just remove the exception , blind
16:30:34 <dag> https://github.com/ansible/ansible/blob/b42ef9f8496926c47c3df0ef87d29a87456e53e0/lib/ansible/modules/cloud/vmware/vsphere_copy.py#L168-L176
16:30:36 <dag> this should all go
16:30:39 <dag> IMO
16:31:08 <dag> and then fix each individual traceback/exception
16:31:11 <akasurde> dag, what if after removing this ?
16:31:18 <akasurde> ok
16:31:32 <dag> try to induce as many failure-cases as we can
16:31:33 <moshloop_> I don't have time to fix each individual traceback and exception
16:31:46 <dag> and we will learn from users if there are any other
16:31:48 <akasurde> you mean to say that you want to individually fix the exception as and when they occur rather than blanketing them
16:32:07 <alongchamps> that sounds complicated...
16:32:11 <akasurde> moshloop_, for now, just remove the whole exception block as dag say
16:32:12 <dag> akasurde: yes, and I want to handle them specific to the exception, rather than what we do there
16:32:23 <akasurde> will see what happens
16:32:36 <dag> alongchamps: it's hard, but we have to start from scratch IMO
16:32:37 <moshloop_> I will remove the block
16:32:52 <akasurde> dag are you OK ?
16:32:57 <dag> the other option is to look back in history to see what it was created like this
16:33:07 <dag> and that history probably gives some insight in the failure cases
16:33:15 <dag> hopefully with traceback
16:33:34 <dag> but since fetch_url() was replaced with open_url() it will likely not be the same anyway
16:33:52 <dag> akasurde: sure
16:34:15 <dag> if moshloop doesn't want to spend the effort, just remove the blanket exception, and we'll see what is reported
16:34:32 * dag no longer has an environment to test this myself
16:36:29 <dag> also not that "exception=traceback.format_exc()" is not necessary as Ansible does this automatically
16:36:40 <dag> I see this module also doing it
16:37:41 <moshloop_> I am happy to remove the block if it will get merged, otherwise I will just close the PR~
16:38:06 <akasurde> moshloop_, Ok
16:38:41 <moshloop_> great, next up https://github.com/ansible/ansible/pull/47930/files
16:39:45 <akasurde> on line 2017, I see some values are missing
16:39:52 * dag is trying to find the changes in ansible-modules-extras :-(
16:40:43 <dag> https://github.com/ansible/ansible-modules-extras/commit/1e1855080526fa4c7c930a66990dbb44a732ce7c#diff-601801af94ec1555ce40e0a5e8624715
16:42:27 <dag> So looking at that, we probably don't have detailed information about known exceptions :-/
16:43:21 <dag> and I don't see any other invocation of open_url() do the same thing
16:44:06 <akasurde> ok
16:45:55 <dag> I only see 2 kinds of exception handling statements for open_url()  HTTPError and Exception (the latter simply does self.module.fail_json(msg=to_native(e))
16:47:34 <dag> so I would got with that
16:48:09 <dag> ah, also except (HTTPError, httplib.HTTPException) as http_exception:
16:49:55 <dag> lib/ansible/modules/remote_management/cpm/cpm_user.py has another implementation, with HTTPError, URLError, SSLValidationError and ConnectionError
16:50:21 <dag> they provide different error msg, but always adds to_native(e)
16:54:21 <dag> hmmm, so the majority of modules still use fetch_url(), but the ones using open_url() have a few variations, I would got for what cpm_user module does
16:56:37 <akasurde> #action moshloop_ to fix 44832 with cpm_user like exception
17:00:09 <moshloop_> I will retest 47930 and post my exact config to reproduce
17:01:37 <akasurde> moshloop_, Could you please paste the setup in comment section of 47930 so that I can reproduce that in my environment
17:01:38 <akasurde> ?
17:01:51 <moshloop_> yip
17:03:02 <akasurde> Thanks
17:04:18 <moshloop_> For https://github.com/ansible/ansible/pull/47936 - besides for the integration test failures and duplicate function,  what else needs to be done ?
17:05:03 <moshloop_> Not exactly sure how the code would work with a normal (non distributed) vswitch
17:07:09 * alongchamps looks up code that does this very thing in vRealize Orchestrator
17:07:14 <alongchamps> that I wrote
17:07:43 <alongchamps> I want to say that for the distinction between DVS portgroups and standard switch portgroups I had to have two different blocks of code
17:09:05 <akasurde> moshloop_, I will check 47936 and put in comments
17:09:40 <moshloop_> at first glance it doesn't look like it supports non DVS portgroups with a vlan specified ?
17:12:09 <akasurde> alongchamps, Are you ok with merging this - https://github.com/ansible/ansible/pull/52554 ?
17:12:10 <alongchamps> agreed
17:12:18 <alongchamps> for moshloop_'s comment!
17:12:27 <akasurde> alongchamps, yup
17:12:48 <alongchamps> yes akasurde, I'm good with merging that. I've been using it actually in my testing for the other issue I have open and it's been working for me
17:13:06 <alongchamps> let's merge/close 52554 and 52526
17:13:11 <akasurde> OK
17:13:11 <akasurde> OK thanks for the feedback
17:13:18 <alongchamps> I can add a comment on 52554 with my s/o
17:14:53 <akasurde> alongchamps, merged
17:15:36 <moshloop_> I also have a couple more modules in the wings - which ones do you think would be worthwhile creating PR's for?
17:16:06 <moshloop_> vmware_vm_groups - (we use them for vm placement for licensing reasons)
17:16:40 <moshloop_> vmware_vm_tags (super slow, but works)
17:16:41 <alongchamps> moshloop_: I definitely have a use case for that
17:16:47 <alongchamps> for vmware_vm_groups
17:17:12 <moshloop_> vro_workflow (basically a wrapper around the HTTP api that makes passing args easier)
17:17:52 <alongchamps> does the vmware_vm_groups functionality overlap with vmware_drs_group and vmware_vm_host_drs_rule?
17:17:56 <alongchamps> @moshloop_
17:18:47 <akasurde> what is vmware_vm_groups ?
17:19:47 <akasurde> vro_workflow seems interesting me
17:20:22 <moshloop_> some overlap with those modules - although I only add a single VM, not specify the entire list
17:20:22 <akasurde> I don't have VRO setup. No public download for VRO for evaluation
17:20:48 <moshloop_> spec = vim.cluster.ConfigSpecEx(groupSpec=[vim.cluster.GroupSpec(info=group,operation="edit")])
17:20:48 <moshloop_> wait_for_task(cluster.ReconfigureEx(spec=spec, modify=True))
17:20:56 <akasurde> moshloop_, I would say add everything you would like to contribute
17:20:57 <alongchamps> there are facts modules for pulling back the whole thing when it comes to DRS groups as well
17:21:00 <dericcrago> I miss VMware Orchestrator
17:21:15 <alongchamps> I used vRO a lot...then discovered Ansible
17:21:25 <alongchamps> and it's about 500% easier
17:22:02 <dericcrago> yeah, I haven't used orchestrator since the vsphere 4 days
17:22:56 <moshloop_> the new cloud based offering isn't actually too bad  (on the face of it)
17:23:14 <alongchamps> vRO in the cloud?
17:23:18 <alongchamps> as in managed
17:23:33 <dericcrago> seemed like it had a chicken / egg problem of not catching on because it didn't have a community and not having a community because it hadn't caught on yet
17:24:54 <moshloop_> it links to a vRO instance, but allows for creating cloudformation like templates for creating vm's - and then attaching attaching code / vro tasks to lifecycle events
17:25:01 <moshloop_> https://blogs.vmware.com/management/2018/08/introducing-cloud-automation.html
17:26:12 <alongchamps> I want to say those are different products
17:29:00 <moshloop_> yes and no
17:29:51 <alongchamps> I wonder if vRO is under the hood powering a lot of it
17:31:50 <alongchamps> did we want to go back to 47936 for the DVS portgroup stuff?
17:32:10 <alongchamps> it looks like the module overall only supports DVS portgroups and not standard switches
17:33:00 <alongchamps> some of the find_obj function calls specify DVS Portgroups so if 47936 makes it more efficient to find those, then it seems ok on the surface (just saying that because I'm not in a position to test it)
17:34:22 <moshloop_> I think the problem is they do an N+1 lookup, my solution is inferering the details from the DVS name (I should probably comment as such)
17:35:17 <alongchamps> for efficiency, we'd only want to find that PG object once
17:35:56 <alongchamps> the code I found basically did the same; loop over all the DVS portgroups until finding one that matches our VLAN
17:38:07 <alongchamps> at the moment I need to go and grab some lunch
17:38:18 <alongchamps> not sure how much more is on the agenda
17:39:35 <akasurde> We don't anything else on the agenda
17:39:50 <akasurde> I think we can close the meeting for now.
17:44:48 <akasurde> #endmeeting