From d81736043b48082d18eebb6f9d92d53d240245e4 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Mon, 30 Sep 2024 15:14:03 +0300 Subject: [PATCH 01/13] feat: Adding context manager for reserve and lock + tests --- bazel/labgrid/client/BUILD.bazel | 21 +++ bazel/labgrid/client/__init__.py | 2 + bazel/labgrid/client/lock.py | 22 +++ bazel/labgrid/client/reservation_test.py | 209 +++++++++++++++++++++ bazel/labgrid/client/reserve.py | 40 ++++ bazel/labgrid/mock/coordinator/BUILD.bazel | 3 +- bazel/labgrid/mock/coordinator/main.py | 44 ++--- bazel/labgrid/mock/crossbar/main.py | 4 + bazel/labgrid/mock/exporter/BUILD.bazel | 3 +- bazel/labgrid/mock/exporter/main.py | 3 + 10 files changed, 328 insertions(+), 23 deletions(-) create mode 100644 bazel/labgrid/client/BUILD.bazel create mode 100644 bazel/labgrid/client/__init__.py create mode 100644 bazel/labgrid/client/lock.py create mode 100644 bazel/labgrid/client/reservation_test.py create mode 100644 bazel/labgrid/client/reserve.py diff --git a/bazel/labgrid/client/BUILD.bazel b/bazel/labgrid/client/BUILD.bazel new file mode 100644 index 00000000..9746be3c --- /dev/null +++ b/bazel/labgrid/client/BUILD.bazel @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +py_library( + name = "client", + srcs = ["__init__.py", "reserve.py", "lock.py"], + data = ["@rules_labgrid//labgrid/client:client"], + visibility = ["//visibility:public"], +) + +py_test( + name = "reservation_test", + srcs = ["reservation_test.py"], + data = ["@rules_labgrid//labgrid/client:client"], + deps = [ + ":client", + "//bazel/labgrid/mock/coordinator:coordinator", + "//bazel/labgrid/mock/exporter:exporter", + "@rules_python//python/runfiles", + ], + visibility = ["//visibility:public"], +) diff --git a/bazel/labgrid/client/__init__.py b/bazel/labgrid/client/__init__.py new file mode 100644 index 00000000..989065fb --- /dev/null +++ b/bazel/labgrid/client/__init__.py @@ -0,0 +1,2 @@ +from .reserve import reserve +from .lock import lock \ No newline at end of file diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py new file mode 100644 index 00000000..51fa850b --- /dev/null +++ b/bazel/labgrid/client/lock.py @@ -0,0 +1,22 @@ +from __future__ import annotations + +from contextlib import contextmanager +from os import environ +from subprocess import DEVNULL, run +from typing import Iterator +from python.runfiles import Runfiles + +from .reserve import reserve + + +@contextmanager +def lock(tags: Mapping[str, str]) -> Iterator[str]: + runfiles = Runfiles.Create() + client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + with reserve(tags) as reservation: + env = environ | {"LG_TOKEN": reservation.token} + run((client, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) + try: + yield reservation.token + finally: + run((client, "-p", "+", "unlock"), check=True, env=env, stdout=DEVNULL) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py new file mode 100644 index 00000000..3b5f62e2 --- /dev/null +++ b/bazel/labgrid/client/reservation_test.py @@ -0,0 +1,209 @@ +#! /usr/bin/env python3 + +import unittest +from subprocess import PIPE, run +import multiprocessing +import time +from python.runfiles import Runfiles + +from bazel.labgrid.mock.coordinator.main import coordinator +from bazel.labgrid.client import lock, reserve # context managers that perform client operations + + +class TestReservation(unittest.TestCase): + @classmethod + def setUpClass(cls): + runfiles = Runfiles.Create() + cls.labgrid_client_path = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + cls.place_name = "reservation_tests_place" + + def _sleep(self, seconds, **kwargs): + """Sleep for a given number of seconds. + **kwargs is not used, but must be defined because external function might send additional arguments. + Args: + seconds (int): The number of seconds to sleep. + """ + time.sleep(seconds) + + def _raise_exception(self, **kwargs): + """Raise an exception.""" + raise Exception("This is a test exception") + + def _reserve_lock_and_execute(self, lg_tags, func, **kwargs): + """Reserve and lock a resource in labgrid. + Args: + lg_tags (dict): The given resource tags in labgrid. + func (string): The function to be executed. + kwargs (dict): The arguments to be passed to the function. + """ + with lock(lg_tags) as token: + kwargs['token'] = token + func(**kwargs) + + def _reserve_and_execute(self, lg_tags, timeout, func, **kwargs): + """Reserve a resource in labgrid without locking it. + Args: + lg_tags (dict): The given resource tags in labgrid. + timeout (int): The timeout for the reservation. + func (string): The function to be executed. + kwargs (dict): The arguments to be passed to the function. + """ + with reserve(lg_tags, timeout) as reservation: + kwargs['token'] = reservation.token + func(**kwargs) + + def _assert_reservation_token(self, place_name, token=None): + """Checks that a reservation token is created and matches the expected token. + If no token is provided, assert that no reservation is found. + Args: + place_name (string): The given resource place name in labgrid. + token (string, optional): Token of reservation. Defaults to None. + """ + cmd = (TestReservation.labgrid_client_path, "-p", place_name, "show") + process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") + print(process.stdout) + reservation_info_list = process.stdout.strip().split("reservation: ") + if token: + assert len(reservation_info_list) == 2, "No reservation found" + reservation_token = reservation_info_list[1].split("\n")[0].strip() + assert reservation_token == token, f"Reservation token mismatch: {reservation_token} != {token}" + else: + # No token provided, check if there is no reservation + assert len(reservation_info_list) == 1, "Reservation was found but not expected" + + def _assert_reservation_by_status(self, place_name, status_list=[], fail_on_match=True): + """Checks if there is/n't any reservation with the requested status, + for a given resource place name in labgrid. + Args: + place_name (string): The given resource place name in labgrid. + status_list (list): list of string that contain the following statuses: waiting | allocated. + for example: ["waiting", "allocated"] + fail_on_match (bool): True if assert should fail when status is found, + False if assert should fail when status is not found. + """ + cmd = (TestReservation.labgrid_client_path, "-p", place_name, "reservations") + process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") + + if process.stdout == "": + if fail_on_match: + # there could not be match because there are no reservations + assert True + else: + assert False, "No reservations found" + + for status in status_list: + if fail_on_match: + if f"state: {status}" in process.stdout: + assert False, f"There are {status} reservations.\n{process.stdout}" + else: + if f"state: {status}" not in process.stdout: + assert False, f"There are no {status} reservations.\n{process.stdout}" + + def test_reserve_and_lock_token_verification(self): + """ + Test that a reservation is created and locked. + Test that a reservation is unlocked and canceled after exiting the context manager. + """ + lg_tags = {"board": "fake_board", "os": "fake_os"} + place_name = TestReservation.place_name + with coordinator(lg_tags, place_name): + self._reserve_lock_and_execute( + lg_tags, + self._assert_reservation_token, + place_name=place_name + ) + + # place was unlocked and reservation was canceled + # checking that the reservation is now released + self._assert_reservation_token(place_name) + + def test_exception_in_lock_context(self): + """ + Test that a reservation is cancelled when exception is thrown in the lock context. + """ + lg_tags = {"board": "fake_board", "os": "fake_os"} + place_name = TestReservation.place_name + with coordinator(lg_tags, place_name): + with self.assertRaises(Exception): + self._reserve_lock_and_execute( + lg_tags, + self._raise_exception + ) + + time.sleep(1) + + # checking that the reservation is released even though there was an exception + self._assert_reservation_by_status(place_name, ["waiting", "allocated"], True) + + def test_waiting_reserve_is_acquired_eventually(self): + """ + Test that a reservation following an allocated reservation is acquired + eventually. This is done by running a process that reserves and sleep + in parallel to another process that waits enough time for it to be + released in order to allocate the new reservation. + """ + lg_tags = {"board": "fake_board", "os": "fake_os"} + place_name = TestReservation.place_name + with coordinator(lg_tags, place_name): + # reserve should be acquired immediately cancelled within 15 seconds + p1 = multiprocessing.Process( + target=self._reserve_and_execute, + args=(lg_tags, + 3, + self._sleep), + kwargs={'seconds': 15} + ) + p1.start() + + # wait for reservation to be acquired and check that it is allocated + time.sleep(3) + self._assert_reservation_by_status(place_name, ["allocated"], False) + + # be able to wait and allocate reservation once the previous has been released + self._reserve_and_execute( + lg_tags, + 25, + self._assert_reservation_token, + place_name=place_name + ) + + def test_verify_lock_mechanism(self): + """ + Test that the lock is working as expected, so after 1 minute + the reservation is still acquired and no new reservation can be done. + The reason fro the 1 minute is in order to differntitate is from + a situation that reserve is done but was not locked. + In this case reservation is released automatically after 1 minute. + """ + lg_tags = {"board": "fake_board", "os":"fake_os"} + place_name = TestReservation.place_name + with coordinator(lg_tags, place_name): + p = multiprocessing.Process( + target=self._reserve_lock_and_execute, + args=(lg_tags, + self._sleep), + kwargs={'seconds': 70} + ) + + p.start() + + # when reserving without locking, reservation should end within 60 seconds. + # in this test since we lock it for 70 seconds we expect the reservation to last + # for this time. + # the folowing functionally make sure that the lock had an effect so the reservation + # is still valid after 60 seconds, which causes a failure on any new reservation. + time.sleep(60) + with self.assertRaises(RuntimeError): + self._reserve_lock_and_execute(lg_tags, + self._assert_reservation_token, + place_name=place_name) + # check there are no waiting reservations + self._assert_reservation_by_status(place_name, ["waiting"], True) + p.join() + + # lock was released therefore we should be able to reserve and lock again + self._reserve_lock_and_execute(lg_tags, self._assert_reservation_token, place_name=place_name) + + +if __name__ == "__main__": + unittest.main() diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py new file mode 100644 index 00000000..b526aeb6 --- /dev/null +++ b/bazel/labgrid/client/reserve.py @@ -0,0 +1,40 @@ +from __future__ import annotations + +from contextlib import contextmanager +from dataclasses import dataclass +from subprocess import DEVNULL, PIPE, TimeoutExpired, run +from typing import Iterator, Mapping + +from python.runfiles import Runfiles + + +@dataclass(frozen=True) +class Reservation: + token: str + +# TODO: this should be in it's own module +@contextmanager +def reserve(tags: Mapping[str, str], timeout: int = 3) -> Iterator[Reservation]: + runfiles = Runfiles.Create() + client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + # FIXME: no way to provide a timeout to the CLI + # FIXME: no way to know if tags will ever be satisfied + # FIXME: will not be allocated if other reservations are cancelled + # FIXME: no machine readable output + # FIXME: does not cancel reservation if killed + cmd = (client, "reserve", "--shell", *(f"{k}={v}" for k, v in tags.items())) + process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") + + assert process.stdout.startswith("export LG_TOKEN=") + token = process.stdout.removeprefix("export LG_TOKEN=").rstrip() + + try: + run((client, "wait", token), check=True, timeout=timeout, stdout=DEVNULL) + except TimeoutExpired: + run((client, "cancel-reservation", token), check=True) + raise RuntimeError("Failed to allocate reservation") + + try: + yield Reservation(token=token) + finally: + run((client, "cancel-reservation", token), check=True) diff --git a/bazel/labgrid/mock/coordinator/BUILD.bazel b/bazel/labgrid/mock/coordinator/BUILD.bazel index ede76656..0402c074 100644 --- a/bazel/labgrid/mock/coordinator/BUILD.bazel +++ b/bazel/labgrid/mock/coordinator/BUILD.bazel @@ -17,7 +17,8 @@ py_library( "//bazel/labgrid/mock/exporter", "//labgrid/client", ], - visibility = ["//bazel/labgrid/mock:__pkg__"], + visibility = ["//bazel/labgrid/mock:__pkg__", + "//bazel/labgrid/client:__pkg__"], deps = [ "//labgrid:pkg", "@rules_python//python/runfiles", diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index b1fbe52f..343d0a11 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -1,6 +1,5 @@ from __future__ import annotations -import asyncio import socket from contextlib import contextmanager from dataclasses import dataclass @@ -11,7 +10,6 @@ from python.runfiles import Runfiles from bazel.labgrid.mock.crossbar.main import crossbar from bazel.labgrid.mock.exporter.main import exporter -from labgrid.remote.coordinator import ApplicationRunner, CoordinatorComponent from .process import coordinator_process @@ -27,12 +25,12 @@ class Coordinator: place: str """The name of the place to create.""" - -def _labgrid_client(*args): +def _labgrid_client(*args, **kwargs): runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - cmd = (client, *args) - run(cmd, check=True, stdout=PIPE, encoding="utf-8") + cmd = (client,*args) + check = kwargs.pop("check", True) + run(cmd, check=check, stdout=PIPE, encoding="utf-8") @contextmanager @@ -44,22 +42,26 @@ def coordinator( with crossbar(): with coordinator_process(): _labgrid_client("--place", place, "create") - _labgrid_client( - "--place", - place, - "set-tags", - *(f"{k}={v}" for k, v in tags.items()), - ) - with exporter(): - # add resources to the "place" + try: _labgrid_client( "--place", place, - "add-match", - f"{socket.gethostname()}/targets/NetworkService", - ) - yield Coordinator( - url=url, - tags=tags, - place=place, + "set-tags", + *(f"{k}={v}" for k, v in tags.items()) ) + + with exporter(): + # add resources to the "place" + _labgrid_client( + "--place", + place, + "add-match", + f"{socket.gethostname()}/targets/NetworkService", + ) + yield Coordinator( + url=url, + tags=tags, + place=place, + ) + finally: + _labgrid_client("--place", place, "delete") diff --git a/bazel/labgrid/mock/crossbar/main.py b/bazel/labgrid/mock/crossbar/main.py index 0ae29d5f..741256bf 100644 --- a/bazel/labgrid/mock/crossbar/main.py +++ b/bazel/labgrid/mock/crossbar/main.py @@ -60,3 +60,7 @@ def crossbar(timeout: int = 10) -> Iterator[int]: yield pid finally: process.terminate() + cmd = (crossbar, "stop") + process = Popen(cmd, stdout=PIPE, stderr=PIPE, encoding="utf-8") + process.wait() + diff --git a/bazel/labgrid/mock/exporter/BUILD.bazel b/bazel/labgrid/mock/exporter/BUILD.bazel index 21ab1523..c4127c9e 100644 --- a/bazel/labgrid/mock/exporter/BUILD.bazel +++ b/bazel/labgrid/mock/exporter/BUILD.bazel @@ -5,5 +5,6 @@ py_library( "config.yaml", "@rules_labgrid//labgrid/exporter", ], - visibility = ["//bazel/labgrid/mock/coordinator:__subpackages__"], + visibility = ["//bazel/labgrid/mock/coordinator:__subpackages__", + "//bazel/labgrid/client:__pkg__"], ) diff --git a/bazel/labgrid/mock/exporter/main.py b/bazel/labgrid/mock/exporter/main.py index e34dd3f7..2b4a053a 100644 --- a/bazel/labgrid/mock/exporter/main.py +++ b/bazel/labgrid/mock/exporter/main.py @@ -24,3 +24,6 @@ def exporter( yield pid finally: process.terminate() + process.wait() + if process.poll() is None: + print("Failed to terminate exporter process... will be terminated on crossbar stop.") -- GitLab From 27166032610dd620d8a276e9847f25f143dfbc9f Mon Sep 17 00:00:00 2001 From: chehay01 Date: Mon, 30 Sep 2024 18:04:58 +0300 Subject: [PATCH 02/13] feat: Fix issue that reservation_test and coordinator tests are running in parallel. They cannot run in parallel because there should be only one instance of coordinator on host in parallel. --- bazel/labgrid/client/reservation_test.py | 2 +- bazel/labgrid/mock/coordinator/BUILD.bazel | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index 3b5f62e2..e1bb95d0 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -15,7 +15,7 @@ class TestReservation(unittest.TestCase): def setUpClass(cls): runfiles = Runfiles.Create() cls.labgrid_client_path = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - cls.place_name = "reservation_tests_place" + cls.place_name = "test_place" def _sleep(self, seconds, **kwargs): """Sleep for a given number of seconds. diff --git a/bazel/labgrid/mock/coordinator/BUILD.bazel b/bazel/labgrid/mock/coordinator/BUILD.bazel index 0402c074..2086df83 100644 --- a/bazel/labgrid/mock/coordinator/BUILD.bazel +++ b/bazel/labgrid/mock/coordinator/BUILD.bazel @@ -32,4 +32,5 @@ py_test( ":coordinator", "@rules_python//python/runfiles", ], + tags = ["exclusive"] # set as exclusive to avoid run in parallel with reservation_test ) -- GitLab From dff262ae5ee84661ac3f0557ca194231b268c17a Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 1 Oct 2024 21:56:57 +0300 Subject: [PATCH 03/13] feat: Add option to reserve and lock for a specific user + test. --- bazel/labgrid/client/reservation_test.py | 37 ++++++++++++++++++++---- bazel/labgrid/client/reserve.py | 16 ++++++---- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index e1bb95d0..18dd4be8 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -1,7 +1,8 @@ #! /usr/bin/env python3 import unittest -from subprocess import PIPE, run +from os import environ +from subprocess import PIPE, run, DEVNULL import multiprocessing import time from python.runfiles import Runfiles @@ -11,12 +12,14 @@ from bazel.labgrid.client import lock, reserve # context managers that perform class TestReservation(unittest.TestCase): + @classmethod def setUpClass(cls): runfiles = Runfiles.Create() cls.labgrid_client_path = runfiles.Rlocation("rules_labgrid/labgrid/client/client") cls.place_name = "test_place" - + cls.lg_tags = {"board": "fake_board", "os": "fake_os"} + def _sleep(self, seconds, **kwargs): """Sleep for a given number of seconds. **kwargs is not used, but must be defined because external function might send additional arguments. @@ -28,6 +31,13 @@ class TestReservation(unittest.TestCase): def _raise_exception(self, **kwargs): """Raise an exception.""" raise Exception("This is a test exception") + + def _try_lock_with_another_user(self, **kwargs): + token = kwargs['token'] + username = "user_test" + env = environ | {"LG_TOKEN": token} + env["USER"] = username + run((TestReservation.labgrid_client_path, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) def _reserve_lock_and_execute(self, lg_tags, func, **kwargs): """Reserve and lock a resource in labgrid. @@ -99,12 +109,25 @@ class TestReservation(unittest.TestCase): if f"state: {status}" not in process.stdout: assert False, f"There are no {status} reservations.\n{process.stdout}" + def test_lock_fails_for_another_users_reservation(self): + """Reservation is done for + """ + lg_tags = TestReservation.lg_tags + place_name = TestReservation.place_name + with coordinator(lg_tags, place_name): + with self.assertRaises(Exception): + self._reserve_and_execute( + lg_tags, + 3, + self._try_lock_with_another_user + ) + def test_reserve_and_lock_token_verification(self): """ Test that a reservation is created and locked. Test that a reservation is unlocked and canceled after exiting the context manager. """ - lg_tags = {"board": "fake_board", "os": "fake_os"} + lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): self._reserve_lock_and_execute( @@ -121,7 +144,7 @@ class TestReservation(unittest.TestCase): """ Test that a reservation is cancelled when exception is thrown in the lock context. """ - lg_tags = {"board": "fake_board", "os": "fake_os"} + lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): with self.assertRaises(Exception): @@ -142,7 +165,7 @@ class TestReservation(unittest.TestCase): in parallel to another process that waits enough time for it to be released in order to allocate the new reservation. """ - lg_tags = {"board": "fake_board", "os": "fake_os"} + lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): # reserve should be acquired immediately cancelled within 15 seconds @@ -166,6 +189,8 @@ class TestReservation(unittest.TestCase): self._assert_reservation_token, place_name=place_name ) + # reservation has been canceled. check that nothing is allocated. + self._assert_reservation_by_status(place_name, ["allocated"], True) def test_verify_lock_mechanism(self): """ @@ -175,7 +200,7 @@ class TestReservation(unittest.TestCase): a situation that reserve is done but was not locked. In this case reservation is released automatically after 1 minute. """ - lg_tags = {"board": "fake_board", "os":"fake_os"} + lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): p = multiprocessing.Process( diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index b526aeb6..d47f26e0 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -1,5 +1,6 @@ from __future__ import annotations +from os import environ from contextlib import contextmanager from dataclasses import dataclass from subprocess import DEVNULL, PIPE, TimeoutExpired, run @@ -14,27 +15,32 @@ class Reservation: # TODO: this should be in it's own module @contextmanager -def reserve(tags: Mapping[str, str], timeout: int = 3) -> Iterator[Reservation]: +def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + # FIXME: no way to provide a timeout to the CLI # FIXME: no way to know if tags will ever be satisfied # FIXME: will not be allocated if other reservations are cancelled # FIXME: no machine readable output # FIXME: does not cancel reservation if killed + env = environ + if username: + env["USER"] = username cmd = (client, "reserve", "--shell", *(f"{k}={v}" for k, v in tags.items())) - process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") + process = run(cmd, check=True, env=env, stdout=PIPE, encoding="utf-8") assert process.stdout.startswith("export LG_TOKEN=") token = process.stdout.removeprefix("export LG_TOKEN=").rstrip() try: - run((client, "wait", token), check=True, timeout=timeout, stdout=DEVNULL) + run((client, "wait", token), check=True, env=env, timeout=timeout, stdout=DEVNULL) except TimeoutExpired: - run((client, "cancel-reservation", token), check=True) + run((client, "cancel-reservation", token), env=env, check=True) raise RuntimeError("Failed to allocate reservation") try: yield Reservation(token=token) finally: - run((client, "cancel-reservation", token), check=True) + run((client, "cancel-reservation", token), env=env, check=True) + -- GitLab From cee3526c4cc0d911f9ea7e48f6a21ae63e273796 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Sun, 6 Oct 2024 13:10:46 +0300 Subject: [PATCH 04/13] feat: flake8 fixes + comments --- bazel/labgrid/client/__init__.py | 2 +- bazel/labgrid/client/lock.py | 8 +- bazel/labgrid/client/reservation_test.py | 106 ++++++++++++++--------- bazel/labgrid/client/reserve.py | 8 +- 4 files changed, 78 insertions(+), 46 deletions(-) diff --git a/bazel/labgrid/client/__init__.py b/bazel/labgrid/client/__init__.py index 989065fb..d3a1f76b 100644 --- a/bazel/labgrid/client/__init__.py +++ b/bazel/labgrid/client/__init__.py @@ -1,2 +1,2 @@ from .reserve import reserve -from .lock import lock \ No newline at end of file +from .lock import lock diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 51fa850b..4727997d 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -3,18 +3,20 @@ from __future__ import annotations from contextlib import contextmanager from os import environ from subprocess import DEVNULL, run -from typing import Iterator +from typing import Iterator, Mapping from python.runfiles import Runfiles from .reserve import reserve @contextmanager -def lock(tags: Mapping[str, str]) -> Iterator[str]: +def lock(tags: Mapping[str, str], username="") -> Iterator[str]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - with reserve(tags) as reservation: + with reserve(tags=tags, username=username) as reservation: env = environ | {"LG_TOKEN": reservation.token} + if username: + env["USER"] = username run((client, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) try: yield reservation.token diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index 18dd4be8..1c0e8935 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -8,7 +8,8 @@ import time from python.runfiles import Runfiles from bazel.labgrid.mock.coordinator.main import coordinator -from bazel.labgrid.client import lock, reserve # context managers that perform client operations +# context managers that perform client operations: +from bazel.labgrid.client import lock, reserve class TestReservation(unittest.TestCase): @@ -16,13 +17,16 @@ class TestReservation(unittest.TestCase): @classmethod def setUpClass(cls): runfiles = Runfiles.Create() - cls.labgrid_client_path = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + cls.labgrid_client_path = runfiles.Rlocation( + "rules_labgrid/labgrid/client/client" + ) cls.place_name = "test_place" cls.lg_tags = {"board": "fake_board", "os": "fake_os"} def _sleep(self, seconds, **kwargs): """Sleep for a given number of seconds. - **kwargs is not used, but must be defined because external function might send additional arguments. + **kwargs is not used, but must be defined because external function + might send additional arguments. Args: seconds (int): The number of seconds to sleep. """ @@ -31,13 +35,14 @@ class TestReservation(unittest.TestCase): def _raise_exception(self, **kwargs): """Raise an exception.""" raise Exception("This is a test exception") - + def _try_lock_with_another_user(self, **kwargs): token = kwargs['token'] username = "user_test" env = environ | {"LG_TOKEN": token} env["USER"] = username - run((TestReservation.labgrid_client_path, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) + run((TestReservation.labgrid_client_path, "-p", "+", "lock"), + check=True, env=env, stdout=DEVNULL) def _reserve_lock_and_execute(self, lg_tags, func, **kwargs): """Reserve and lock a resource in labgrid. @@ -76,41 +81,50 @@ class TestReservation(unittest.TestCase): if token: assert len(reservation_info_list) == 2, "No reservation found" reservation_token = reservation_info_list[1].split("\n")[0].strip() - assert reservation_token == token, f"Reservation token mismatch: {reservation_token} != {token}" + assert reservation_token == token, \ + f"Reservation token mismatch: {reservation_token} != {token}" else: # No token provided, check if there is no reservation - assert len(reservation_info_list) == 1, "Reservation was found but not expected" + assert len(reservation_info_list) == 1, \ + "Reservation was found but not expected" - def _assert_reservation_by_status(self, place_name, status_list=[], fail_on_match=True): + def _assert_reservation_by_status(self, place_name, status_list=[], + fail_on_match=True): """Checks if there is/n't any reservation with the requested status, for a given resource place name in labgrid. Args: place_name (string): The given resource place name in labgrid. - status_list (list): list of string that contain the following statuses: waiting | allocated. + status_list (list): list of string that contain + the following statuses: waiting | allocated. for example: ["waiting", "allocated"] - fail_on_match (bool): True if assert should fail when status is found, - False if assert should fail when status is not found. + fail_on_match (bool): True if assert should fail when status is + found, False if assert should fail when status is not found. """ - cmd = (TestReservation.labgrid_client_path, "-p", place_name, "reservations") + cmd = (TestReservation.labgrid_client_path, + "-p", place_name, "reservations") process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") if process.stdout == "": if fail_on_match: # there could not be match because there are no reservations - assert True + assert True else: assert False, "No reservations found" for status in status_list: if fail_on_match: if f"state: {status}" in process.stdout: - assert False, f"There are {status} reservations.\n{process.stdout}" + assert False, \ + f"There are {status} reservations.\n{process.stdout}" else: if f"state: {status}" not in process.stdout: - assert False, f"There are no {status} reservations.\n{process.stdout}" + assert False, \ + "There are no {status} reservations.\n" +\ + process.stdout def test_lock_fails_for_another_users_reservation(self): - """Reservation is done for + """Reservation is done for the host user, but another user is trying to lock, + which should raise an exception. """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name @@ -121,11 +135,12 @@ class TestReservation(unittest.TestCase): 3, self._try_lock_with_another_user ) - + def test_reserve_and_lock_token_verification(self): """ Test that a reservation is created and locked. - Test that a reservation is unlocked and canceled after exiting the context manager. + Test that a reservation is unlocked and canceled after exiting + from the context manager. """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name @@ -142,7 +157,8 @@ class TestReservation(unittest.TestCase): def test_exception_in_lock_context(self): """ - Test that a reservation is cancelled when exception is thrown in the lock context. + Test that a reservation is cancelled when exception is thrown + in the lock context. """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name @@ -152,11 +168,14 @@ class TestReservation(unittest.TestCase): lg_tags, self._raise_exception ) - + time.sleep(1) - # checking that the reservation is released even though there was an exception - self._assert_reservation_by_status(place_name, ["waiting", "allocated"], True) + # checking that the reservation is released + # even though there was an exception + self._assert_reservation_by_status( + place_name, ["waiting", "allocated"], True + ) def test_waiting_reserve_is_acquired_eventually(self): """ @@ -168,9 +187,10 @@ class TestReservation(unittest.TestCase): lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): - # reserve should be acquired immediately cancelled within 15 seconds + # reserve should be acquired immediately cancelled + # within 15 seconds p1 = multiprocessing.Process( - target=self._reserve_and_execute, + target=self._reserve_and_execute, args=(lg_tags, 3, self._sleep), @@ -178,11 +198,14 @@ class TestReservation(unittest.TestCase): ) p1.start() - # wait for reservation to be acquired and check that it is allocated + # wait for reservation to be acquired + # and check that it is allocated time.sleep(3) - self._assert_reservation_by_status(place_name, ["allocated"], False) + self._assert_reservation_by_status(place_name, ["allocated"], + False) - # be able to wait and allocate reservation once the previous has been released + # be able to wait and allocate reservation once the previous + # has been released self._reserve_and_execute( lg_tags, 25, @@ -196,27 +219,29 @@ class TestReservation(unittest.TestCase): """ Test that the lock is working as expected, so after 1 minute the reservation is still acquired and no new reservation can be done. - The reason fro the 1 minute is in order to differntitate is from - a situation that reserve is done but was not locked. + The reason for the 1 minute is in order to differntitate is from + a situation that reserve is done but was not locked. In this case reservation is released automatically after 1 minute. """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name with coordinator(lg_tags, place_name): p = multiprocessing.Process( - target=self._reserve_lock_and_execute, + target=self._reserve_lock_and_execute, args=(lg_tags, self._sleep), kwargs={'seconds': 70} ) - + p.start() - - # when reserving without locking, reservation should end within 60 seconds. - # in this test since we lock it for 70 seconds we expect the reservation to last - # for this time. - # the folowing functionally make sure that the lock had an effect so the reservation - # is still valid after 60 seconds, which causes a failure on any new reservation. + + # when reserving without locking, reservation should end + # within 60 seconds. + # in this test since we lock it for 70 seconds we expect + # the reservation to last for this time. + # the folowing functionally make sure that the lock had + # an effect so the reservation is still valid after 60 seconds, + # which causes a failure on any new reservation. time.sleep(60) with self.assertRaises(RuntimeError): self._reserve_lock_and_execute(lg_tags, @@ -226,8 +251,11 @@ class TestReservation(unittest.TestCase): self._assert_reservation_by_status(place_name, ["waiting"], True) p.join() - # lock was released therefore we should be able to reserve and lock again - self._reserve_lock_and_execute(lg_tags, self._assert_reservation_token, place_name=place_name) + # lock was released therefore we should be able to reserve + # and lock again + self._reserve_lock_and_execute(lg_tags, + self._assert_reservation_token, + place_name=place_name) if __name__ == "__main__": diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index d47f26e0..68397b20 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -13,6 +13,7 @@ from python.runfiles import Runfiles class Reservation: token: str + # TODO: this should be in it's own module @contextmanager def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[Reservation]: @@ -27,14 +28,16 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[ env = environ if username: env["USER"] = username - cmd = (client, "reserve", "--shell", *(f"{k}={v}" for k, v in tags.items())) + cmd = (client, "reserve", "--shell", + *(f"{k}={v}" for k, v in tags.items())) process = run(cmd, check=True, env=env, stdout=PIPE, encoding="utf-8") assert process.stdout.startswith("export LG_TOKEN=") token = process.stdout.removeprefix("export LG_TOKEN=").rstrip() try: - run((client, "wait", token), check=True, env=env, timeout=timeout, stdout=DEVNULL) + run((client, "wait", token), check=True, env=env, timeout=timeout, + stdout=DEVNULL) except TimeoutExpired: run((client, "cancel-reservation", token), env=env, check=True) raise RuntimeError("Failed to allocate reservation") @@ -43,4 +46,3 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[ yield Reservation(token=token) finally: run((client, "cancel-reservation", token), env=env, check=True) - -- GitLab From d0f247f465f3478940b883727580502faebd61c3 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Sun, 6 Oct 2024 16:33:27 +0300 Subject: [PATCH 05/13] feat: Adding test for lock with username --- bazel/labgrid/client/lock.py | 3 +- bazel/labgrid/client/reservation_test.py | 67 +++++++++++++++++------- 2 files changed, 49 insertions(+), 21 deletions(-) diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 4727997d..0a5570d9 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -21,4 +21,5 @@ def lock(tags: Mapping[str, str], username="") -> Iterator[str]: try: yield reservation.token finally: - run((client, "-p", "+", "unlock"), check=True, env=env, stdout=DEVNULL) + run((client, "-p", "+", "unlock"), check=True, env=env, + stdout=DEVNULL) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index 1c0e8935..f6761a17 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -38,51 +38,63 @@ class TestReservation(unittest.TestCase): def _try_lock_with_another_user(self, **kwargs): token = kwargs['token'] - username = "user_test" + username = "another_user" env = environ | {"LG_TOKEN": token} env["USER"] = username run((TestReservation.labgrid_client_path, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) - def _reserve_lock_and_execute(self, lg_tags, func, **kwargs): + def _reserve_lock_and_execute(self, lg_tags, owner, func, **kwargs): """Reserve and lock a resource in labgrid. Args: lg_tags (dict): The given resource tags in labgrid. + owner (string): The username that should own the reservation. + if empty string, the reservation is done for the host user. func (string): The function to be executed. kwargs (dict): The arguments to be passed to the function. """ - with lock(lg_tags) as token: + with lock(lg_tags, owner) as token: kwargs['token'] = token func(**kwargs) - def _reserve_and_execute(self, lg_tags, timeout, func, **kwargs): + def _reserve_and_execute(self, lg_tags, timeout, owner, func, **kwargs): """Reserve a resource in labgrid without locking it. Args: lg_tags (dict): The given resource tags in labgrid. timeout (int): The timeout for the reservation. + owner (string): The username that should own the reservation. + if empty string, the reservation is done for the host user. func (string): The function to be executed. kwargs (dict): The arguments to be passed to the function. """ - with reserve(lg_tags, timeout) as reservation: + with reserve(lg_tags, timeout, owner) as reservation: kwargs['token'] = reservation.token func(**kwargs) - def _assert_reservation_token(self, place_name, token=None): - """Checks that a reservation token is created and matches the expected token. + def _assert_reservation_token_and_username(self, place_name, token=None, + username=""): + """Checks that a reservation token is created + and matches the expected token. If no token is provided, assert that no reservation is found. Args: place_name (string): The given resource place name in labgrid. token (string, optional): Token of reservation. Defaults to None. + username (string, optional): The username that should own + the reservation. """ cmd = (TestReservation.labgrid_client_path, "-p", place_name, "show") process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") - print(process.stdout) reservation_info_list = process.stdout.strip().split("reservation: ") if token: assert len(reservation_info_list) == 2, "No reservation found" reservation_token = reservation_info_list[1].split("\n")[0].strip() assert reservation_token == token, \ f"Reservation token mismatch: {reservation_token} != {token}" + if username: + owner = process.stdout.strip().split("acquired: ")[1].\ + split("\n")[0].strip() + assert owner.endswith(f"/{username}"), \ + f"Username {username} is not acquired the reservation" else: # No token provided, check if there is no reservation assert len(reservation_info_list) == 1, \ @@ -128,32 +140,37 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name + username = "user_test" with coordinator(lg_tags, place_name): with self.assertRaises(Exception): self._reserve_and_execute( lg_tags, 3, + username, self._try_lock_with_another_user ) def test_reserve_and_lock_token_verification(self): """ - Test that a reservation is created and locked. - Test that a reservation is unlocked and canceled after exiting + Test that a reservation is created and locked for the owner. + Test that a reservation is unlocked and canceled after exiting. from the context manager. """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name + username = "user_test" with coordinator(lg_tags, place_name): self._reserve_lock_and_execute( lg_tags, - self._assert_reservation_token, - place_name=place_name + username, + self._assert_reservation_token_and_username, + place_name=place_name, + username=username ) # place was unlocked and reservation was canceled # checking that the reservation is now released - self._assert_reservation_token(place_name) + self._assert_reservation_token_and_username(place_name) def test_exception_in_lock_context(self): """ @@ -166,6 +183,7 @@ class TestReservation(unittest.TestCase): with self.assertRaises(Exception): self._reserve_lock_and_execute( lg_tags, + "", self._raise_exception ) @@ -193,6 +211,7 @@ class TestReservation(unittest.TestCase): target=self._reserve_and_execute, args=(lg_tags, 3, + "", self._sleep), kwargs={'seconds': 15} ) @@ -209,7 +228,8 @@ class TestReservation(unittest.TestCase): self._reserve_and_execute( lg_tags, 25, - self._assert_reservation_token, + "", + self._assert_reservation_token_and_username, place_name=place_name ) # reservation has been canceled. check that nothing is allocated. @@ -229,6 +249,7 @@ class TestReservation(unittest.TestCase): p = multiprocessing.Process( target=self._reserve_lock_and_execute, args=(lg_tags, + "", self._sleep), kwargs={'seconds': 70} ) @@ -244,18 +265,24 @@ class TestReservation(unittest.TestCase): # which causes a failure on any new reservation. time.sleep(60) with self.assertRaises(RuntimeError): - self._reserve_lock_and_execute(lg_tags, - self._assert_reservation_token, - place_name=place_name) + self._reserve_lock_and_execute( + lg_tags, + "", + self._assert_reservation_token_and_username, + place_name=place_name + ) # check there are no waiting reservations self._assert_reservation_by_status(place_name, ["waiting"], True) p.join() # lock was released therefore we should be able to reserve # and lock again - self._reserve_lock_and_execute(lg_tags, - self._assert_reservation_token, - place_name=place_name) + self._reserve_lock_and_execute( + lg_tags, + "", + self._assert_reservation_token_and_username, + place_name=place_name + ) if __name__ == "__main__": -- GitLab From 9cf7b2e37d9dcfac8ca3ebfb9c657bb15f6163e4 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 8 Oct 2024 20:10:16 +0300 Subject: [PATCH 06/13] feat: Review fixes --- .gitignore | 1 + bazel/labgrid/client/BUILD.bazel | 16 +- bazel/labgrid/client/lock.py | 6 +- bazel/labgrid/client/reservation_test.py | 288 +++++---------------- bazel/labgrid/client/reserve.py | 17 +- bazel/labgrid/mock/__init__.py | 1 - bazel/labgrid/mock/coordinator/BUILD.bazel | 8 +- bazel/labgrid/mock/exporter/BUILD.bazel | 6 +- 8 files changed, 103 insertions(+), 240 deletions(-) diff --git a/.gitignore b/.gitignore index bd4b2dae..49e1020e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ /.bazelrc.user /node_modules/ /public/ +/.vscode/ \ No newline at end of file diff --git a/bazel/labgrid/client/BUILD.bazel b/bazel/labgrid/client/BUILD.bazel index 9746be3c..8eb58a32 100644 --- a/bazel/labgrid/client/BUILD.bazel +++ b/bazel/labgrid/client/BUILD.bazel @@ -2,20 +2,24 @@ load("@rules_python//python:defs.bzl", "py_library", "py_test") py_library( name = "client", - srcs = ["__init__.py", "reserve.py", "lock.py"], - data = ["@rules_labgrid//labgrid/client:client"], + srcs = [ + "__init__.py", + "lock.py", + "reserve.py", + ], + data = ["//labgrid/client"], visibility = ["//visibility:public"], ) py_test( name = "reservation_test", srcs = ["reservation_test.py"], - data = ["@rules_labgrid//labgrid/client:client"], + data = ["//labgrid/client"], + visibility = ["//visibility:public"], deps = [ ":client", - "//bazel/labgrid/mock/coordinator:coordinator", - "//bazel/labgrid/mock/exporter:exporter", + "//bazel/labgrid/mock/coordinator", + "//bazel/labgrid/mock/exporter", "@rules_python//python/runfiles", ], - visibility = ["//visibility:public"], ) diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 0a5570d9..07c89190 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -10,16 +10,16 @@ from .reserve import reserve @contextmanager -def lock(tags: Mapping[str, str], username="") -> Iterator[str]: +def lock(tags: Mapping[str, str], username: str ="") -> Iterator[str]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") with reserve(tags=tags, username=username) as reservation: env = environ | {"LG_TOKEN": reservation.token} if username: - env["USER"] = username + env["LG_USERNAME"] = username run((client, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) try: - yield reservation.token + yield reservation finally: run((client, "-p", "+", "unlock"), check=True, env=env, stdout=DEVNULL) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index f6761a17..ebbc5d6d 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -1,10 +1,10 @@ #! /usr/bin/env python3 import unittest -from os import environ -from subprocess import PIPE, run, DEVNULL -import multiprocessing +import yaml +from subprocess import PIPE, run import time +import threading from python.runfiles import Runfiles from bazel.labgrid.mock.coordinator.main import coordinator @@ -23,132 +23,39 @@ class TestReservation(unittest.TestCase): cls.place_name = "test_place" cls.lg_tags = {"board": "fake_board", "os": "fake_os"} - def _sleep(self, seconds, **kwargs): - """Sleep for a given number of seconds. - **kwargs is not used, but must be defined because external function - might send additional arguments. - Args: - seconds (int): The number of seconds to sleep. - """ - time.sleep(seconds) - - def _raise_exception(self, **kwargs): - """Raise an exception.""" - raise Exception("This is a test exception") - - def _try_lock_with_another_user(self, **kwargs): - token = kwargs['token'] - username = "another_user" - env = environ | {"LG_TOKEN": token} - env["USER"] = username - run((TestReservation.labgrid_client_path, "-p", "+", "lock"), - check=True, env=env, stdout=DEVNULL) - - def _reserve_lock_and_execute(self, lg_tags, owner, func, **kwargs): - """Reserve and lock a resource in labgrid. - Args: - lg_tags (dict): The given resource tags in labgrid. - owner (string): The username that should own the reservation. - if empty string, the reservation is done for the host user. - func (string): The function to be executed. - kwargs (dict): The arguments to be passed to the function. - """ - with lock(lg_tags, owner) as token: - kwargs['token'] = token - func(**kwargs) + cls.__cm = coordinator( + tags=TestReservation.lg_tags, + place=TestReservation.place_name + ) + cls.coordinator = cls.__cm.__enter__() - def _reserve_and_execute(self, lg_tags, timeout, owner, func, **kwargs): - """Reserve a resource in labgrid without locking it. - Args: - lg_tags (dict): The given resource tags in labgrid. - timeout (int): The timeout for the reservation. - owner (string): The username that should own the reservation. - if empty string, the reservation is done for the host user. - func (string): The function to be executed. - kwargs (dict): The arguments to be passed to the function. - """ - with reserve(lg_tags, timeout, owner) as reservation: - kwargs['token'] = reservation.token - func(**kwargs) + @classmethod + def tearDownClass(cls): + cls.__cm.__exit__(None, None, None) - def _assert_reservation_token_and_username(self, place_name, token=None, - username=""): - """Checks that a reservation token is created - and matches the expected token. - If no token is provided, assert that no reservation is found. + def _get_place_details_dict(self, place_name): + """Execute the labgrid client show command, parse the output + and return a dictionary with the place details. Args: place_name (string): The given resource place name in labgrid. - token (string, optional): Token of reservation. Defaults to None. - username (string, optional): The username that should own - the reservation. """ cmd = (TestReservation.labgrid_client_path, "-p", place_name, "show") process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") - reservation_info_list = process.stdout.strip().split("reservation: ") - if token: - assert len(reservation_info_list) == 2, "No reservation found" - reservation_token = reservation_info_list[1].split("\n")[0].strip() - assert reservation_token == token, \ - f"Reservation token mismatch: {reservation_token} != {token}" - if username: - owner = process.stdout.strip().split("acquired: ")[1].\ - split("\n")[0].strip() - assert owner.endswith(f"/{username}"), \ - f"Username {username} is not acquired the reservation" - else: - # No token provided, check if there is no reservation - assert len(reservation_info_list) == 1, \ - "Reservation was found but not expected" - - def _assert_reservation_by_status(self, place_name, status_list=[], - fail_on_match=True): - """Checks if there is/n't any reservation with the requested status, - for a given resource place name in labgrid. + dict = yaml.safe_load(process.stdout) + return list(dict.values())[0] + + def _reserve_and_sleep(self, lg_tags, timeout): + with reserve(lg_tags): + time.sleep(timeout) + + def _assert_unlocked_and_canceled(self, place_name): + """Checking that the place is not acquired and not reserved. Args: - place_name (string): The given resource place name in labgrid. - status_list (list): list of string that contain - the following statuses: waiting | allocated. - for example: ["waiting", "allocated"] - fail_on_match (bool): True if assert should fail when status is - found, False if assert should fail when status is not found. - """ - cmd = (TestReservation.labgrid_client_path, - "-p", place_name, "reservations") - process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") - - if process.stdout == "": - if fail_on_match: - # there could not be match because there are no reservations - assert True - else: - assert False, "No reservations found" - - for status in status_list: - if fail_on_match: - if f"state: {status}" in process.stdout: - assert False, \ - f"There are {status} reservations.\n{process.stdout}" - else: - if f"state: {status}" not in process.stdout: - assert False, \ - "There are no {status} reservations.\n" +\ - process.stdout - - def test_lock_fails_for_another_users_reservation(self): - """Reservation is done for the host user, but another user is trying to lock, - which should raise an exception. + place_name (string): the given resource place name in labgrid. """ - lg_tags = TestReservation.lg_tags - place_name = TestReservation.place_name - username = "user_test" - with coordinator(lg_tags, place_name): - with self.assertRaises(Exception): - self._reserve_and_execute( - lg_tags, - 3, - username, - self._try_lock_with_another_user - ) + dict = self._get_place_details_dict(place_name) + self.assertNotEqual("acquired", "None", "Reservation was not unlocked") + self.assertNotIn("reservation", dict, "Reservation was not canceled") def test_reserve_and_lock_token_verification(self): """ @@ -159,18 +66,22 @@ class TestReservation(unittest.TestCase): lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name username = "user_test" - with coordinator(lg_tags, place_name): - self._reserve_lock_and_execute( - lg_tags, + with lock(lg_tags, username) as reservation: + dict = self._get_place_details_dict(place_name) + place_token = dict["reservation"] + owner = dict["acquired"].split("/")[-1] + self.assertEqual( + place_token, + reservation.token, + f"Reservation token mismatch: {place_token} != {reservation.token}" + ) + self.assertEqual( + owner, username, - self._assert_reservation_token_and_username, - place_name=place_name, - username=username + f"Username {username} is not acquired the reservation (owned by {owner})" ) - # place was unlocked and reservation was canceled - # checking that the reservation is now released - self._assert_reservation_token_and_username(place_name) + self._assert_unlocked_and_canceled(place_name) def test_exception_in_lock_context(self): """ @@ -179,21 +90,24 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name - with coordinator(lg_tags, place_name): - with self.assertRaises(Exception): - self._reserve_lock_and_execute( - lg_tags, - "", - self._raise_exception - ) + with self.assertRaises(Exception): + with lock(lg_tags): + raise Exception("This is a test exception") - time.sleep(1) + time.sleep(1) + # checking that the reservation is released despite the exception + self._assert_unlocked_and_canceled(place_name) - # checking that the reservation is released - # even though there was an exception - self._assert_reservation_by_status( - place_name, ["waiting", "allocated"], True - ) + def test_reserve_fails_if_timeout(self): + """ + Test that a reservation is cancelled when exception is thrown + in the lock context. + """ + lg_tags = TestReservation.lg_tags + with reserve(lg_tags): + with self.assertRaises(RuntimeError): + with reserve(lg_tags, 1): + pass def test_waiting_reserve_is_acquired_eventually(self): """ @@ -204,84 +118,20 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name - with coordinator(lg_tags, place_name): - # reserve should be acquired immediately cancelled - # within 15 seconds - p1 = multiprocessing.Process( - target=self._reserve_and_execute, - args=(lg_tags, - 3, - "", - self._sleep), - kwargs={'seconds': 15} - ) - p1.start() - - # wait for reservation to be acquired - # and check that it is allocated - time.sleep(3) - self._assert_reservation_by_status(place_name, ["allocated"], - False) - - # be able to wait and allocate reservation once the previous - # has been released - self._reserve_and_execute( - lg_tags, - 25, - "", - self._assert_reservation_token_and_username, - place_name=place_name - ) - # reservation has been canceled. check that nothing is allocated. - self._assert_reservation_by_status(place_name, ["allocated"], True) - - def test_verify_lock_mechanism(self): - """ - Test that the lock is working as expected, so after 1 minute - the reservation is still acquired and no new reservation can be done. - The reason for the 1 minute is in order to differntitate is from - a situation that reserve is done but was not locked. - In this case reservation is released automatically after 1 minute. - """ - lg_tags = TestReservation.lg_tags - place_name = TestReservation.place_name - with coordinator(lg_tags, place_name): - p = multiprocessing.Process( - target=self._reserve_lock_and_execute, - args=(lg_tags, - "", - self._sleep), - kwargs={'seconds': 70} - ) - - p.start() - - # when reserving without locking, reservation should end - # within 60 seconds. - # in this test since we lock it for 70 seconds we expect - # the reservation to last for this time. - # the folowing functionally make sure that the lock had - # an effect so the reservation is still valid after 60 seconds, - # which causes a failure on any new reservation. - time.sleep(60) - with self.assertRaises(RuntimeError): - self._reserve_lock_and_execute( - lg_tags, - "", - self._assert_reservation_token_and_username, - place_name=place_name - ) - # check there are no waiting reservations - self._assert_reservation_by_status(place_name, ["waiting"], True) - p.join() - # lock was released therefore we should be able to reserve - # and lock again - self._reserve_lock_and_execute( - lg_tags, - "", - self._assert_reservation_token_and_username, - place_name=place_name + threading.Thread( + target=self._reserve_and_sleep, + args=(lg_tags, 1) + ).start() + + with reserve(lg_tags, 15) as reservation: + # if we entered here p1 process is already terminated + dict = self._get_place_details_dict(place_name) + place_token = dict["reservation"] + self.assertEqual( + place_token, + reservation.token, + f"Reservation token mismatch: {place_token} != {reservation.token}" ) diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index 68397b20..bbcbd755 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -5,6 +5,7 @@ from contextlib import contextmanager from dataclasses import dataclass from subprocess import DEVNULL, PIPE, TimeoutExpired, run from typing import Iterator, Mapping +import yaml from python.runfiles import Runfiles @@ -12,11 +13,12 @@ from python.runfiles import Runfiles @dataclass(frozen=True) class Reservation: token: str + owner: str # TODO: this should be in it's own module @contextmanager -def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[Reservation]: +def reserve(tags: Mapping[str, str], timeout: int = 3, username: str = "") -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") @@ -27,13 +29,16 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[ # FIXME: does not cancel reservation if killed env = environ if username: - env["USER"] = username - cmd = (client, "reserve", "--shell", + env["LG_USERNAME"] = username + cmd = (client, "reserve", *(f"{k}={v}" for k, v in tags.items())) process = run(cmd, check=True, env=env, stdout=PIPE, encoding="utf-8") + reservation_dict = yaml.safe_load(process.stdout) + reservation_dict_values = list(reservation_dict.values())[0] + token = reservation_dict_values["token"] + owner = reservation_dict_values["owner"].split("/")[-1] - assert process.stdout.startswith("export LG_TOKEN=") - token = process.stdout.removeprefix("export LG_TOKEN=").rstrip() + assert token try: run((client, "wait", token), check=True, env=env, timeout=timeout, @@ -43,6 +48,6 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username="") -> Iterator[ raise RuntimeError("Failed to allocate reservation") try: - yield Reservation(token=token) + yield Reservation(token=token, owner=owner) finally: run((client, "cancel-reservation", token), env=env, check=True) diff --git a/bazel/labgrid/mock/__init__.py b/bazel/labgrid/mock/__init__.py index 37a4981a..e69de29b 100644 --- a/bazel/labgrid/mock/__init__.py +++ b/bazel/labgrid/mock/__init__.py @@ -1 +0,0 @@ -from .coordinator.main import coordinator diff --git a/bazel/labgrid/mock/coordinator/BUILD.bazel b/bazel/labgrid/mock/coordinator/BUILD.bazel index 2086df83..10ff95ad 100644 --- a/bazel/labgrid/mock/coordinator/BUILD.bazel +++ b/bazel/labgrid/mock/coordinator/BUILD.bazel @@ -17,8 +17,10 @@ py_library( "//bazel/labgrid/mock/exporter", "//labgrid/client", ], - visibility = ["//bazel/labgrid/mock:__pkg__", - "//bazel/labgrid/client:__pkg__"], + visibility = [ + "//bazel/labgrid/client:__pkg__", + "//bazel/labgrid/mock:__pkg__", + ], deps = [ "//labgrid:pkg", "@rules_python//python/runfiles", @@ -28,9 +30,9 @@ py_library( py_test( name = "test", srcs = ["test.py"], + tags = ["exclusive"], # set as exclusive to avoid run in parallel with reservation_test deps = [ ":coordinator", "@rules_python//python/runfiles", ], - tags = ["exclusive"] # set as exclusive to avoid run in parallel with reservation_test ) diff --git a/bazel/labgrid/mock/exporter/BUILD.bazel b/bazel/labgrid/mock/exporter/BUILD.bazel index c4127c9e..95aa5ef4 100644 --- a/bazel/labgrid/mock/exporter/BUILD.bazel +++ b/bazel/labgrid/mock/exporter/BUILD.bazel @@ -5,6 +5,8 @@ py_library( "config.yaml", "@rules_labgrid//labgrid/exporter", ], - visibility = ["//bazel/labgrid/mock/coordinator:__subpackages__", - "//bazel/labgrid/client:__pkg__"], + visibility = [ + "//bazel/labgrid/client:__pkg__", + "//bazel/labgrid/mock/coordinator:__subpackages__", + ], ) -- GitLab From 932a29c4fea6f3d0c7708d00a438cfb1229f00cc Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 19 Nov 2024 18:15:13 +0200 Subject: [PATCH 07/13] feat: review fixes --- bazel/labgrid/client/BUILD.bazel | 1 + bazel/labgrid/client/__init__.py | 1 + bazel/labgrid/client/common.py | 7 +++++ bazel/labgrid/client/reservation_test.py | 40 ++++++++++-------------- bazel/labgrid/client/reserve.py | 5 --- 5 files changed, 26 insertions(+), 28 deletions(-) create mode 100644 bazel/labgrid/client/common.py diff --git a/bazel/labgrid/client/BUILD.bazel b/bazel/labgrid/client/BUILD.bazel index 8eb58a32..a1d0586d 100644 --- a/bazel/labgrid/client/BUILD.bazel +++ b/bazel/labgrid/client/BUILD.bazel @@ -6,6 +6,7 @@ py_library( "__init__.py", "lock.py", "reserve.py", + "common.py" ], data = ["//labgrid/client"], visibility = ["//visibility:public"], diff --git a/bazel/labgrid/client/__init__.py b/bazel/labgrid/client/__init__.py index d3a1f76b..2d40e50b 100644 --- a/bazel/labgrid/client/__init__.py +++ b/bazel/labgrid/client/__init__.py @@ -1,2 +1,3 @@ from .reserve import reserve from .lock import lock +from .common import labgrid_client diff --git a/bazel/labgrid/client/common.py b/bazel/labgrid/client/common.py new file mode 100644 index 00000000..48c87104 --- /dev/null +++ b/bazel/labgrid/client/common.py @@ -0,0 +1,7 @@ +from subprocess import PIPE, run + + +def labgrid_client(labgrid_client_path, *args): + cmd = (labgrid_client_path,) + args + process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") + return process diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index ebbc5d6d..d20948bf 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -2,14 +2,13 @@ import unittest import yaml -from subprocess import PIPE, run import time import threading from python.runfiles import Runfiles from bazel.labgrid.mock.coordinator.main import coordinator # context managers that perform client operations: -from bazel.labgrid.client import lock, reserve +from bazel.labgrid.client import lock, reserve, labgrid_client class TestReservation(unittest.TestCase): @@ -33,29 +32,20 @@ class TestReservation(unittest.TestCase): def tearDownClass(cls): cls.__cm.__exit__(None, None, None) - def _get_place_details_dict(self, place_name): + def _get_place(self, name): """Execute the labgrid client show command, parse the output and return a dictionary with the place details. Args: place_name (string): The given resource place name in labgrid. """ - cmd = (TestReservation.labgrid_client_path, "-p", place_name, "show") - process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") - dict = yaml.safe_load(process.stdout) - return list(dict.values())[0] + process = labgrid_client(TestReservation.labgrid_client_path, "-p", name, "show") + show_command_dict = yaml.safe_load(process.stdout) + place_details = list(show_command_dict.values())[0] + return place_details def _reserve_and_sleep(self, lg_tags, timeout): with reserve(lg_tags): time.sleep(timeout) - - def _assert_unlocked_and_canceled(self, place_name): - """Checking that the place is not acquired and not reserved. - Args: - place_name (string): the given resource place name in labgrid. - """ - dict = self._get_place_details_dict(place_name) - self.assertNotEqual("acquired", "None", "Reservation was not unlocked") - self.assertNotIn("reservation", dict, "Reservation was not canceled") def test_reserve_and_lock_token_verification(self): """ @@ -67,7 +57,7 @@ class TestReservation(unittest.TestCase): place_name = TestReservation.place_name username = "user_test" with lock(lg_tags, username) as reservation: - dict = self._get_place_details_dict(place_name) + dict = self._get_place(place_name) place_token = dict["reservation"] owner = dict["acquired"].split("/")[-1] self.assertEqual( @@ -81,7 +71,9 @@ class TestReservation(unittest.TestCase): f"Username {username} is not acquired the reservation (owned by {owner})" ) - self._assert_unlocked_and_canceled(place_name) + dict = self._get_place(place_name) + self.assertNotEqual(dict["acquired"], None, "Reservation was not unlocked") + self.assertNotIn("reservation", dict, "Reservation was not canceled") def test_exception_in_lock_context(self): """ @@ -94,19 +86,21 @@ class TestReservation(unittest.TestCase): with lock(lg_tags): raise Exception("This is a test exception") + # waiting for reservation's new status to be updated time.sleep(1) # checking that the reservation is released despite the exception - self._assert_unlocked_and_canceled(place_name) + dict = self._get_place(place_name) + self.assertNotEqual(dict["acquired"], None, "Reservation was not unlocked") + self.assertNotIn("reservation", dict, "Reservation was not canceled") def test_reserve_fails_if_timeout(self): """ - Test that a reservation is cancelled when exception is thrown - in the lock context. + Test that a reservation throws error when failed to reserve """ lg_tags = TestReservation.lg_tags with reserve(lg_tags): with self.assertRaises(RuntimeError): - with reserve(lg_tags, 1): + with reserve(lg_tags, timeout=1): pass def test_waiting_reserve_is_acquired_eventually(self): @@ -126,7 +120,7 @@ class TestReservation(unittest.TestCase): with reserve(lg_tags, 15) as reservation: # if we entered here p1 process is already terminated - dict = self._get_place_details_dict(place_name) + dict = self._get_place(place_name) place_token = dict["reservation"] self.assertEqual( place_token, diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index bbcbd755..d1916ac8 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -16,16 +16,11 @@ class Reservation: owner: str -# TODO: this should be in it's own module @contextmanager def reserve(tags: Mapping[str, str], timeout: int = 3, username: str = "") -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - # FIXME: no way to provide a timeout to the CLI - # FIXME: no way to know if tags will ever be satisfied - # FIXME: will not be allocated if other reservations are cancelled - # FIXME: no machine readable output # FIXME: does not cancel reservation if killed env = environ if username: -- GitLab From d13da656e70c1115f727feb232ea9c79221a5ffe Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 19 Nov 2024 18:29:49 +0200 Subject: [PATCH 08/13] feat: Format --- bazel/labgrid/client/lock.py | 5 ++--- bazel/labgrid/client/reservation_test.py | 22 ++++++++++------------ bazel/labgrid/client/reserve.py | 16 +++++++++++----- bazel/labgrid/mock/coordinator/main.py | 8 +++----- bazel/labgrid/mock/crossbar/main.py | 1 - bazel/labgrid/mock/exporter/main.py | 4 +++- 6 files changed, 29 insertions(+), 27 deletions(-) diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 07c89190..5d9583da 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -10,7 +10,7 @@ from .reserve import reserve @contextmanager -def lock(tags: Mapping[str, str], username: str ="") -> Iterator[str]: +def lock(tags: Mapping[str, str], username: str = "") -> Iterator[str]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") with reserve(tags=tags, username=username) as reservation: @@ -21,5 +21,4 @@ def lock(tags: Mapping[str, str], username: str ="") -> Iterator[str]: try: yield reservation finally: - run((client, "-p", "+", "unlock"), check=True, env=env, - stdout=DEVNULL) + run((client, "-p", "+", "unlock"), check=True, env=env, stdout=DEVNULL) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index d20948bf..e613559e 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -7,12 +7,12 @@ import threading from python.runfiles import Runfiles from bazel.labgrid.mock.coordinator.main import coordinator + # context managers that perform client operations: from bazel.labgrid.client import lock, reserve, labgrid_client class TestReservation(unittest.TestCase): - @classmethod def setUpClass(cls): runfiles = Runfiles.Create() @@ -23,8 +23,7 @@ class TestReservation(unittest.TestCase): cls.lg_tags = {"board": "fake_board", "os": "fake_os"} cls.__cm = coordinator( - tags=TestReservation.lg_tags, - place=TestReservation.place_name + tags=TestReservation.lg_tags, place=TestReservation.place_name ) cls.coordinator = cls.__cm.__enter__() @@ -38,11 +37,13 @@ class TestReservation(unittest.TestCase): Args: place_name (string): The given resource place name in labgrid. """ - process = labgrid_client(TestReservation.labgrid_client_path, "-p", name, "show") + process = labgrid_client( + TestReservation.labgrid_client_path, "-p", name, "show" + ) show_command_dict = yaml.safe_load(process.stdout) place_details = list(show_command_dict.values())[0] return place_details - + def _reserve_and_sleep(self, lg_tags, timeout): with reserve(lg_tags): time.sleep(timeout) @@ -63,12 +64,12 @@ class TestReservation(unittest.TestCase): self.assertEqual( place_token, reservation.token, - f"Reservation token mismatch: {place_token} != {reservation.token}" + f"Reservation token mismatch: {place_token} != {reservation.token}", ) self.assertEqual( owner, username, - f"Username {username} is not acquired the reservation (owned by {owner})" + f"Username {username} is not acquired the reservation (owned by {owner})", ) dict = self._get_place(place_name) @@ -113,10 +114,7 @@ class TestReservation(unittest.TestCase): lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name - threading.Thread( - target=self._reserve_and_sleep, - args=(lg_tags, 1) - ).start() + threading.Thread(target=self._reserve_and_sleep, args=(lg_tags, 1)).start() with reserve(lg_tags, 15) as reservation: # if we entered here p1 process is already terminated @@ -125,7 +123,7 @@ class TestReservation(unittest.TestCase): self.assertEqual( place_token, reservation.token, - f"Reservation token mismatch: {place_token} != {reservation.token}" + f"Reservation token mismatch: {place_token} != {reservation.token}", ) diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index d1916ac8..5b3a5fff 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -17,7 +17,9 @@ class Reservation: @contextmanager -def reserve(tags: Mapping[str, str], timeout: int = 3, username: str = "") -> Iterator[Reservation]: +def reserve( + tags: Mapping[str, str], timeout: int = 3, username: str = "" +) -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") @@ -25,8 +27,7 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username: str = "") -> It env = environ if username: env["LG_USERNAME"] = username - cmd = (client, "reserve", - *(f"{k}={v}" for k, v in tags.items())) + cmd = (client, "reserve", *(f"{k}={v}" for k, v in tags.items())) process = run(cmd, check=True, env=env, stdout=PIPE, encoding="utf-8") reservation_dict = yaml.safe_load(process.stdout) reservation_dict_values = list(reservation_dict.values())[0] @@ -36,8 +37,13 @@ def reserve(tags: Mapping[str, str], timeout: int = 3, username: str = "") -> It assert token try: - run((client, "wait", token), check=True, env=env, timeout=timeout, - stdout=DEVNULL) + run( + (client, "wait", token), + check=True, + env=env, + timeout=timeout, + stdout=DEVNULL, + ) except TimeoutExpired: run((client, "cancel-reservation", token), env=env, check=True) raise RuntimeError("Failed to allocate reservation") diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index 343d0a11..35a053b9 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -25,10 +25,11 @@ class Coordinator: place: str """The name of the place to create.""" + def _labgrid_client(*args, **kwargs): runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - cmd = (client,*args) + cmd = (client, *args) check = kwargs.pop("check", True) run(cmd, check=check, stdout=PIPE, encoding="utf-8") @@ -44,10 +45,7 @@ def coordinator( _labgrid_client("--place", place, "create") try: _labgrid_client( - "--place", - place, - "set-tags", - *(f"{k}={v}" for k, v in tags.items()) + "--place", place, "set-tags", *(f"{k}={v}" for k, v in tags.items()) ) with exporter(): diff --git a/bazel/labgrid/mock/crossbar/main.py b/bazel/labgrid/mock/crossbar/main.py index 741256bf..5c89d650 100644 --- a/bazel/labgrid/mock/crossbar/main.py +++ b/bazel/labgrid/mock/crossbar/main.py @@ -63,4 +63,3 @@ def crossbar(timeout: int = 10) -> Iterator[int]: cmd = (crossbar, "stop") process = Popen(cmd, stdout=PIPE, stderr=PIPE, encoding="utf-8") process.wait() - diff --git a/bazel/labgrid/mock/exporter/main.py b/bazel/labgrid/mock/exporter/main.py index 2b4a053a..32b2b0c4 100644 --- a/bazel/labgrid/mock/exporter/main.py +++ b/bazel/labgrid/mock/exporter/main.py @@ -26,4 +26,6 @@ def exporter( process.terminate() process.wait() if process.poll() is None: - print("Failed to terminate exporter process... will be terminated on crossbar stop.") + print( + "Failed to terminate exporter process... will be terminated on crossbar stop." + ) -- GitLab From 093594a8b541b98369993b2029043fe0e59326bb Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 19 Nov 2024 19:31:09 +0200 Subject: [PATCH 09/13] feat: review fixes --- .gitignore | 3 +- bazel/labgrid/client/BUILD.bazel | 2 +- bazel/labgrid/client/common.py | 28 ++++++-- bazel/labgrid/client/lock.py | 23 ++++--- bazel/labgrid/client/reservation_test.py | 75 ++++++++++++++-------- bazel/labgrid/client/reserve.py | 35 +++++----- bazel/labgrid/mock/coordinator/BUILD.bazel | 2 +- bazel/labgrid/mock/coordinator/main.py | 40 +++++++----- bazel/labgrid/mock/coordinator/test.py | 13 +++- bazel/labgrid/mock/crossbar/config.yaml | 2 +- bazel/labgrid/mock/crossbar/main.py | 11 +++- 11 files changed, 149 insertions(+), 85 deletions(-) diff --git a/.gitignore b/.gitignore index 49e1020e..3cc0e8e8 100644 --- a/.gitignore +++ b/.gitignore @@ -2,5 +2,4 @@ /bazel-* /.bazelrc.user /node_modules/ -/public/ -/.vscode/ \ No newline at end of file +/public/ \ No newline at end of file diff --git a/bazel/labgrid/client/BUILD.bazel b/bazel/labgrid/client/BUILD.bazel index a1d0586d..458ba20d 100644 --- a/bazel/labgrid/client/BUILD.bazel +++ b/bazel/labgrid/client/BUILD.bazel @@ -4,9 +4,9 @@ py_library( name = "client", srcs = [ "__init__.py", + "common.py", "lock.py", "reserve.py", - "common.py" ], data = ["//labgrid/client"], visibility = ["//visibility:public"], diff --git a/bazel/labgrid/client/common.py b/bazel/labgrid/client/common.py index 48c87104..0ed19dc1 100644 --- a/bazel/labgrid/client/common.py +++ b/bazel/labgrid/client/common.py @@ -1,7 +1,25 @@ -from subprocess import PIPE, run +import yaml +from subprocess import run, PIPE -def labgrid_client(labgrid_client_path, *args): - cmd = (labgrid_client_path,) + args - process = run(cmd, check=True, stdout=PIPE, encoding="utf-8") - return process +def labgrid_client(labgrid_client_path, *args, **kwargs): + env = None + url_params = () + timeout = None + + if "env" in kwargs: + env = kwargs["env"] + if "url" in kwargs: + url_params = ("-x", kwargs["url"]) + if "timeout" in kwargs: + timeout = kwargs["timeout"] + + cmd = (labgrid_client_path,) + url_params + args + process = run( + cmd, check=True, stdout=PIPE, env=env, timeout=timeout, encoding="utf-8" + ) + + if process.stdout: + return yaml.safe_load(process.stdout) + else: + return [] diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 5d9583da..8590caa9 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -2,23 +2,22 @@ from __future__ import annotations from contextlib import contextmanager from os import environ -from subprocess import DEVNULL, run -from typing import Iterator, Mapping +from typing import Iterator from python.runfiles import Runfiles from .reserve import reserve +from .common import labgrid_client @contextmanager -def lock(tags: Mapping[str, str], username: str = "") -> Iterator[str]: +def lock(reservation: reserve.Reservation) -> Iterator[str]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - with reserve(tags=tags, username=username) as reservation: - env = environ | {"LG_TOKEN": reservation.token} - if username: - env["LG_USERNAME"] = username - run((client, "-p", "+", "lock"), check=True, env=env, stdout=DEVNULL) - try: - yield reservation - finally: - run((client, "-p", "+", "unlock"), check=True, env=env, stdout=DEVNULL) + env = environ | {"LG_TOKEN": reservation.token} + if reservation.owner: + env["LG_USERNAME"] = reservation.owner + labgrid_client(client, "-p", "+", "lock", env=env, url=reservation.url) + try: + yield + finally: + labgrid_client(client, "-p", "+", "unlock", env=env, url=reservation.url) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index e613559e..430883aa 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -1,7 +1,7 @@ #! /usr/bin/env python3 import unittest -import yaml +import os import time import threading from python.runfiles import Runfiles @@ -21,9 +21,19 @@ class TestReservation(unittest.TestCase): ) cls.place_name = "test_place" cls.lg_tags = {"board": "fake_board", "os": "fake_os"} + # in order to allow parallel execution of 2 coordinators, + # the following parameters should be unique accross tests: + # port, log_path + port = 20409 + cls.url = f"ws://localhost:{port}/ws" + cls.log_path = os.path.join(os.getcwd(), TestReservation.__name__) + os.mkdir(cls.log_path) cls.__cm = coordinator( - tags=TestReservation.lg_tags, place=TestReservation.place_name + tags=TestReservation.lg_tags, + place=TestReservation.place_name, + url=TestReservation.url, + log_path=TestReservation.log_path, ) cls.coordinator = cls.__cm.__enter__() @@ -37,15 +47,18 @@ class TestReservation(unittest.TestCase): Args: place_name (string): The given resource place name in labgrid. """ - process = labgrid_client( - TestReservation.labgrid_client_path, "-p", name, "show" + show_command_dict = labgrid_client( + TestReservation.labgrid_client_path, + "-p", + name, + "show", + url=TestReservation.url, ) - show_command_dict = yaml.safe_load(process.stdout) place_details = list(show_command_dict.values())[0] return place_details - def _reserve_and_sleep(self, lg_tags, timeout): - with reserve(lg_tags): + def _reserve_and_sleep(self, lg_tags, timeout, url): + with reserve(tags=lg_tags, url=url): time.sleep(timeout) def test_reserve_and_lock_token_verification(self): @@ -57,20 +70,24 @@ class TestReservation(unittest.TestCase): lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name username = "user_test" - with lock(lg_tags, username) as reservation: - dict = self._get_place(place_name) - place_token = dict["reservation"] - owner = dict["acquired"].split("/")[-1] - self.assertEqual( - place_token, - reservation.token, - f"Reservation token mismatch: {place_token} != {reservation.token}", - ) - self.assertEqual( - owner, - username, - f"Username {username} is not acquired the reservation (owned by {owner})", - ) + url = TestReservation.url + with reserve( + tags=lg_tags, timeout=3, username=username, url=url + ) as reservation: + with lock(reservation): + dict = self._get_place(place_name) + place_token = dict["reservation"] + owner = dict["acquired"].split("/")[-1] + self.assertEqual( + place_token, + reservation.token, + f"Reservation token mismatch: {place_token} != {reservation.token}", + ) + self.assertEqual( + owner, + username, + f"Username {username} is not acquired the reservation (owned by {owner})", + ) dict = self._get_place(place_name) self.assertNotEqual(dict["acquired"], None, "Reservation was not unlocked") @@ -83,9 +100,11 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name + url = TestReservation.url with self.assertRaises(Exception): - with lock(lg_tags): - raise Exception("This is a test exception") + with reserve(tags=lg_tags, url=url) as reservation: + with lock(reservation): + raise Exception("This is a test exception") # waiting for reservation's new status to be updated time.sleep(1) @@ -99,9 +118,10 @@ class TestReservation(unittest.TestCase): Test that a reservation throws error when failed to reserve """ lg_tags = TestReservation.lg_tags - with reserve(lg_tags): + url = TestReservation.url + with reserve(tags=lg_tags, url=url): with self.assertRaises(RuntimeError): - with reserve(lg_tags, timeout=1): + with reserve(tags=lg_tags, timeout=1, url=url): pass def test_waiting_reserve_is_acquired_eventually(self): @@ -113,10 +133,11 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name + url = TestReservation.url - threading.Thread(target=self._reserve_and_sleep, args=(lg_tags, 1)).start() + threading.Thread(target=self._reserve_and_sleep, args=(lg_tags, 1, url)).start() - with reserve(lg_tags, 15) as reservation: + with reserve(tags=lg_tags, timeout=25, url=url) as reservation: # if we entered here p1 process is already terminated dict = self._get_place(place_name) place_token = dict["reservation"] diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index 5b3a5fff..47cd90bf 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -3,33 +3,42 @@ from __future__ import annotations from os import environ from contextlib import contextmanager from dataclasses import dataclass -from subprocess import DEVNULL, PIPE, TimeoutExpired, run +from subprocess import ( + TimeoutExpired, +) from typing import Iterator, Mapping -import yaml from python.runfiles import Runfiles +from .common import labgrid_client @dataclass(frozen=True) class Reservation: token: str owner: str + url: str @contextmanager def reserve( - tags: Mapping[str, str], timeout: int = 3, username: str = "" + tags: Mapping[str, str], + timeout: int = 3, + username: str = "", + url="ws://localhost:20408/ws", ) -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + # FIXME: no way to provide a timeout to the CLI + # FIXME: no way to know if tags will ever be satisfied # FIXME: does not cancel reservation if killed env = environ if username: env["LG_USERNAME"] = username - cmd = (client, "reserve", *(f"{k}={v}" for k, v in tags.items())) - process = run(cmd, check=True, env=env, stdout=PIPE, encoding="utf-8") - reservation_dict = yaml.safe_load(process.stdout) + + reservation_dict = labgrid_client( + client, "reserve", *(f"{k}={v}" for k, v in tags.items()), env=env, url=url + ) reservation_dict_values = list(reservation_dict.values())[0] token = reservation_dict_values["token"] owner = reservation_dict_values["owner"].split("/")[-1] @@ -37,18 +46,12 @@ def reserve( assert token try: - run( - (client, "wait", token), - check=True, - env=env, - timeout=timeout, - stdout=DEVNULL, - ) + labgrid_client(client, "wait", token, env=env, timeout=timeout, url=url) except TimeoutExpired: - run((client, "cancel-reservation", token), env=env, check=True) + labgrid_client(client, "cancel-reservation", token, env=env, url=url) raise RuntimeError("Failed to allocate reservation") try: - yield Reservation(token=token, owner=owner) + yield Reservation(token=token, owner=owner, url=url) finally: - run((client, "cancel-reservation", token), env=env, check=True) + labgrid_client(client, "cancel-reservation", token, env=env, url=url) diff --git a/bazel/labgrid/mock/coordinator/BUILD.bazel b/bazel/labgrid/mock/coordinator/BUILD.bazel index 10ff95ad..5faf224a 100644 --- a/bazel/labgrid/mock/coordinator/BUILD.bazel +++ b/bazel/labgrid/mock/coordinator/BUILD.bazel @@ -13,6 +13,7 @@ py_library( srcs = ["main.py"], data = [ "process.py", + "//bazel/labgrid/client", "//bazel/labgrid/mock/crossbar", "//bazel/labgrid/mock/exporter", "//labgrid/client", @@ -30,7 +31,6 @@ py_library( py_test( name = "test", srcs = ["test.py"], - tags = ["exclusive"], # set as exclusive to avoid run in parallel with reservation_test deps = [ ":coordinator", "@rules_python//python/runfiles", diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index 35a053b9..f16964e3 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -3,13 +3,14 @@ from __future__ import annotations import socket from contextlib import contextmanager from dataclasses import dataclass -from subprocess import PIPE, run from typing import Iterator, Mapping +from urllib.parse import urlparse from python.runfiles import Runfiles from bazel.labgrid.mock.crossbar.main import crossbar from bazel.labgrid.mock.exporter.main import exporter +from bazel.labgrid.client.common import labgrid_client from .process import coordinator_process @@ -26,35 +27,40 @@ class Coordinator: """The name of the place to create.""" -def _labgrid_client(*args, **kwargs): - runfiles = Runfiles.Create() - client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - cmd = (client, *args) - check = kwargs.pop("check", True) - run(cmd, check=check, stdout=PIPE, encoding="utf-8") - - @contextmanager def coordinator( tags: Mapping[str, str], place: str = "test_place", url: str = "ws://127.0.0.1:20408/ws", # web_socket_address + log_path: str = "", ) -> Iterator[Coordinator]: - with crossbar(): - with coordinator_process(): - _labgrid_client("--place", place, "create") + runfiles = Runfiles.Create() + client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + parsed_url = urlparse(url) + port = parsed_url.port + + with crossbar(port=port, log_path=log_path): + with coordinator_process(url=url): + labgrid_client(client, "--place", place, "create", url=url) try: - _labgrid_client( - "--place", place, "set-tags", *(f"{k}={v}" for k, v in tags.items()) + labgrid_client( + client, + "--place", + place, + "set-tags", + *(f"{k}={v}" for k, v in tags.items()), + url=url, ) - with exporter(): + with exporter(url=url): # add resources to the "place" - _labgrid_client( + labgrid_client( + client, "--place", place, "add-match", f"{socket.gethostname()}/targets/NetworkService", + url=url, ) yield Coordinator( url=url, @@ -62,4 +68,4 @@ def coordinator( place=place, ) finally: - _labgrid_client("--place", place, "delete") + labgrid_client(client, "--place", place, "delete", url=url) diff --git a/bazel/labgrid/mock/coordinator/test.py b/bazel/labgrid/mock/coordinator/test.py index dbabade3..3b084e61 100644 --- a/bazel/labgrid/mock/coordinator/test.py +++ b/bazel/labgrid/mock/coordinator/test.py @@ -1,3 +1,4 @@ +import os import socket from subprocess import PIPE, run from unittest import TestCase, main @@ -10,7 +11,17 @@ from bazel.labgrid.mock.coordinator.main import coordinator class CoordinatorTestCase(TestCase): @classmethod def setUpClass(cls) -> None: - cls.__cm = coordinator(tags={"device": "fake"}, place="test_place") + # in order to allow parallel execution of 2 coordinators, + # the following parameters should be unique accross tests: + # port, log_path + port = 20410 + cls.url = f"ws://localhost:{port}/ws" + cls.log_path = os.path.join(os.getcwd(), CoordinatorTestCase.__name__) + os.mkdir(cls.log_path) + + cls.__cm = coordinator( + tags={"device": "fake"}, place="test_place", url=CoordinatorTestCase.url + ) cls.coordinator = cls.__cm.__enter__() runfiles = Runfiles.Create() cls._client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") diff --git a/bazel/labgrid/mock/crossbar/config.yaml b/bazel/labgrid/mock/crossbar/config.yaml index 5684ca05..e614d980 100644 --- a/bazel/labgrid/mock/crossbar/config.yaml +++ b/bazel/labgrid/mock/crossbar/config.yaml @@ -21,7 +21,7 @@ workers: - type: web endpoint: type: tcp - port: 20408 + port: $CROSSBAR_PORT paths: /: type: static diff --git a/bazel/labgrid/mock/crossbar/main.py b/bazel/labgrid/mock/crossbar/main.py index 5c89d650..90e2721c 100644 --- a/bazel/labgrid/mock/crossbar/main.py +++ b/bazel/labgrid/mock/crossbar/main.py @@ -1,4 +1,5 @@ import multiprocessing +from os import environ from contextlib import contextmanager from subprocess import PIPE, Popen from typing import Iterator @@ -36,7 +37,8 @@ def crossbar_status(process, timeout: int = 10) -> Iterator[None]: @contextmanager -def crossbar(timeout: int = 10) -> Iterator[int]: +def crossbar(timeout: int = 10, port: int = 20408, log_path="") -> Iterator[int]: + env = environ | {"CROSSBAR_PORT": str(port)} runfiles = Runfiles.Create() crossbar = runfiles.Rlocation( "rules_labgrid/bazel/labgrid/mock/crossbar/crossbar_binary" @@ -45,6 +47,9 @@ def crossbar(timeout: int = 10) -> Iterator[int]: "rules_labgrid/bazel/labgrid/mock/crossbar/config.yaml" ) cmd = (crossbar, "start", "--config", data_file_path) + if log_path: + cmd += ("--cbdir", log_path) + process = Popen( cmd, stdout=PIPE, @@ -52,6 +57,7 @@ def crossbar(timeout: int = 10) -> Iterator[int]: encoding="utf-8", universal_newlines=True, bufsize=1, + env=env, ) try: pid = process.pid @@ -60,6 +66,7 @@ def crossbar(timeout: int = 10) -> Iterator[int]: yield pid finally: process.terminate() + # cmd = (crossbar, "stop", "--cbdir", "/data_sdb/work/ragnarok/labgrid/rules_labgrid/bazel/labgrid/client") cmd = (crossbar, "stop") - process = Popen(cmd, stdout=PIPE, stderr=PIPE, encoding="utf-8") + process = Popen(cmd, stdout=PIPE, stderr=PIPE, env=env, encoding="utf-8") process.wait() -- GitLab From bd6ffa7c45725465e75f092a3da412a647bbd516 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 26 Nov 2024 11:47:14 +0200 Subject: [PATCH 10/13] feat: Adding port manager --- .gitlab-ci.yml | 2 +- bazel/labgrid/client/reservation_test.py | 8 ++-- bazel/labgrid/mock/coordinator/main.py | 47 ++++++++++++++++++++++++ bazel/labgrid/mock/coordinator/test.py | 8 ++-- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d5d6e75a..af1e366c 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,7 +71,7 @@ test: # FIXME: enable remote builds # - remote script: - - cd "${ROOT}"; bazelisk test --config="${CONFIG}" //... + - cd "${ROOT}"; bazelisk test --config="${CONFIG}" //... --test_output=all # TODO: switch this out for `rules_semantic_release` semantic-release: diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index 430883aa..1d341226 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -6,7 +6,7 @@ import time import threading from python.runfiles import Runfiles -from bazel.labgrid.mock.coordinator.main import coordinator +from bazel.labgrid.mock.coordinator.main import coordinator, port_manager # context managers that perform client operations: from bazel.labgrid.client import lock, reserve, labgrid_client @@ -23,9 +23,8 @@ class TestReservation(unittest.TestCase): cls.lg_tags = {"board": "fake_board", "os": "fake_os"} # in order to allow parallel execution of 2 coordinators, # the following parameters should be unique accross tests: - # port, log_path - port = 20409 - cls.url = f"ws://localhost:{port}/ws" + cls.port = port_manager.get_unused_port() + cls.url = f"ws://localhost:{cls.port}/ws" cls.log_path = os.path.join(os.getcwd(), TestReservation.__name__) os.mkdir(cls.log_path) @@ -40,6 +39,7 @@ class TestReservation(unittest.TestCase): @classmethod def tearDownClass(cls): cls.__cm.__exit__(None, None, None) + port_manager.release_port(cls.port) def _get_place(self, name): """Execute the labgrid client show command, parse the output diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index f16964e3..d468317c 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -5,6 +5,8 @@ from contextlib import contextmanager from dataclasses import dataclass from typing import Iterator, Mapping from urllib.parse import urlparse +import threading +import datetime # chen from python.runfiles import Runfiles @@ -69,3 +71,48 @@ def coordinator( ) finally: labgrid_client(client, "--place", place, "delete", url=url) + + +class PortManager: + _instance = None + _default_port = 20408 + _used_ports = [] + _lock = threading.Lock() + + def __new__(cls): + print("chen111") + if cls._instance is None: + cls._instance = super().__new__(cls) + print("chen - new instance") + else: + print("chen - existing instance") + return cls._instance + + def get_unused_port(self): + with self._lock: + print( + f"chen - {datetime.datetime.now()} - entered get_unused_port, used_ports={self._used_ports}" + ) + new_port = 0 + if len(self._used_ports) == 0: + new_port = self._default_port + self._used_ports.append(new_port) + else: + new_port = self._used_ports[-1] + 1 + self._used_ports.append(new_port) + + return new_port + + def release_port(self, port): + with self._lock: + print( + f"chen - {datetime.datetime.now()} - entered release_port, port={port}, used_ports={self._used_ports}" + ) + if port in self._used_ports: + self._used_ports.remove(port) + print( + f"chen - {datetime.datetime.now()} - release_port, used_ports={self._used_ports}" + ) + + +port_manager = PortManager() diff --git a/bazel/labgrid/mock/coordinator/test.py b/bazel/labgrid/mock/coordinator/test.py index 3b084e61..8a61381a 100644 --- a/bazel/labgrid/mock/coordinator/test.py +++ b/bazel/labgrid/mock/coordinator/test.py @@ -5,7 +5,7 @@ from unittest import TestCase, main from python.runfiles import Runfiles -from bazel.labgrid.mock.coordinator.main import coordinator +from bazel.labgrid.mock.coordinator.main import coordinator, port_manager class CoordinatorTestCase(TestCase): @@ -14,8 +14,8 @@ class CoordinatorTestCase(TestCase): # in order to allow parallel execution of 2 coordinators, # the following parameters should be unique accross tests: # port, log_path - port = 20410 - cls.url = f"ws://localhost:{port}/ws" + cls.port = port_manager.get_unused_port() + cls.url = f"ws://localhost:{cls.port}/ws" cls.log_path = os.path.join(os.getcwd(), CoordinatorTestCase.__name__) os.mkdir(cls.log_path) @@ -29,6 +29,8 @@ class CoordinatorTestCase(TestCase): @classmethod def tearDownClass(cls) -> None: cls.__cm.__exit__(None, None, None) + port_manager.release_port(cls.port) + raise Exception("chen- exception") def client(self, *args: str) -> str: env = {"LG_CROSSBAR": self.coordinator.url} -- GitLab From aac21c81209fc92561d464cd8989984e75f3eca4 Mon Sep 17 00:00:00 2001 From: chehay01 Date: Tue, 26 Nov 2024 16:24:17 +0200 Subject: [PATCH 11/13] feat: Use portrange instead PortManager --- .gitlab-ci.yml | 2 +- bazel/labgrid/client/reservation_test.py | 18 ++++---- bazel/labgrid/mock/coordinator/main.py | 53 +----------------------- bazel/labgrid/mock/coordinator/test.py | 10 +---- bazel/labgrid/mock/crossbar/config.yaml | 2 +- bazel/labgrid/mock/crossbar/main.py | 31 ++++++++------ 6 files changed, 32 insertions(+), 84 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index af1e366c..d5d6e75a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -71,7 +71,7 @@ test: # FIXME: enable remote builds # - remote script: - - cd "${ROOT}"; bazelisk test --config="${CONFIG}" //... --test_output=all + - cd "${ROOT}"; bazelisk test --config="${CONFIG}" //... # TODO: switch this out for `rules_semantic_release` semantic-release: diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index 1d341226..a5ec11c0 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -6,7 +6,7 @@ import time import threading from python.runfiles import Runfiles -from bazel.labgrid.mock.coordinator.main import coordinator, port_manager +from bazel.labgrid.mock.coordinator.main import coordinator # context managers that perform client operations: from bazel.labgrid.client import lock, reserve, labgrid_client @@ -22,16 +22,13 @@ class TestReservation(unittest.TestCase): cls.place_name = "test_place" cls.lg_tags = {"board": "fake_board", "os": "fake_os"} # in order to allow parallel execution of 2 coordinators, - # the following parameters should be unique accross tests: - cls.port = port_manager.get_unused_port() - cls.url = f"ws://localhost:{cls.port}/ws" + # port and log_path should be unique cls.log_path = os.path.join(os.getcwd(), TestReservation.__name__) os.mkdir(cls.log_path) cls.__cm = coordinator( tags=TestReservation.lg_tags, place=TestReservation.place_name, - url=TestReservation.url, log_path=TestReservation.log_path, ) cls.coordinator = cls.__cm.__enter__() @@ -39,7 +36,6 @@ class TestReservation(unittest.TestCase): @classmethod def tearDownClass(cls): cls.__cm.__exit__(None, None, None) - port_manager.release_port(cls.port) def _get_place(self, name): """Execute the labgrid client show command, parse the output @@ -52,7 +48,7 @@ class TestReservation(unittest.TestCase): "-p", name, "show", - url=TestReservation.url, + url=TestReservation.coordinator.url, ) place_details = list(show_command_dict.values())[0] return place_details @@ -70,7 +66,7 @@ class TestReservation(unittest.TestCase): lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name username = "user_test" - url = TestReservation.url + url = TestReservation.coordinator.url with reserve( tags=lg_tags, timeout=3, username=username, url=url ) as reservation: @@ -100,7 +96,7 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name - url = TestReservation.url + url = TestReservation.coordinator.url with self.assertRaises(Exception): with reserve(tags=lg_tags, url=url) as reservation: with lock(reservation): @@ -118,7 +114,7 @@ class TestReservation(unittest.TestCase): Test that a reservation throws error when failed to reserve """ lg_tags = TestReservation.lg_tags - url = TestReservation.url + url = TestReservation.coordinator.url with reserve(tags=lg_tags, url=url): with self.assertRaises(RuntimeError): with reserve(tags=lg_tags, timeout=1, url=url): @@ -133,7 +129,7 @@ class TestReservation(unittest.TestCase): """ lg_tags = TestReservation.lg_tags place_name = TestReservation.place_name - url = TestReservation.url + url = TestReservation.coordinator.url threading.Thread(target=self._reserve_and_sleep, args=(lg_tags, 1, url)).start() diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index d468317c..eab3a997 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -5,8 +5,6 @@ from contextlib import contextmanager from dataclasses import dataclass from typing import Iterator, Mapping from urllib.parse import urlparse -import threading -import datetime # chen from python.runfiles import Runfiles @@ -33,15 +31,13 @@ class Coordinator: def coordinator( tags: Mapping[str, str], place: str = "test_place", - url: str = "ws://127.0.0.1:20408/ws", # web_socket_address log_path: str = "", ) -> Iterator[Coordinator]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - parsed_url = urlparse(url) - port = parsed_url.port - with crossbar(port=port, log_path=log_path): + with crossbar(log_path=log_path) as port: + url = f"ws://127.0.0.1:{port}/ws" with coordinator_process(url=url): labgrid_client(client, "--place", place, "create", url=url) try: @@ -71,48 +67,3 @@ def coordinator( ) finally: labgrid_client(client, "--place", place, "delete", url=url) - - -class PortManager: - _instance = None - _default_port = 20408 - _used_ports = [] - _lock = threading.Lock() - - def __new__(cls): - print("chen111") - if cls._instance is None: - cls._instance = super().__new__(cls) - print("chen - new instance") - else: - print("chen - existing instance") - return cls._instance - - def get_unused_port(self): - with self._lock: - print( - f"chen - {datetime.datetime.now()} - entered get_unused_port, used_ports={self._used_ports}" - ) - new_port = 0 - if len(self._used_ports) == 0: - new_port = self._default_port - self._used_ports.append(new_port) - else: - new_port = self._used_ports[-1] + 1 - self._used_ports.append(new_port) - - return new_port - - def release_port(self, port): - with self._lock: - print( - f"chen - {datetime.datetime.now()} - entered release_port, port={port}, used_ports={self._used_ports}" - ) - if port in self._used_ports: - self._used_ports.remove(port) - print( - f"chen - {datetime.datetime.now()} - release_port, used_ports={self._used_ports}" - ) - - -port_manager = PortManager() diff --git a/bazel/labgrid/mock/coordinator/test.py b/bazel/labgrid/mock/coordinator/test.py index 8a61381a..9933959f 100644 --- a/bazel/labgrid/mock/coordinator/test.py +++ b/bazel/labgrid/mock/coordinator/test.py @@ -5,7 +5,7 @@ from unittest import TestCase, main from python.runfiles import Runfiles -from bazel.labgrid.mock.coordinator.main import coordinator, port_manager +from bazel.labgrid.mock.coordinator.main import coordinator class CoordinatorTestCase(TestCase): @@ -14,14 +14,10 @@ class CoordinatorTestCase(TestCase): # in order to allow parallel execution of 2 coordinators, # the following parameters should be unique accross tests: # port, log_path - cls.port = port_manager.get_unused_port() - cls.url = f"ws://localhost:{cls.port}/ws" cls.log_path = os.path.join(os.getcwd(), CoordinatorTestCase.__name__) os.mkdir(cls.log_path) - cls.__cm = coordinator( - tags={"device": "fake"}, place="test_place", url=CoordinatorTestCase.url - ) + cls.__cm = coordinator(tags={"device": "fake"}, place="test_place") cls.coordinator = cls.__cm.__enter__() runfiles = Runfiles.Create() cls._client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") @@ -29,8 +25,6 @@ class CoordinatorTestCase(TestCase): @classmethod def tearDownClass(cls) -> None: cls.__cm.__exit__(None, None, None) - port_manager.release_port(cls.port) - raise Exception("chen- exception") def client(self, *args: str) -> str: env = {"LG_CROSSBAR": self.coordinator.url} diff --git a/bazel/labgrid/mock/crossbar/config.yaml b/bazel/labgrid/mock/crossbar/config.yaml index e614d980..b09bab8e 100644 --- a/bazel/labgrid/mock/crossbar/config.yaml +++ b/bazel/labgrid/mock/crossbar/config.yaml @@ -21,7 +21,7 @@ workers: - type: web endpoint: type: tcp - port: $CROSSBAR_PORT + portrange: [1024,65535] paths: /: type: static diff --git a/bazel/labgrid/mock/crossbar/main.py b/bazel/labgrid/mock/crossbar/main.py index 90e2721c..4c420826 100644 --- a/bazel/labgrid/mock/crossbar/main.py +++ b/bazel/labgrid/mock/crossbar/main.py @@ -7,9 +7,11 @@ from typing import Iterator from python.runfiles import Runfiles -def crossbar_status_check(process): +def crossbar_status_check(process, queue): for line in process.stdout: - if "Router worker001 has started" in line: + if "Site starting on " in line: + port = line.split()[-1].strip() + queue.put(port) return @@ -19,8 +21,14 @@ class CrossbarError(RuntimeError): @contextmanager def crossbar_status(process, timeout: int = 10) -> Iterator[None]: + queue = multiprocessing.Queue() monitor_thread = multiprocessing.Process( - target=crossbar_status_check, args=(process,), daemon=True + target=crossbar_status_check, + args=( + process, + queue, + ), + daemon=True, ) monitor_thread.start() try: @@ -29,7 +37,8 @@ def crossbar_status(process, timeout: int = 10) -> Iterator[None]: raise CrossbarError( f"Failed to start the crossbar: {process.stderr.read().strip()}" ) - yield + port = queue.get() + yield port finally: monitor_thread.terminate() monitor_thread.join() @@ -37,8 +46,7 @@ def crossbar_status(process, timeout: int = 10) -> Iterator[None]: @contextmanager -def crossbar(timeout: int = 10, port: int = 20408, log_path="") -> Iterator[int]: - env = environ | {"CROSSBAR_PORT": str(port)} +def crossbar(timeout: int = 10, log_path="") -> Iterator[int]: runfiles = Runfiles.Create() crossbar = runfiles.Rlocation( "rules_labgrid/bazel/labgrid/mock/crossbar/crossbar_binary" @@ -46,6 +54,7 @@ def crossbar(timeout: int = 10, port: int = 20408, log_path="") -> Iterator[int] data_file_path = runfiles.Rlocation( "rules_labgrid/bazel/labgrid/mock/crossbar/config.yaml" ) + # ports are assigned automatically by crossbar using portrange value cmd = (crossbar, "start", "--config", data_file_path) if log_path: cmd += ("--cbdir", log_path) @@ -57,16 +66,14 @@ def crossbar(timeout: int = 10, port: int = 20408, log_path="") -> Iterator[int] encoding="utf-8", universal_newlines=True, bufsize=1, - env=env, ) try: pid = process.pid assert isinstance(pid, int) - with crossbar_status(process): - yield pid + with crossbar_status(process) as port: + yield port finally: process.terminate() - # cmd = (crossbar, "stop", "--cbdir", "/data_sdb/work/ragnarok/labgrid/rules_labgrid/bazel/labgrid/client") - cmd = (crossbar, "stop") - process = Popen(cmd, stdout=PIPE, stderr=PIPE, env=env, encoding="utf-8") + cmd = (crossbar, "stop", "--cbdir", log_path) + process = Popen(cmd, stdout=PIPE, stderr=PIPE, encoding="utf-8") process.wait() -- GitLab From f3b88c2098d473c5d81df222420675476869645d Mon Sep 17 00:00:00 2001 From: chehay01 Date: Wed, 27 Nov 2024 11:20:40 +0200 Subject: [PATCH 12/13] feat: yielding url instead of port --- bazel/labgrid/mock/coordinator/main.py | 3 +-- bazel/labgrid/mock/crossbar/main.py | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index eab3a997..392aacc5 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -36,8 +36,7 @@ def coordinator( runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - with crossbar(log_path=log_path) as port: - url = f"ws://127.0.0.1:{port}/ws" + with crossbar(log_path=log_path) as url: with coordinator_process(url=url): labgrid_client(client, "--place", place, "create", url=url) try: diff --git a/bazel/labgrid/mock/crossbar/main.py b/bazel/labgrid/mock/crossbar/main.py index 4c420826..4976b98a 100644 --- a/bazel/labgrid/mock/crossbar/main.py +++ b/bazel/labgrid/mock/crossbar/main.py @@ -71,7 +71,8 @@ def crossbar(timeout: int = 10, log_path="") -> Iterator[int]: pid = process.pid assert isinstance(pid, int) with crossbar_status(process) as port: - yield port + url = f"ws://127.0.0.1:{port}/ws" + yield url finally: process.terminate() cmd = (crossbar, "stop", "--cbdir", log_path) -- GitLab From 2b0e7e02456cadb320b1a430ac762ec586229bac Mon Sep 17 00:00:00 2001 From: chehay01 Date: Wed, 27 Nov 2024 17:29:34 +0200 Subject: [PATCH 13/13] feat: review fixes --- .gitignore | 2 +- bazel/labgrid/client/common.py | 6 +++++- bazel/labgrid/client/lock.py | 7 ++----- bazel/labgrid/client/reservation_test.py | 8 +------- bazel/labgrid/client/reserve.py | 13 +++++-------- bazel/labgrid/mock/coordinator/main.py | 12 ++---------- bazel/labgrid/mock/coordinator/test.py | 3 +-- 7 files changed, 17 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index 3cc0e8e8..bd4b2dae 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,4 @@ /bazel-* /.bazelrc.user /node_modules/ -/public/ \ No newline at end of file +/public/ diff --git a/bazel/labgrid/client/common.py b/bazel/labgrid/client/common.py index 0ed19dc1..e5e4b707 100644 --- a/bazel/labgrid/client/common.py +++ b/bazel/labgrid/client/common.py @@ -1,12 +1,16 @@ import yaml from subprocess import run, PIPE +from python.runfiles import Runfiles -def labgrid_client(labgrid_client_path, *args, **kwargs): +def labgrid_client(*args, **kwargs): env = None url_params = () timeout = None + runfiles = Runfiles.Create() + labgrid_client_path = runfiles.Rlocation("rules_labgrid/labgrid/client/client") + if "env" in kwargs: env = kwargs["env"] if "url" in kwargs: diff --git a/bazel/labgrid/client/lock.py b/bazel/labgrid/client/lock.py index 8590caa9..0972779a 100644 --- a/bazel/labgrid/client/lock.py +++ b/bazel/labgrid/client/lock.py @@ -3,7 +3,6 @@ from __future__ import annotations from contextlib import contextmanager from os import environ from typing import Iterator -from python.runfiles import Runfiles from .reserve import reserve from .common import labgrid_client @@ -11,13 +10,11 @@ from .common import labgrid_client @contextmanager def lock(reservation: reserve.Reservation) -> Iterator[str]: - runfiles = Runfiles.Create() - client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") env = environ | {"LG_TOKEN": reservation.token} if reservation.owner: env["LG_USERNAME"] = reservation.owner - labgrid_client(client, "-p", "+", "lock", env=env, url=reservation.url) + labgrid_client("-p", "+", "lock", env=env, url=reservation.url) try: yield finally: - labgrid_client(client, "-p", "+", "unlock", env=env, url=reservation.url) + labgrid_client("-p", "+", "unlock", env=env, url=reservation.url) diff --git a/bazel/labgrid/client/reservation_test.py b/bazel/labgrid/client/reservation_test.py index a5ec11c0..0847aed4 100644 --- a/bazel/labgrid/client/reservation_test.py +++ b/bazel/labgrid/client/reservation_test.py @@ -4,7 +4,6 @@ import unittest import os import time import threading -from python.runfiles import Runfiles from bazel.labgrid.mock.coordinator.main import coordinator @@ -15,14 +14,10 @@ from bazel.labgrid.client import lock, reserve, labgrid_client class TestReservation(unittest.TestCase): @classmethod def setUpClass(cls): - runfiles = Runfiles.Create() - cls.labgrid_client_path = runfiles.Rlocation( - "rules_labgrid/labgrid/client/client" - ) cls.place_name = "test_place" cls.lg_tags = {"board": "fake_board", "os": "fake_os"} # in order to allow parallel execution of 2 coordinators, - # port and log_path should be unique + # unique log_path must be set cls.log_path = os.path.join(os.getcwd(), TestReservation.__name__) os.mkdir(cls.log_path) @@ -44,7 +39,6 @@ class TestReservation(unittest.TestCase): place_name (string): The given resource place name in labgrid. """ show_command_dict = labgrid_client( - TestReservation.labgrid_client_path, "-p", name, "show", diff --git a/bazel/labgrid/client/reserve.py b/bazel/labgrid/client/reserve.py index 47cd90bf..f0ccfcda 100644 --- a/bazel/labgrid/client/reserve.py +++ b/bazel/labgrid/client/reserve.py @@ -21,10 +21,7 @@ class Reservation: @contextmanager def reserve( - tags: Mapping[str, str], - timeout: int = 3, - username: str = "", - url="ws://localhost:20408/ws", + tags: Mapping[str, str], url: str, timeout: int = 3, username: str = "" ) -> Iterator[Reservation]: runfiles = Runfiles.Create() client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") @@ -37,7 +34,7 @@ def reserve( env["LG_USERNAME"] = username reservation_dict = labgrid_client( - client, "reserve", *(f"{k}={v}" for k, v in tags.items()), env=env, url=url + "reserve", *(f"{k}={v}" for k, v in tags.items()), env=env, url=url ) reservation_dict_values = list(reservation_dict.values())[0] token = reservation_dict_values["token"] @@ -46,12 +43,12 @@ def reserve( assert token try: - labgrid_client(client, "wait", token, env=env, timeout=timeout, url=url) + labgrid_client("wait", token, env=env, timeout=timeout, url=url) except TimeoutExpired: - labgrid_client(client, "cancel-reservation", token, env=env, url=url) + labgrid_client("cancel-reservation", token, env=env, url=url) raise RuntimeError("Failed to allocate reservation") try: yield Reservation(token=token, owner=owner, url=url) finally: - labgrid_client(client, "cancel-reservation", token, env=env, url=url) + labgrid_client("cancel-reservation", token, env=env, url=url) diff --git a/bazel/labgrid/mock/coordinator/main.py b/bazel/labgrid/mock/coordinator/main.py index 392aacc5..835641e5 100644 --- a/bazel/labgrid/mock/coordinator/main.py +++ b/bazel/labgrid/mock/coordinator/main.py @@ -4,9 +4,6 @@ import socket from contextlib import contextmanager from dataclasses import dataclass from typing import Iterator, Mapping -from urllib.parse import urlparse - -from python.runfiles import Runfiles from bazel.labgrid.mock.crossbar.main import crossbar from bazel.labgrid.mock.exporter.main import exporter @@ -33,15 +30,11 @@ def coordinator( place: str = "test_place", log_path: str = "", ) -> Iterator[Coordinator]: - runfiles = Runfiles.Create() - client = runfiles.Rlocation("rules_labgrid/labgrid/client/client") - with crossbar(log_path=log_path) as url: with coordinator_process(url=url): - labgrid_client(client, "--place", place, "create", url=url) + labgrid_client("--place", place, "create", url=url) try: labgrid_client( - client, "--place", place, "set-tags", @@ -52,7 +45,6 @@ def coordinator( with exporter(url=url): # add resources to the "place" labgrid_client( - client, "--place", place, "add-match", @@ -65,4 +57,4 @@ def coordinator( place=place, ) finally: - labgrid_client(client, "--place", place, "delete", url=url) + labgrid_client("--place", place, "delete", url=url) diff --git a/bazel/labgrid/mock/coordinator/test.py b/bazel/labgrid/mock/coordinator/test.py index 9933959f..51a26aee 100644 --- a/bazel/labgrid/mock/coordinator/test.py +++ b/bazel/labgrid/mock/coordinator/test.py @@ -12,8 +12,7 @@ class CoordinatorTestCase(TestCase): @classmethod def setUpClass(cls) -> None: # in order to allow parallel execution of 2 coordinators, - # the following parameters should be unique accross tests: - # port, log_path + # unique log_path must be set cls.log_path = os.path.join(os.getcwd(), CoordinatorTestCase.__name__) os.mkdir(cls.log_path) -- GitLab