Convert ovn-tester to use asyncio

[putnopvut]

ovn-tester currently works in a synchronous and serialized manner. If we are going to provision, say, 50 ports, the operation will go something like:

• Issue SSH command to create a port, and wait for that to return
• Issue SSH commands to update load balancers, and wait for them to return
• Issue SSH commands to ping ports, and wait for them to return
  Repeat 50 times.

With this series, we use python's asyncio framework to create tasks that are each responsible for performing the above operations. Each task looks something like:

• Issue SSH command to create a port, then yield control to the event loop
• Collect result of SSH command, then issue SSH commands to update load balancers. Yield control to the event loop.
• Collect result of SSH command, then issue SSH commands to ping ports. Yield control to the event loop.
• Collect result of SSH command.
  With asyncio, we can deploy multiple of these tasks at once, allowing the event loop to give control to the individual tasks as they have data to work with. The tests have been updated to take a "queries_per_second" option that allows for the number of tasks to create per second to be configured. By default, all the tests create 20 tasks per second.

In order to support asyncio, this also changes the SSH library from paramiko to asyncssh.

See the individual commits in this PR for explanations of all of what is happening in the code.

Some notes:

• This change makes use of asyncio functions that are only available in python 3.7+. ovn-heater should probably be updated to ensure that a proper version of python is used in the virtualenv when running tests.
• When testing this change in a cluster of RHEL8 servers, I found that the central container that was receiving the ovn-nbctl commands would get hung. It appeared that issuing multiple docker exec calls at once caused the container to deadlock. I fixed this by upgrading the docker version to 20.10.8 on the server in question. ovn-heater should probably be updated to ensure that a new enough docker version is installed on the physical nodes.
• Stats are accurate, but may potentially be misleading.
  • Since test iterations run concurrently, adding together multiple iterations' times doesn't make much sense. As an example, I ran the ovn-20.yml scenario on a cluster, and the entire test took 20 minutes to complete. However, looking at the netpol_multitenant report, the sum of all iterations is 70 hours.
  • Measurements include idle time while the event loop runs other tasks. As an example, let's say we issue an SSH command to ping a port. 5 seconds later, the SSH command completes. However, because the event loop is running other tasks, the result of that SSH command is not collected until 30 seconds after the ping was initially issued. In this case, the stats would show the ping took 30 seconds. It may be worth changing our measurements to be performed on the server instead (or in addition to our client-side timing)

[dceara]

This looks good overall, thanks! I only left a few minor comments and questions. Also, sorry, but you have to rebase.

[dceara]

I think we can avoid this lock all together if we split the add_vips API into, e.g., reserve_vips() (synchronous and just adding the items to the load balancer vips dict and returning them) and add_vips() coroutine to actually set the vips. If you agree, I'll leave it up to you whether we should make the change in this PR or in a separate one.

[putnopvut]

I was messing with this just now, and removing the locking is going to be non-trivial.

Adding VIPs is not an additive operation. Under the hood it calls ovn-nbctl set Load_Balancer vips .... That means each individual task is re-setting the entire VIP list instead of just adding their own VIPs to the configured load balancer. This means that each task needs to have exclusive access to the load balancer.

The VIPs are added in batches, meaning that each time we add new VIPs, we might end up making multiple ovn-nbctl calls per load balancer. Since all of those ovn-nbctl calls are asynchronous, it gives other tasks the opportunity to foul up the configured VIPs. So even if we could make the locking finer-grained, I think we'd still need locking as long as we're batching the VIPs.

Having said that, I actually noticed that there's a bug in the current add_vips() method due to the batching code. Since we are setting the VIPs on the load balancer instead of adding to them, it means that each batch undoes the previous batch's operation. Effectively it means that a load balancer will never have more than MAX_VIPS_IN_BATCH (currently 500) vips configured. This may not be as big an issue with load balancer per service, but it's still a bug. Since the operation is not additive, I actually have no idea how batching can work here.

If we could make adding VIPs additive, then this would be easy since each task would be performing independent non-overlapping operations. It would also allow for proper batching.

[dceara]

I was messing with this just now, and removing the locking is going to be non-trivial.

Adding VIPs is not an additive operation. Under the hood it calls ovn-nbctl set Load_Balancer vips .... That means each individual task is re-setting the entire VIP list instead of just adding their own VIPs to the configured load balancer. This means that each task needs to have exclusive access to the load balancer.

Actually, I think adding VIPs is an additive operation, it translates to something equivalent to:

