Improves route control script #710

saltiyazan · 2023-08-02T13:41:56Z

Improve the route_control.py script by adding logging, error handling, and threading.

This PR was motivated by the goal of improving logging and adding a thread that retries pinging endpoints that are not in the ARP table. (In case of packet loss for example when pinged for the first time). Other refactoring work has been done as well to improve the readability of the code and reduce complexity.

Adds threading to ping IPs that don't have entries in the ARP table.
Adds logging.
Improves Error handling.
Breaks the code into classes.

Breaks it into classes Adds type hints and docstrings Adds error handling Adds logging

omecproject · 2023-08-02T13:41:59Z

Can one of the admins verify this patch?

Adds new test cases Fixes lint and static issues

gruyaume

Overall I feel like this code is pretty complex for what it's supposed to do. This PR increases the amount of lines of code by 50%. I understand that we have improved error handling but it also feels like we increased complexity

I understand why we want to go OOP, but the class selection and separation is "weird". I think we should spend a bit more time designing this. A class diagram could be useful.

I'm also yet unconvinced of the need for threads.

gruyaume · 2023-08-07T17:29:36Z

conf/test_route_control.py

We probably should place tests in the project's test directory

I agree, we can move it no problem, but looking at the structure of the project and at some other test files being placed in many different locations, it becomes less clear to me which directory should hold these tests.

gruyaume · 2023-08-07T17:31:09Z

.github/workflows/py-workflow.yml

+    - name: Install dependencies
+      run: ./venv/bin/pip install -r conf/test_requirements.txt
+
+    - name: Lint with flake8


Are we sure about introducing mypy/flake8 here? I'd have a different PR for this as there can be discussions on theses points alone independent of the code implementation below

You are right, this can and maybe should be part of different discussion :)
I removed flake8 and mypy for now.

gruyaume · 2023-08-07T17:31:50Z

.github/workflows/py-workflow.yml

(not related to this file)

In the PR description, please add context as to why we are making those changes in the first place.

gruyaume · 2023-08-07T17:32:59Z

conf/test_route_control.py

+    def tearDown(self):
+        self.ipdb.release()
+
+    def test_parse_valid_entry_and_dst_len_is_zero(self):


Why drop the given/when/then test naming convention?

Looking at tests in the projects, naming does not seem to follow a specific convention, tests seem to have descriptive names instead.
What do you think should use given/when/then? (Happy to do so)

I went back to given/when/then ✅

gruyaume · 2023-08-07T17:35:11Z

conf/route_control.py

    parser = argparse.ArgumentParser(
-        description='Basic IPv4 Routing Controller')
-    parser.add_argument('-i',
-                        type=str,
-                        nargs='+',
-                        help='interface(s) to control')
-    parser.add_argument('--ip',
-                        type=str,
-                        default='localhost',
-                        help='BESSD address')
-    parser.add_argument('--port', type=str, default='10514', help='BESSD port')
-
-    # for holding command-line arguments
-    global args
+        description="Basic IPv4 Routing Controller"
+    )
+    parser.add_argument(
+        "-i", type=str, nargs="+", help="interface(s) to control"
+    )
+    parser.add_argument(
+        "--ip", type=str, default="localhost", help="BESSD address"
+    )
+    parser.add_argument(
+        "--port", type=str, default="10514", help="BESSD port"
+    )
+
    args = parser.parse_args()


This whole block could have its own function

gruyaume · 2023-08-07T17:43:40Z

conf/route_control.py

+        self.ipdb = ipdb
+        self.bess_controller = bess_controller
+        self.route_parser = route_parser
+        self.ping_missing = Thread(target=self._ping_missing_entries, daemon=True)  # noqa: E501


Does this really need to be multi-threaded? It feels overkilled for what we really need

The variable name is somewhat confusing, it's really a thread that we are talking about here.

It already is multi-threaded through ipdb and the event callback. I think this design is better than the alternative (the main thread looping and pinging missing entries), as the thread's job is really clear. Also, if in the future we need to react to other events, or execute other checks, we can start small threads with clear focus.

gruyaume · 2023-08-07T17:48:14Z

conf/route_control.py


+DESTINATION_IP = "NDA_DST"


"NDA_DST" is not an IP address. These variables are confusing

Improved the naming for these variables in general. ✅

gruyaume · 2023-08-07T17:49:00Z

