GNS3 / gns3-server

GNS3 server
GNU General Public License v3.0
798 stars 262 forks source link

Update not completed before another request comes #68

Closed grossmj closed 9 years ago

grossmj commented 9 years ago

Bug when changing the RAM setting multiple times and too quickly in the GUI. The Ghost IOS creation cannot be completed before another request to update arrives. The solution might be to have a lock system to prevent this kind of race conditions.

Server error from 127.0.0.1:8000: R4: Traceback (most recent call last):
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/web/route.py", line 110, in control_schema
    yield from func(request, response)
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/handlers/dynamips_vm_handler.py", line 109, in update
    yield from dynamips_manager.ghost_ios_support(vm)
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/__init__.py", line 334, in ghost_ios_support
    yield from self._set_ghost_ios(vm)
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/__init__.py", line 423, in _set_ghost_ios
    yield from ghost.create()
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/nodes/router.py", line 187, in create
    platform=self._platform))
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/dynamips_hypervisor.py", line 291, in send
    chunk = yield from self._reader.read(1024)  # match to Dynamips' buffer size
  File "/usr/lib/python3.4/asyncio/streams.py", line 430, in read
    self._waiter = self._create_waiter('read')
  File "/usr/lib/python3.4/asyncio/streams.py", line 373, in _create_waiter
    'already waiting for incoming data' % func_name)
RuntimeError: read() called while another coroutine is already waiting for incoming data
grossmj commented 9 years ago

Well I have the same issue and this is more serious this time. For instance if I add an Ethernet switch and connect 4 VPCS to that switch. Then I choose to delete this switch and get the following errors:

Server error : Traceback (most recent call last):
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/web/route.py", line 123, in control_schema
    yield from func(request, response)
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/handlers/api/dynamips_device_handler.py", line 184, in delete_nio
    yield from device.remove_nio(port_number)
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/nodes/ethernet_switch.py", line 167, in remove_nio
    yield from self._hypervisor.send('ethsw remove_nio "{name}" {nio}'.format(name=self._name, nio=nio))
  File "/home/grossmj/PycharmProjects/gns3-server/gns3server/modules/dynamips/dynamips_hypervisor.py", line 262, in send
    chunk = yield from self._reader.read(1024)  # match to Dynamips' buffer size
  File "/usr/lib/python3.4/asyncio/streams.py", line 430, in read
    self._waiter = self._create_waiter('read')
  File "/usr/lib/python3.4/asyncio/streams.py", line 373, in _create_waiter
    'already waiting for incoming data' % func_name)
RuntimeError: read() called while another coroutine is already waiting for incoming data

This is because when the GUI deletes a node, it first deletes all the links and an HTTP request to delete the NIO allocated to each link is sent to the server. The GUI doesn't wait for anything, the requests are sent in a batch, which doesn't allow the server to treat the requests sequencially.

Now we are back to my early concerns with using asyncio. We should have either a lock system or queue requests to avoid the above issue. I tend to prefer a queue.

@noplay Should we implement a queue at the VM level or the handler level? How do you think we can have something not too intrusive.

julien-duponchelle commented 9 years ago

I think the less intrusive is a lock implemented at the handler level.

We can do something like that:

    @classmethod
    @Route.post(
        r"/projects/{project_id}/dynamips/vms/{vm_id}/resume",
        parameters={
            "project_id": "UUID for the project",
            "vm_id": "UUID for the instance"
        }
    )
    @no_concurrency("vm_id")
    def suspend(request, response):

        dynamips_manager = Dynamips.instance()
        vm = dynamips_manager.get_vm(request.match_info["vm_id"], project_id=request.match_info["project_id"])
        yield from vm.resume()
        response.set_status(204)

Or even simpler in the route if we have a vm_id, we have a lock blocking all other operations until the first operation is completed.

grossmj commented 9 years ago

The current lock system prevent the above errors :+1:

Just one thing we will have to be careful is when we support long polling for VMs... ;)