$ ovn-nbctl lb-add lb-test 42.42.42.42:4242 21.21.21.21:2121
$ ovn-nbctl list load_balancer
_uuid               : 00874eb0-2266-43da-870c-3e8f9cb55617
name                : lb-test
vips                : {"42.42.42.42:4242"="21.21.21.21:2121"}
$ ovn-nbctl set load_balancer lb-test vips:\"43.43.43.43:4343\"=\"22.22.22.22:222\"
$ ovn-nbctl list load_balancer
_uuid               : 00874eb0-2266-43da-870c-3e8f9cb55617
name                : lb-test
vips                : {"42.42.42.42:4242"="21.21.21.21:2121", "43.43.43.43:4343"="22.22.22.22:222"

The VIPs are added in batches, meaning that each time we add new VIPs, we might end up making multiple ovn-nbctl calls per load balancer. Since all of those ovn-nbctl calls are asynchronous, it gives other tasks the opportunity to foul up the configured VIPs. So even if we could make the locking finer-grained, I think we'd still need locking as long as we're batching the VIPs.

Having said that, I actually noticed that there's a bug in the current add_vips() method due to the batching code. Since we are setting the VIPs on the load balancer instead of adding to them, it means that each batch undoes the previous batch's operation. Effectively it means that a load balancer will never have more than MAX_VIPS_IN_BATCH (currently 500) vips configured. This may not be as big an issue with load balancer per service, but it's still a bug. Since the operation is not additive, I actually have no idea how batching can work here.

As mentioned above, unless I'm missing something, the operation is already additive.

If we could make adding VIPs additive, then this would be easy since each task would be performing independent non-overlapping operations. It would also allow for proper batching.

[putnopvut]

Actually, I think adding VIPs is an additive operation, it translates to something equivalent to:

$ ovn-nbctl lb-add lb-test 42.42.42.42:4242 21.21.21.21:2121
$ ovn-nbctl list load_balancer
_uuid               : 00874eb0-2266-43da-870c-3e8f9cb55617
name                : lb-test
vips                : {"42.42.42.42:4242"="21.21.21.21:2121"}
$ ovn-nbctl set load_balancer lb-test vips:\"43.43.43.43:4343\"=\"22.22.22.22:222\"
$ ovn-nbctl list load_balancer
_uuid               : 00874eb0-2266-43da-870c-3e8f9cb55617
name                : lb-test
vips                : {"42.42.42.42:4242"="21.21.21.21:2121", "43.43.43.43:4343"="22.22.22.22:222"

That works as long as the key for the VIP is different each time. It appears our tests do not attempt to add backends to existing VIPs, so you're right this should work for our purposes.

If we can guarantee that the set of keys in the dictionary of VIPs passed to add_vips() does not intersect with the set of keys already configured on the load balancer, we could also simplify some of the code in add_vips()

[dceara]

Shall we raise an exception then if a test tries to add backends to an already configured VIP?

[mohammadheib]

Hi,
i think here we can save few seconds by looping over workers instead of ports and create a bundle of ports per ports for each loop iteration i'm not sure if that could really improve the timing of the tests but i saw some improvement on my tests:

@@ -651,10 +659,15 @@ class Cluster(object):
     async def provision_ports(self, n_ports, passive=False):
         tasks = []
         ctx = ovn_context.active_context
-        for _ in range(n_ports):
+        
+        for _ in range(len(self.worker_nodes)):
             worker = self.select_worker_for_port()
-            tasks.append(ctx.create_task(worker.provision_ports(self, 1,
+            tasks.append(ctx.create_task(worker.provision_ports(self, (n_ports//len(self.worker_nodes)),
                                                                 passive)))
+        for _ in range(n_ports%len(self.worker_nodes)):
+            worker = self.select_worker_for_port()
+            tasks.append(ctx.create_task(worker.provision_ports(self, 1, passive)))
+
         results = await asyncio.gather(*tasks)
         return [ports[0] for ports in results]

@@ -319,10 +320,17 @@ class WorkerNode(Node):
         await vsctl.del_port(port)
 
     async def provision_ports(self, cluster, n_ports, passive=False):
-        ports = [await self.provision_port(cluster, passive)
+        pcreate_tasks = []
+        pbind_tasks = []
+        ctx = ovn_context.active_context
+        
+        pcreate_tasks = [ctx.create_task(self.provision_port(cluster, passive))
                  for i in range(n_ports)]
+        ports = await asyncio.gather(*pcreate_tasks)
+
         for port in ports:
-            await self.bind_port(port)
+            pbind_tasks.append(ctx.create_task(self.bind_port(port)))
+        await asyncio.gather(*pbind_tasks)
         return ports
 

[putnopvut]

Hi, just leaving a comment to explain a few things. I have a couple more commits I want to add to this series and then I'll update the PR. I'm testing them in my async_sandbox branch right now. The first is one that changes how we wait for ports to be up. The second adds the connection pool as has been discussed in this PR.

When reviewing, I suspect you'll be able to find things that could be optimized, or you could find places where we can use asyncio features in ways that will be more efficient. But I think those sorts of improvements can wait until after the initial asyncio support is merged.

I also opened two issues: #85 and #86 . These issues exist without asyncio, but with asyncio they're even worse. I don't think those issues should impede us from getting the initial asyncio code in, though. These can be addressed afterwards.

It's currently Friday afternoon where I am so I've run out of time for the week. However, I plan on getting this PR updated early next week so that this can be reviewed again.

[putnopvut]

Hello again. I've committed the last set of patches I planned to commit for this PR. It is stable and tests are behaving as expected. As mentioned in the previous comment, there is room for some improvement, and those recognized improvements have been opened as issues.

[dceara]

If I understand correctly, with this, the only (mostly) reliable measurement is the wait_for_port_up value because we use ovn-installed-ts to determine when a port is up (I'm saying "mostly" because the start timestamp is inaccurate, it's after lsp-add and lsp-get were executed).

Should we emphasize this in the report somehow? Or shall we remove the other measurements?

Also, do you have some test run results on larger scale deployments (more physical machines)?

[dceara]

What if ovs-vsctl --timeout=... wait-until .. timed out?