conf/route_control.py


+    def __init__(self, ipdb):


gruyaume · 2023-08-07T17:49:56Z

conf/route_control.py



-def mac2hex(mac):
-    return int(mac.replace(':', ''), 16)
+class RouteEntryParser:


This is more a function than an object.

I agree, moved.

gruyaume · 2023-08-07T17:52:31Z

conf/route_control.py

+class RouteController:
+    def __init__(
+        self,
+        bess_controller: BessController,


Why does a RouteController have a bess_controller as an attribute?

RouteController uses the interface of BessController to add the routes in BESS.

ghislainbourgeois

Great work, added some comments.

ghislainbourgeois · 2023-08-07T19:42:06Z

conf/route_control.py

+        Args:
+            update_module (str): The name of the module to delete.
+        """
+        for i in range(MAX_RETRIES):


Suggested change

for i in range(MAX_RETRIES):

for _ in range(MAX_RETRIES):

ghislainbourgeois · 2023-08-07T19:46:44Z

conf/route_control.py

+        self.ipdb = ipdb
+        self.bess_controller = bess_controller
+        self.route_parser = route_parser
+        self.ping_missing = Thread(target=self._ping_missing_entries, daemon=True)  # noqa: E501


It already is multi-threaded through ipdb and the event callback. I think this design is better than the alternative (the main thread looping and pinging missing entries), as the thread's job is really clear. Also, if in the future we need to react to other events, or execute other checks, we can start small threads with clear focus.

ghislainbourgeois · 2023-08-07T19:54:52Z

conf/route_control.py

+        unresolved_next_hop = self.unresolved_arp_queries_cache.get(
+            attr_dict[DESTINATION_IP]
+        )
+        gateway_mac = attr_dict[LINK_LAYER_ADDRESS]
+        if unresolved_next_hop:


Suggested change

unresolved_next_hop = self.unresolved_arp_queries_cache.get(

attr_dict[DESTINATION_IP]

)

gateway_mac = attr_dict[LINK_LAYER_ADDRESS]

if unresolved_next_hop:

route_entry = self.unresolved_arp_queries_cache.get(

attr_dict[DESTINATION_IP]

)

gateway_mac = attr_dict[LINK_LAYER_ADDRESS]

if route_entry:

If we find the entry in the cache, it is not an unresolved next hop, since we just received the MAC address.

ghislainbourgeois · 2023-08-07T19:57:24Z

conf/route_control.py

+        Optional[str]: The MAC address of the target IP.
+    """
+    neighbors = ipr.get_neighbours(dst=target_ip)
+    for i in range(len(neighbors)):


Suggested change

for i in range(len(neighbors)):

for neighbor in neighbors:

Renames constants Small changes in the design Extracts arg parsing

gab-arrobo · 2023-08-10T00:24:03Z

Hi @saltiyazan, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:

✒️ 👉 https://cla.opennetworking.org

After signing, make sure to add your Github user ID saltiyazan to the agreement.

For more information or help:" https://wiki.opennetworking.org/x/BgCUI

@saltiyazan, you need to sign the CLA. So, the GitHub Actions can run

gruyaume · 2023-08-14T12:50:02Z

conf/route_control.py

+DELETE_ROUTE_ACTION = "RTM_DELROUTE"
+NEW_ROUTE_ACTION = "RTM_NEWROUTE"
+INTERFACE = "RTA_OIF"
+DESTINATION_IP = "RTA_DST"


Not sure of this variable name, it suggests it's an IP address.

The same applies to a bunch of variables here

I agree with you that it is a bit confusing.
For now I added a "KEY_" prefix, that might make it clearer that these variables hold key literals and not actual values.
Happy to hear any suggestion you might have.

gruyaume · 2023-08-15T12:49:25Z

conf/test-requirements.txt

@@ -0,0 +1,5 @@
+pyroute2
+scapy
+flake8


Do we need flake8/mypy to run the tests?

no, removed

gruyaume · 2023-08-15T12:50:42Z

conf/route_control.py


-# for retrieving neighbor info
-from pyroute2 import IPDB, IPRoute
+from pybess.bess import *  # type: ignore[import]


I'd remove those "# type: ignore[import]" if we're not implementing mypy here

This applies at a couple of places

gruyaume · 2023-08-15T12:54:01Z

conf/route_control.py

+    ipr = IPRoute()
+    ipdb = IPDB()
+    bess_controller = BessController(ip_arg, port_arg)
+    controller = RouteController(


route_controller may be a better variable name

gruyaume · 2023-08-15T12:55:41Z

conf/route_control.py

+        """
+        self.bess = self.get_bess(ip=bess_ip, port=bess_port)
+
+    def get_bess(self, ip: str, port: str) -> "BESS":  # type: ignore[name-defined]


This method should probably be private

gruyaume · 2023-08-15T12:56:55Z

conf/route_control.py

+        """Connects to the BESS daemon."""
+        bess = BESS()  # type: ignore[name-defined]
+        logger.info("Connecting to BESS daemon...")
+        for _ in range(MAX_RETRIES):


MAX_RETRIES should probably be specific to the BessController class and not be global. I'd suggest either changing the name or placing it under the BessController class

moved, same for SLEEP_S

gruyaume · 2023-08-15T12:57:56Z

conf/route_control.py

+            gate_idx (int): Gate of the module used in the route.
+            module_name (str): The name of the module.
+        """
+        logger.info(


No need to have the logging both before and after the action was completed. I think we're good with the other logs (errors or success)

This applies at a couple of places

I did some improvements. ✅

gruyaume · 2023-08-15T13:00:52Z

conf/route_control.py

+                )
+            )
+
+    def del_module(self, module_name) -> None:


We use "create", "link", "update", why not use "delete" (instead of "del")

gruyaume · 2023-08-15T13:01:01Z

conf/route_control.py

+                )
+            )
+
+    def del_module(self, module_name) -> None:


