Persistent lock connection for a reservation

[chenhayat]

Hi @jluebbe,
The current setup for LabGrid does not allow detecting an aggressively killed client.
When we reserve and lock a reservation, if for some reason the client is aggressively killed we will not get to the unlock and therefore the reservation will remain locked forever without knowing if we should release it.
In order to solve it we though to hold the reservation's network connection open: if the client is killed the connection will be dropped and the reservation/lock removed. So the lock command remains waiting and on ctrl+c event is will be unlocked.
What's your opinion regarding that?

Something like: labgrid-client -p+ lock --hold
on labgrid-client:

service = await self.service("org.labgrid.hold")
while True:
    # wait for SIGTERM
await service.send(UnholdMessage{})

On labgrid-coordinator:

def orgLabgridHold(msg):
    reserve()
    lock()
    try:
        while waiting_for_unhold_message:
            sleep(1)
    except ServiceDropped:
        log.Debug("service dropped")
    finally:
        unlock()
        unreserve()

[chenhayat]

I can also see that this comment was added 8 years ago in coordinatoy.py AcquirePlace function by Jan, so there was a plan to do that.

        # FIXME use the session object instead? or something else which
        # survives disconnecting clients?

Anyone has any opinion on that ? @Bastian-Krause @Emantor @JoshuaWatt @rohieb @ejoerns

[Emantor]

This does look like a feasible solution to me, the labgrid-client console command uses a similar scheme to quit microcom when a place is released. @jluebbe your input is required here.

[gregory-bronstain]

Hello @jluebbe.
Any updates?

[jluebbe]

The main issue with automatic unlocks is unresponsive clients that still perform some actions after a timeout. For example, a PC that is suspended in the middle of a pytest run: depending on timing, it might power cycle at DUT, although the place lock timeout out and was released while it was suspend. So, for now the only option is a manual unlock --kick, which can be done if a user is confident that it's safe.

Some time ago we changed the exporter to run ser2net on a random port that changes when a SerialPort is unlocked and locked again, so an old client cannot disrupt a the new owner of a place anymore. This approach isn't feasible for many other kinds of resources, though.

For scenarios where you can be sure that a client can never cause issues after a timeout (such as when a CI job aborts and all processes are killed), a timeout should be safe enough. As long as a ClientStream is still open at the gRPC level, a lock would still be considered active. The simplest change to implement this would be:

• extend the AcquirePlaceRequest gRPC message with an optional timeout
• add a --timeout to labgrid-client lock to set this message
• start a timer in the coordinator when the last ClientStream for a user is closed
• when the timer expires, force-release the locked place(s)

Alternatively, it could be implemented via reservations. The timeout would be specified via CreateReservationRequest. Then there are some options regarding how the reservation would be kept active:

• have an active ClientStream for that user
• keep extending the timeout by an explicit call for that reservation (which could be done a dedicated labgrid-client keep-reservation or something similar)

Finally, if you have an out-of-band way to detect that a client was aggressively killed, e.g. by knowing which CI jobs use which labgrid reservations, you could have an external service do the equivalent of labgrid-client unlock --kick for any reservation which belongs to a CI job that is no longer running. Perhaps it would be useful to attach some tags to a reservation for this approach.

[idan4u]

Thank you @jluebbe for your answer!

If I understood correctly the first approach we could also do something like:

1. We could hold open a clientstream by labgrid-client monitor? maybe there is another way?
2. labgrid-client reserve --timeout 30
3. labgrid-client acquire --timeout 30
4. 30 sec after the ClientStream drops the place will be unlocked.

When the ClientStream drops we also need to free reservations as well right?

[mattyclarkson]

As an update on the proof-of-concept implementation: we need to share the session context between commands so that when the monitor session drops, it does not release any resources from other sessions that could be occuring in other processes on the same machine.

Strawperson usage looks like:

$ labgrid-client --session $$ monitor &
$ MONITOR_PID="$!"
$ eval $(labgrid-client --session $$ reserve --shell tag=value)
$ labgrid-client --session $$ -p + lock
$ kill "${MONITOR_PID}" # lock and reservation are released by the coordinator which is keeping track of IP + session name

For sake of simplicity, we are not implementing the timeout and releasing resource as soon as the monitor is dropped. We do not need the timeout functionality so would prefer not implementing that for maintenance purposes.

We're unsure about the name of the --session argument, bikeshedding welcome.

--session argument could be added to each of the subparsers rather than parser to make commands session aware. Putting on the parser makes it easier to make sub-commands easier to make session aware but it would be a no-op in most cases, so likely better to put on the subparser