aristanetworks / cvprac

Other
46 stars 47 forks source link

save_topology vs add_temp_action #160

Open mharista opened 3 years ago

mharista commented 3 years ago

Details from Tamas:

where I apply an image to a device but I had to use a new function, which is a copy of apply_image_to_element() but without creating addTempAction In an environment running scripts simultaneously it is possible to hit an issue where if saveTopology is called at roughly the same time, CVP generates multiple tasks because the REST APIs do not currently support concurrency.

The solution is to instead of doing clnt.api._save_topology_v2([]) (Where in this case sending an empty list for the data implies executing all current temp actions) we need to pass the tempAction data for the specific action desired to be executed.

Another issue is that in order to do this you must not call _add_temp_action(data)

Instead you pass the temp action data for the specific action in your _save_topology_v2()

Another issue is that the data passed to save_topology for a temp action is slightly different from the data passed to add_temp_action.

Data format for temp action passed to save_topology: data = [ { "NetworkRollbackTask": False, "taskJson": json.dumps([{'info': info, 'infoPreview': info, 'note': '', 'action': 'associate', 'nodeType': 'imagebundle', 'nodeId': node_id, 'toId': element['key'], 'toIdType': id_type, 'fromId': '', 'nodeName': image['name'], 'fromName': '', 'toName': name, 'childTasks': [], 'parentTask': ''}]) } ]

Data format for temp action passed to add_temp_action for the same action as above: data = {'data': [{'info': info, 'infoPreview': info, 'note': '', 'action': 'associate', 'nodeType': 'imagebundle', 'nodeId': '', 'toId': element['key'], 'toIdType': id_type, 'fromId': '', 'nodeName': '', 'fromName': '', 'toName': name, 'ignoreNodeId': node_id, 'ignoreNodeName': image['name'], 'childTasks': [], 'parentTask': ''}]}

Work around example - docs/labs/lab05-device-management/add_image_wo_tempaction.py

mtache commented 2 years ago

I would like to emphasis this issue because I had the same opinion when reading the code: one could save tempAction of another user when using saveTopology.do with an empty body.

I faced another situation: When deleting a container using cvp_api.delete_container, there is comment saying that addTempAction.do does not raise an error when deleting a container with children. The code then tries to get the container info and eventually hits a 404 since the container is supposed to be deleted. It shows some ERROR logs which is quite confusing for non-insider.

Maybe using only saveTopology.do without tempAction.do would make this "still exists" check obsolete ?