Missing typing for module name

gruyaume · 2023-08-15T13:01:40Z

conf/route_control.py

-
-        link_route_module(bess, gateway_mac, item)
+                logger.info("Module %s destroyed", module_name)
+                logger.info("BESS resume all")


This log doesn't match with what has been done

Moves and renames constatns

Fixes mac_to_hex function Removes unnecessary int conversion

gab-arrobo · 2023-09-12T16:54:59Z

@saltiyazan, I am going to try to review your PR over the weekend. I am pretty busy until Thursday.

gruyaume

Overall looks good but the unit tests could be improved. Our current implementation prevents future refactoring.

gruyaume · 2023-09-12T20:28:38Z

conf/test_route_control.py

+    def test_given_valid_ip_when_validate_ipv4_then_returns_true(self):
+        self.assertTrue(validate_ipv4("192.168.1.1"))
+        self.assertFalse(validate_ipv4("192.168.300.1"))


This should probably be 2 different tests, here we validate both the positive and negative outcomes

gruyaume · 2023-09-12T20:31:20Z

conf/test_route_control.py

+        )
+        self.assertIsNone(result)
+
+    @patch.object(RouteController, "_handle_new_route_entry")


We should not patch private methods. With the state of the tests right now (we patch a _parse_route_entry_msg, _parse_route_entry_msg, _probe_addr and more), we can't refactor the code and expect the unit tests to still pass.

Agreed, I improved the tests.
Now we don't patch anything private.

Removes patches of private functions Uses MAX_GATES-1 as gate index for default routes

gab-arrobo · 2023-09-19T21:56:38Z

ok to test

saltiyazan · 2023-09-20T06:55:43Z

ok to test

Awesome @gab-arrobo , should I run some tests manually that are not ran by the CI? could you please tell me which ones? reading through the testing section I see that "PTF tests for BESS-UPF" might be the one not ran by the CI.

gab-arrobo · 2023-09-26T03:58:48Z

retest this please

gab-arrobo

Hi @saltiyazan, thanks for your contribution and sorry for the time it took me to review it.
There is an issue with your modifications because for some reason the output of the pipeline for the N6 interface (in my case, ens801f1) is not getting properly built/assembled. Attached are two files showing the pipeline. The one named current-routectl.svg shows the UPF pipeline using the current route-control.py script, while the other figure (new-routectl.svg) shows the UPF pipeline using your modified/improved route-control.py script and as you can see in the new-routectl.svg, the ens801f1Routes, ens801f1DstMACCB.... and ens801f1Merge are not getting connected and the route-control.py script is "responsible" for that connection. Please take a look at the issue and properly address it. Thanks!

gab-arrobo

