CiscoDevNet/python-viptela

device_templates.attach_to_template polls w/ utils.waitfor_action_completion

jeremypng opened this issue · 9 comments

I think the SDK is trying to do too much here. Instead of providing a wrapper for the API it is building in logic that the user may or may not actually want. Would anyone be opposed if I submitted a pull request to remove the waitfor_action_completion from this function and just return the actionId?

Then if someone wants to know the status of the actionId they can request it, loop it, or whatever in their own code.

Just for a little background on this repo, we have not taken the view that the SDK should simply be Python bindings for the vManage API, rather that it is OK to put additional logic in places where it makes sense. That said, this repo also houses the CLI and the Ansible modules and they are probably the most widely used abstractions of the SDK. The poll-for-action-completion logic could be moved there instead (and would need to be completed and tested for any PR that changes the attach logic due to the way we version things at the moment.)

On the other hand, will it be a common use case that we would want to attach a device to a template and not know whether it completed or not? I don't really know the answer to that and could certainly be convinced that it would be a common use case. In our use case (primarily automation for DevOps CI/CD) it is not something we need.

I guess I want a snappy API call that doesn't block. I'm using this in a function passed to a Django-RQ worker. The default rqworker timeout on Netbox where I'm using this is 300 seconds. Most of the time the actions on VManage complete in that time, but I don't want to depend on it and not get the actionId back so I can easily go check it later. Once I have the actionId, an RQ-Scheduler job runs periodically to pull the status of any outstanding actions (on DNA Center or Vmanage). If no one else has that need, I can just modify it before I install it. It's a small change.

What about just adding a new parameter wait that defaults to true and does the waitfor_action_completion - that way your code can still use the library without modification, but none of the existing code needs to be changed.

That approach works for me. If we do this PR, we should probably look at all areas waitfor_action_completion is used and do something similar. @jeremypng - are you up for submitting a PR that adds a wait parameter with default true in these places?

Sure, I'll look through and find the other spots it is used and add wait to those as well.

I also need to be able to query the actionId's status on demand. I could add that function to utilities alongside the current waitfor_action_completion. Would you guys rather have that in a separate PR?

Would you guys rather have that in a separate PR?

I'm OK with it being in the same PR because it is something we should have for when wait is false. But @jlothian might have other feelings on that.

I'm with @jasonking3 - the changes are pretty well related, and I don't think it would make the review unwieldy to have it all lumped together.

Ok, I found several to add wait to but found these 3 where it doesn't make sense:

  1. ansible\vmanage.py - This already has a vmanage_params which defines wait and takes care of this from Ansible
  2. apps\clean.py - This would fail to delete the policy if the action didn't complete. So no need to add it here.
  3. data\template_data.py - The import_attachment_list is a long running batch job that wants the results. So no need to add it here.

There are the spots I am adding it:

  1. api\certificate.py - install_device_cert, push_certificates
  2. api\device_templates.py - reattach_device_template, attach_to_template

Then add a function called get_action_status that returns the same data structure as waitfor_action_completion, but instead of looping will just return whatever it gets on the first request.

Let me know if that sounds good and I'll test the changes and then submit a PR.

This sounds reasonable to me. It makes sense that the only place we should need this is in the api methods. I'm fine with the get_action_status name. 👍