The issue with connecting the modules for the output of the N6 interface is that the ens801f1Routes is trying to connect to the ens801f1DstMAC... using ogate 8191 which is already used by the ens801f1bad_route module. The correct ogate should be 0

gab-arrobo · 2023-10-18T03:19:21Z

conf/route_control.py

+        if route_entry.prefix_len == 0 and route_entry.dest_prefix == "0.0.0.0":
+            gate_idx = self.MAX_GATES - 1


The issue with the pipeline for the output of the N6 interface is due to this if statement. This route 0.0.0.0/0 is/was already added to the routes module and connected to the bad_route module using routes ogate 8191 in the ports.py. So, you cannot use it again the same ogate and this is why the output of the N6 pipeline does not get properly connected/linked.

gab-arrobo · 2023-10-18T03:43:44Z

conf/route_control.py

+        if route_entry.prefix_len == 0 and route_entry.dest_prefix == "0.0.0.0":
+            gate_idx = self.MAX_GATES - 1
        else:
-          print("The IP address {} is valid ipv6 address. Ignore ".format(ipb))
-          return
-    except:
-        print("The IP address {} is invalid".format(item.neighbor_ip))
-        return
-    # Probe ARP request by sending ping
-    send_ping(item.neighbor_ip)
-
-    # Probe ARP request
-    ##send_arp(neighbor_ip, src_mac, item.iface)
-
-
-def parse_new_route(msg):
-    item = NeighborEntry()
-    # Fetch prefix_len
-    item.prefix_len = msg['dst_len']
-    # Default route
-    if item.prefix_len == 0:
-        item.iprange = '0.0.0.0'
-
-    for att in msg['attrs']:
-        if 'RTA_DST' in att:
-            # Fetch IP range
-            # ('RTA_DST', iprange)
-            item.iprange = att[1]
-        if 'RTA_GATEWAY' in att:
-            # Fetch gateway MAC address
-            # ('RTA_GATEWAY', neighbor_ip)
-            item.neighbor_ip = att[1]
-            _mac = fetch_mac(att[1])
-            if not _mac:
-                gateway_mac = 0
-            else:
-                gateway_mac = mac2hex(_mac)
-        if 'RTA_OIF' in att:
-            # Fetch interface name
-            # ('RTA_OIF', iface)
-            item.iface = ipdb.interfaces[int(att[1])].ifname
-
-    if not item.iface in args.i or not item.iprange or not item.neighbor_ip:
-        # Neighbor info is invalid
-        del item
-        return
-
-    # if mac is 0, send ARP request
-    if gateway_mac == 0:
-        print('Adding entry {} in arp probe table. Neighbor: {}'.format(item.iface,item.neighbor_ip))
-        probe_addr(item, ipdb.interfaces[item.iface].address)
-
-    else:  # if gateway_mac is set
-        print('Linking module {}Routes with {}Merge (Dest MAC: {})'.format(
-            item.iface, item.iface, _mac))
-
-        link_route_module(bess, gateway_mac, item)
-
-
-def parse_new_neighbor(msg):
-    for att in msg['attrs']:
-        if 'NDA_DST' in att:
-            # ('NDA_DST', neighbor_ip)
-            neighbor_ip = att[1]
-        if 'NDA_LLADDR' in att:
-            # ('NDA_LLADDR', neighbor_mac)
-            gateway_mac = att[1]
-
-    item = arpcache.get(neighbor_ip)
-    if item:
-        print('Linking module {}Routes with {}Merge (Dest MAC: {})'.format(
-            item.iface, item.iface, gateway_mac))
-
-        # Add route entry, and add item in the registered neighbor cache
-        link_route_module(bess, mac2hex(gateway_mac), item)
-
-        # Remove entry from unresolved arp cache
-        del arpcache[neighbor_ip]
-
-
-def parse_del_route(msg):
-    item = NeighborEntry()
-    for att in msg['attrs']:
-        if 'RTA_DST' in att:
-            # Fetch IP range
-            # ('RTA_DST', iprange)
-            item.iprange = att[1]
-        if 'RTA_GATEWAY' in att:
-            # Fetch gateway MAC address
-            # ('RTA_GATEWAY', neighbor_ip)
-            item.neighbor_ip = att[1]
-        if 'RTA_OIF' in att:
-            # Fetch interface name
-            # ('RTA_OIF', iface)
-            item.iface = ipdb.interfaces[int(att[1])].ifname
+            gate_idx = self._get_gate_idx(route_entry, route_module_name)


Given that the route for 0.0.0.0/0 is already set in the ports.py, a simple solution for the pipeline to be fully built/assembled would be to replace the following:

if route_entry.prefix_len == 0 and route_entry.dest_prefix == "0.0.0.0": gate_idx = self.MAX_GATES - 1 else: gate_idx = self._get_gate_idx(route_entry, route_module_name)

by this:

gate_idx = self._get_gate_idx(route_entry, route_module_name)

Hi @gab-arrobo
Thanks for looking into it. I made the suggested change :) Let me know if it's all good now.

It looks good. I am planning to approve and merge it later tonight. Again, thanks for your contribution.

gab-arrobo

Tested and works as expected.

gab-arrobo · 2023-10-19T17:48:22Z

Hi @saltiyazan, quick question. I just noticed the message below in the GHA. Is it expected?

2023-10-19 17:39:35,954 ERROR Error parsing route entry message
Traceback (most recent call last):
  File "/home/runner/work/upf/upf/conf/route_control.py", line 679, in _parse_route_entry_msg
    attr_dict = dict(route_entry["attrs"])
                     ~~~~~~~~~~~^^^^^^^^^
TypeError: string indices must be integers, not 'str'

saltiyazan · 2023-10-19T19:54:14Z

GHA

Yes, it is expected, this is logged by

try:
            attr_dict = dict(route_entry["attrs"])
        except Exception:
            logger.exception("Error parsing route entry message")
            return None

logger.exception logs the trace I think, and test_given_invalid_route_message_when_parse_message_then_returns_none is expected to run into the error, therefore the log messages.

gab-arrobo · 2023-10-19T19:56:32Z

GHA

Yes, it is expected, this is logged by
try:
            attr_dict = dict(route_entry["attrs"])
        except Exception:
            logger.exception("Error parsing route entry message")
            return None
logger.exception logs the trace I think, and test_given_invalid_route_message_when_parse_message_then_returns_none is expected to run into the error, therefore the log messages.

Ok, thanks for the clarification.

gab-arrobo · 2023-10-19T20:01:37Z

@saltiyazan, any comments/thoughts about the deprecation of IPDB shown in the route-control-tests GHA? Is this something you can help to address?

..2023-10-19 18:16:04,892 WARNING Deprecation warning https://docs.pyroute2.org/ipdb_toc.html
2023-10-19 18:16:04,892 WARNING To remove this DeprecationWarning exception, start IPDB(deprecation_warning=False, ...)
/home/runner/work/upf/upf/venv/lib/python3.11/site-packages/pyroute2/ipdb/main.py:841: DeprecationWarning: IPDB module is deprecated and will be removed in 0.7.1
  warnings.warn(
2023-10-19 18:16:04,893 INFO Mac address found for 192.168.1.1, Mac: 00:1a:2b:3c:4d:5e
.2023-10-19 18:16:04,893 WARNING Deprecation warning https://docs.pyroute2.org/ipdb_toc.html
2023-10-19 18:16:04,893 WARNING To remove this DeprecationWarning exception, start IPDB(deprecation_warning=False, ...)

saltiyazan · 2023-10-21T14:53:39Z

Chore: Use upstream project for UPF configurations canonical/sdcore-upf-bess-rock#10

@gab-arrobo I added it to the list of possible improvements our team could contribute to.

gab-arrobo · 2024-02-05T18:59:00Z

conf/test_route_control.py

+    def test_given_invalid_ip_when_validate_ipv4_then_returns_false(self):
+        self.assertFalse(validate_ipv4("192.168.300.1"))
+
+    def test_given_invalid_ip_when_validate_ipv4_then_returns_false(self):


Hi @saltiyazan, I am working on replacing the IPDB for the NDB and while doing so, I noticed that this test has the exact same name as the previous definition/function. Should it be IPv6 instead of IPv4?

- def test_given_invalid_ip_when_validate_ipv4_then_returns_false(self): + def test_given_invalid_ip_when_validate_ipv6_then_returns_false(self):

Hi @gab-arrobo
It seems like this should have been one test case asserting on different values.

saltiyazan added 2 commits July 24, 2023 15:16

Initial modifications

893317b

General improvements of route_control.py script

6431fe0

Breaks it into classes Adds type hints and docstrings Adds error handling Adds logging

Removes comments

72cb345

saltiyazan force-pushed the improves-route-control-script branch 7 times, most recently from 0f922e1 to 3ea8ee7 Compare August 3, 2023 11:23

Adds initial unit tests

dd0aa92

saltiyazan force-pushed the improves-route-control-script branch from 3ea8ee7 to dd0aa92 Compare August 3, 2023 11:38

saltiyazan added 3 commits August 3, 2023 17:29

Adding more tests

3dfb3ca

Fixes styling and adds new tests

3dec219

Adds new test cases Fixes lint and static issues

Adds github workflow file

9377722

gruyaume suggested changes Aug 7, 2023

View reviewed changes

ghislainbourgeois suggested changes Aug 7, 2023

View reviewed changes

saltiyazan force-pushed the improves-route-control-script branch from 77e6668 to 2d1c0d7 Compare August 9, 2023 15:58

Addresses review comments

d8c96b7

Renames constants Small changes in the design Extracts arg parsing

saltiyazan force-pushed the improves-route-control-script branch from 2d1c0d7 to d8c96b7 Compare August 9, 2023 16:02

Adds new test cases

5092260

saltiyazan requested review from ghislainbourgeois and gruyaume August 14, 2023 08:35

gruyaume reviewed Aug 15, 2023

View reviewed changes

saltiyazan force-pushed the improves-route-control-script branch 2 times, most recently from e917a36 to e5616f0 Compare August 28, 2023 12:35

Addresses review comments

1189218

Moves and renames constatns

saltiyazan force-pushed the improves-route-control-script branch from e5616f0 to 1189218 Compare August 28, 2023 15:22

Addresses review comments

8b88d00

Fixes mac_to_hex function Removes unnecessary int conversion

saltiyazan requested a review from gab-arrobo September 12, 2023 13:20

gruyaume reviewed Sep 12, 2023

View reviewed changes

Addresses review comments, fixes tests

f83da1d

Removes patches of private functions Uses MAX_GATES-1 as gate index for default routes

saltiyazan force-pushed the improves-route-control-script branch from 77b3f85 to f83da1d Compare September 18, 2023 09:01

Merge branch 'master' into improves-route-control-script

fe9dcaf

gruyaume approved these changes Oct 17, 2023

View reviewed changes

gab-arrobo requested a review from gruyaume October 18, 2023 01:32

gab-arrobo reviewed Oct 18, 2023

View reviewed changes

gab-arrobo self-requested a review October 18, 2023 02:56

gab-arrobo reviewed Oct 18, 2023

View reviewed changes

gab-arrobo self-requested a review October 18, 2023 03:12

gab-arrobo reviewed Oct 18, 2023

View reviewed changes

gab-arrobo self-requested a review October 18, 2023 03:37

gab-arrobo reviewed Oct 18, 2023

View reviewed changes

Fixes gate_idx computation when adding a new neighbor

dbbc186

gab-arrobo approved these changes Oct 19, 2023

View reviewed changes

gab-arrobo merged commit 7018518 into omec-project:master Oct 19, 2023
10 checks passed

ghislainbourgeois mentioned this pull request Oct 20, 2023

Chore: Use upstream project for UPF configurations canonical/sdcore-upf-bess-rock#10

Merged

5 tasks

saltiyazan mentioned this pull request Nov 28, 2023

Replace IPDB with NDB in UPF #735

Open

gab-arrobo reviewed Feb 5, 2024

View reviewed changes

		if route_entry.prefix_len == 0 and route_entry.dest_prefix == "0.0.0.0":
		gate_idx = self.MAX_GATES - 1


		DESTINATION_IP = "NDA_DST"

Improves route control script #710

Improves route control script #710

Conversation

saltiyazan commented Aug 2, 2023 • edited

omecproject commented Aug 2, 2023

gruyaume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghislainbourgeois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-arrobo commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-arrobo commented Sep 12, 2023

gruyaume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-arrobo commented Sep 19, 2023

saltiyazan commented Sep 20, 2023

gab-arrobo commented Sep 26, 2023

gab-arrobo left a comment

Choose a reason for hiding this comment

gab-arrobo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-arrobo Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saltiyazan commented Aug 2, 2023 •

edited

gab-arrobo Oct 18, 2023 •

edited

gab-arrobo commented Oct 19, 2023 •

edited