Enhance charm's life-cycle #118

chanchiwai-ray · 2023-11-27T04:14:54Z

No description provided.

for the exporter service.

linting. Co-authored-by: jneo8 <[email protected]>

hw_resources.py

chanchiwai-ray · 2023-11-27T04:15:38Z

Unit test / functional test will be added if the approach is approved

jneo8

Some question need more information and discussion.

jneo8 · 2023-11-27T09:21:44Z

src/charm.py

+            resource_white_list,
+            resource_black_list=self._stored.resource_install_status,  # type: ignore[arg-type]
+        )
+        self._stored.resource_install_status = resource_install_status


status make my confuse with MaintenanceStatus

One is boolean, another is juju's unit status.

Maybe result might be a better choice -- resource_install_result

No problem, I personally will never think of status with *Status (in libjuju) 😆

jneo8 · 2023-11-27T09:37:29Z

src/exporter.py

@@ -0,0 +1,137 @@
+"""Exporter relation observer."""


I do like the idea we try to let exporter service follow the charm's life cycle. I would like to say it's a nice design. So I am +1 with this.

But in the other hand I would like to point out because we create one more layer between the service and the charm, it somehow increase the complexity we have here.

Is that possible we combine the exporter and service together? Or you think it has benefit we have this three layers design?

I agree with your point. Introducing another layer for the charm to interface with the exporter functionality might be quite confusing.

So I think it's good to think about what's the core idea behind the src/exporter.py::Observer and src/service.py::Exporter and see if there's a chance we can combine or if we really need those 2 layers.

Just to be clear, there is no "3 layers for the charm". Each subclass of ops.Object is sort of like a leaf to the ops.CharmBase, so there is only 2 layers.

If we choose to combine the Service(object) and the relation handler Observer(ops.Object) together, then the service class will be like Service(ops.Object), then it's "inheritance", and I used to do that. But there's no clear "inheritance" between a relation handler and a exporter service. So I chose composition over inheritance. However, that's only a benefit on the meaning of code.

The other benefits are:

Easier unit test: relation handler is decoupled with whatever service (systemd or snap), so the test for relation handler should be unchanged even if we later switch from a systemd to a snap.

Easier to rewrite the exporter from a systemd service to a snap, since they are decoupled.

But that's being said, the benefit is in the future, and not present. If we want to refactor it later, we can still do it later (but I doubt we will do it later)

When I say three layers it's because when we try to debug we still need to check three classes. charm -> exporter -> service. Yes, we have a composition over inheritance pattern here but it somehow make it more hard to debugging. For decoupled part because we already extract the service from charm, the service is already decoupled from charm. But do we need to decouple service from exporter life cycle now?

The different of one service vs service + exporter is when we try to switch from systemd to snap:

service: Re-implement the service.

service + exporter: Re-implement the service, but can we ignore the life cycle define in exporter in reality?

Also when the time coming, do we need to keep the systemd service class?
If you think the both answers are yes, then we can keep this design else I suggest to have merge them use mixin.

jneo8 · 2023-11-27T09:41:35Z

src/charm.py

+        if not all(resource_install_status.values()):
+            failed_resources = [r for r, s in resource_install_status.items() if not s]
+            msg = f"Failed to install resources: {', '.join(failed_resources)}"
+            logger.error(msg)
            self._stored.blocked_msg = msg


blocked_msg seems not useful anymore.

jneo8 · 2023-11-27T09:42:59Z

src/charm.py

        self.hw_tool_helper.remove(self.model.resources)
-        self.exporter.uninstall()
+        self._stored.agent_installed = False


Instead of having manual config here. Maybe a functional to set default value?

jneo8 · 2023-11-27T09:43:24Z

src/charm.py

+                self.model.unit.status = ErrorStatus(msg)
+                return
+
+        self.update_status()

    def _on_remove(self, _: EventBase) -> None:


What happen if remove fail?

Currently, there's not remove fail logic in the code

So it just get into error state?

jneo8 · 2023-11-27T09:45:45Z

src/charm.py

-            ],
-        )
-        self.exporter = Exporter(self.charm_dir)
+        self.exporter_observer = exporter.Observer(self)


self.exporter_observer sounds not precise. Just self.exporter?

Also, I want to think about what we're "observing" here. exporter.Observer feels like I'm initializing a class that observes the exporter but it looks more like the class is observing the cos-agent relation?

jneo8 · 2023-11-27T09:48:39Z

src/charm.py

+            success, message = self.exporter_observer.validate_agent_configs(options)
+            if not success:
+                self.model.unit.status = BlockedStatus(message)
+                event.defer()


If the config is not valid. User need to re-config it, right?
Then why we need this event.defer()

jneo8 · 2023-11-27T09:56:56Z

src/hw_resources.py

+    return True
+
+
+def validate_checksum(support_version_infos: t.List[ToolVersionInfo], path: Path) -> bool:


It's very weird that I have a checksum module and the checksum function is here. I suggest to move it back to checksum module.

You could argue that checksum.py contains all the checksums for the various resource tools but doesn't need to contain the actual validation logic which could happen elsewhere.

checksum.py is only the definition of the checksum similar to config.py. The main purpose of validating checksum is for the resource, so I put them together.

we can maybe move checksum.py to config.py, and rename config.py to constants.py

The main purpose of validating checksum is for the resource, so I put them together.

I agree with this approach. We could possibly rename checksum.py to maybe tool_info.py since it contains more than the checksums for each of the tools. The checksum validation functionality could go into the ResourceHelper class I detailed in my other comment.

I will still choose to have all the part relate to checksum in this module. I don't want to switch between charm to verify the logic of checksum checks.

constants.py is somehow bad because we are not just putting constants in the config. All the related config or loading functions will be here also. No necessary to split it.

One more thing to add is we would like to have someone easily contribute to the checksum. So intuitively checksum.py is the best place.

Sorry, still -1 for this change.

jneo8 · 2023-11-27T09:59:27Z

src/hw_resources.py

@@ -0,0 +1,101 @@
+"""Wrapper for `model.Resources`."""


The definition of hardware, hw_tool, and hw_resources modules is somehow overlapping. I suggest to merge hardware and hw_resource together.

there are indeed 2 different distinct items + 1 helper: hardware (for hardware verifier), resources (for fetching user uploaded resource) + hw_tool

Here are my thoughts:

current

hw_resources.py

contains exception classes for resource check fails

utils related to resource file handling eg: check_file_exists, validate_size etc.

hardware.py

constant containing supported raid storages

utils for general hw checks. eg: lshw, get_bmc_address

utils for hw verification, {raid,bmc}_hw_verifier}

hw_tools.py

utils related to package management, eg: install_deb, remove_deb, check_deb_pkg_installed

strategy classes for various tools

HwToolHelper class

what i'm proposing

utils.py/ helpers.py

This will be a general class with util methods contained in these helper classes similar to HwToolHelper

ResourceHelper: This contains mostly the helper methods from hw_resources.py -- check_file_exists, validate_size etc.

PackageHelper -- will contain all the apt and deb related functions -- install_deb, remove_deb, check_deb_pkg_installed, install_apt_package

HwHelper: contains helper methods for hardware related tasks. --lshw, get_bmc_address, validate_redfish_credentials

HwVerifier: contains the "verifier" methods from hardware.py and generate the whitelist -- {raid,bmc}_hw_verifier}

Also exception classes for resource checks.

hw_tools.py

This will now only have the strategy classes + HwToolHelper class

The constant containing supported raid storages from hardware.py should go into config.py/constants.py

I suggest just keep it simple:

hw_helpers.py for all the operation not relate to resource directly, including check file size or build soft-link.

hw_tools.py for all the operation relate to tools.

dashmage

Apologies in advance for the large number of comments. Overall, I think these are really good changes for hardware-observer and appreciate the time taken to plan and code this out 👍 Lot of my suggestions are just typos or re-wording to comments and doc-strings :)

dashmage · 2023-11-27T11:12:22Z

src/charm.py

-            ],
-        )
-        self.exporter = Exporter(self.charm_dir)
+        self.exporter_observer = exporter.Observer(self)


Also, I want to think about what we're "observing" here. exporter.Observer feels like I'm initializing a class that observes the exporter but it looks more like the class is observing the cos-agent relation?

dashmage · 2023-11-27T11:16:05Z

src/exporter.py

@@ -0,0 +1,137 @@
+"""Exporter relation observer."""


I agree with your point. Introducing another layer for the charm to interface with the exporter functionality might be quite confusing.

So I think it's good to think about what's the core idea behind the src/exporter.py::Observer and src/service.py::Exporter and see if there's a chance we can combine or if we really need those 2 layers.

dashmage · 2023-11-27T11:17:11Z

src/exporter.py

+        """Initialize the class."""
+        super().__init__(charm, EXPORTER_RELATION_NAME)
+        self.charm = charm
+        self.agent = Exporter(charm.charm_dir)


IMO, we should choose a different name than agent for our hw exporter.

dashmage · 2023-11-27T11:17:46Z

src/exporter.py

+
+    @property
+    def agent_enabled(self) -> bool:
+        """Return True if cos-agent relation is not zero."""


nit: Might be nicer as Return True if cos-agent relation is present

dashmage · 2023-11-27T11:19:13Z

src/charm.py

+            resource_white_list,
+            resource_black_list=self._stored.resource_install_status,  # type: ignore[arg-type]
+        )
+        self._stored.resource_install_status = resource_install_status


Maybe result might be a better choice -- resource_install_result

dashmage · 2023-11-27T12:12:34Z

src/hw_resources.py

+    return True
+
+
+def validate_checksum(support_version_infos: t.List[ToolVersionInfo], path: Path) -> bool:


You could argue that checksum.py contains all the checksums for the various resource tools but doesn't need to contain the actual validation logic which could happen elsewhere.

dashmage · 2023-11-27T12:15:04Z

src/service.py

-            REDFISH_HOST=redfish_creds.get("host", ""),
-            REDFISH_USERNAME=redfish_creds.get("username", ""),
-            REDFISH_PASSWORD=redfish_creds.get("password", ""),
+            REDFISH_OPTIONS=redfish_options,


Collecting all the redfish options into one structure is a really good idea 👍

dashmage · 2023-11-27T12:19:31Z

src/service.py


 logger = getLogger(__name__)


+NUM_RETRIES = 3


We could possible provide some more context to these constants. Maybe:

EXPORTER_HEALTH_RETRY_COUNT = 3 EXPORTER_HEALTH_RETRY_TIMEOUT = 3

It's within the context of service.py

dashmage · 2023-11-27T12:22:31Z

src/service.py

+        """Check the health of the exporter daemon and try to recover it if needed.
+
+        This function perform health check on exporter daemon if the exporter
+        is already installed. If it is somehow stopped, we should try to


In what situation would the exporter have been stopped (and not crashed) and require a restart logic to fix it?

maybe stopped by the operator?

Hmm, I wouldn't imagine the operator directly intervening with the lifecycle of the exporter service in the ideal case. Because then the operator would also know to start it back up again. But I can understand than things aren't ideal always and maybe we do need to have restarts to bring up the exporter. But this feels quite "hacky" for some reason.

dashmage · 2023-11-27T12:24:02Z

templates/hardware-exporter-config.yaml.j2

-redfish_host: "{{ REDFISH_HOST }}"
-redfish_username: "{{ REDFISH_USERNAME }}"
-redfish_password: "{{ REDFISH_PASSWORD }}"
+{% if REDFISH_OPTIONS.get('enable', False)  %}


Should have a hyphen (-) before the closing %

Probably not, the indentation will be wrong:

port: 1000 level: debug enable_collectors: - 1 - 2 - 3 redfish_host: "" redfish_username: "" redfish_password: ""

Oh I meant like this : {% if REDFISH_OPTIONS.get('enable', False) -%} where it ends with -%} similar to the other if statements.

Yep, it's the same as above. Somehow it thinks the redfish_host: "" part of enable_collectors

jneo8

Add review for hw_tools.py

jneo8 · 2023-11-28T02:36:32Z

src/hw_tools.py

+    package_source_mapping = t.Dict[str, str]
+
+    @abstractmethod
+    def add_repos(self) -> None:


We maybe don't need this if repo is not required. I suggest to use Mixin instead of abstractmethod method.

jneo8 · 2023-11-28T02:39:35Z

src/hw_tools.py

+        """Check if the tool is valid or not."""
+
+
+class APTStrategy(APTStrategyABC):


I am -1 for this.
This make inheritance has too many levels. Suggest to re-design this part.

Then I would suggest remove APTStrategyABC and TRPStrategyABC since they are not useful abstraction. APTStrategy and TRPStrategy are enough

jneo8 · 2023-11-28T02:40:56Z

src/hw_tools.py

+            success &= check_deb_pkg_installed(package)
+        return success
+
+    def validate_tool(self) -> bool:


which {package} ?

Added some logs, but we will not be using it, since in the end the error will be a strategy failure

jneo8 · 2023-11-28T02:43:52Z

src/hw_tools.py

-    """Third party resource strategy class."""
+    """Third party resource strategy abstract class."""
+
+    origin_path: Path


I am -1 for this. We can't suppose that every third party tool follow this pattern to control it's resource. This maybe can't fit in some special case.

Put implement details in child class has some reason.

jneo8 · 2023-11-28T02:45:11Z

src/hw_tools.py

-    origin_path = Path("/opt/MegaRAID/storcli/storcli64")
-    symlink_bin = TOOLS_DIR / HWTool.STORCLI.value
+
+class TPRStrategy(TPRStrategyABC):


-1 here.
Same reason for too many levels of inheritance here.

Same here, then I would suggest remove APTStrategyABC and TRPStrategyABC since they are not useful abstraction. APTStrategy and TRPStrategy are enough

jneo8 · 2023-11-28T02:53:34Z

src/hw_tools.py

+                collector = EXPORTER_COLLECTOR_MAPPING.get(tool)
+                if collector is not None:
+                    collectors += collector
+            self._hw_collector_white_list = collectors


I would not choose to store some status in this class. The mapping logic actually is just mapping from HWTool to another string. It's not expensive.

yep, hw_tool_white_list only this is expensive, updated

jneo8 · 2023-11-28T02:55:01Z

src/hw_tools.py

+    @property
+    def hw_tool_white_list(self) -> t.List[HWTool]:
+        """Define hardware tool white list."""
+        # cache the white list because it might be expensive to get


This is not a good practice for caching to store some information in the class. Every time you re-init this class the cache is gone.

Instead of cache instead class. A ttl cache or a decorator to decide when to use refresh maybe more make sense?

More to add is making class become no-status can ignore a lot of hidden bugs.

used cached_property

… from cos_agent.py

…y, and put it into ...". In particular, the refactoring in strategy classes are removed, and only added `check_status` and `validate_tool` methods. This partially reverts commit fbbd346.

installation status of the tools.

dashmage

Left some more comments. Overall, I think we could use some nice refactoring with the hardware related modules. hw_tool.py looks good adding the validate and install checks directly in the parent strategy classes 👍

dashmage · 2023-11-28T12:30:43Z

templates/hardware-exporter-config.yaml.j2

-redfish_host: "{{ REDFISH_HOST }}"
-redfish_username: "{{ REDFISH_USERNAME }}"
-redfish_password: "{{ REDFISH_PASSWORD }}"
+{% if REDFISH_OPTIONS.get('enable', False)  %}


Oh I meant like this : {% if REDFISH_OPTIONS.get('enable', False) -%} where it ends with -%} similar to the other if statements.

dashmage · 2023-11-28T13:10:15Z

src/hw_resources.py

@@ -0,0 +1,101 @@
+"""Wrapper for `model.Resources`."""


Here are my thoughts:

current

hw_resources.py

contains exception classes for resource check fails

utils related to resource file handling eg: check_file_exists, validate_size etc.

hardware.py

constant containing supported raid storages

utils for general hw checks. eg: lshw, get_bmc_address

utils for hw verification, {raid,bmc}_hw_verifier}

hw_tools.py

utils related to package management, eg: install_deb, remove_deb, check_deb_pkg_installed

strategy classes for various tools

HwToolHelper class

what i'm proposing

utils.py/ helpers.py

This will be a general class with util methods contained in these helper classes similar to HwToolHelper

ResourceHelper: This contains mostly the helper methods from hw_resources.py -- check_file_exists, validate_size etc.

PackageHelper -- will contain all the apt and deb related functions -- install_deb, remove_deb, check_deb_pkg_installed, install_apt_package

HwHelper: contains helper methods for hardware related tasks. --lshw, get_bmc_address, validate_redfish_credentials

HwVerifier: contains the "verifier" methods from hardware.py and generate the whitelist -- {raid,bmc}_hw_verifier}

Also exception classes for resource checks.

hw_tools.py

This will now only have the strategy classes + HwToolHelper class

The constant containing supported raid storages from hardware.py should go into config.py/constants.py

dashmage · 2023-11-28T13:14:44Z

src/hw_resources.py

+    return True
+
+
+def validate_checksum(support_version_infos: t.List[ToolVersionInfo], path: Path) -> bool:


The main purpose of validating checksum is for the resource, so I put them together.

I agree with this approach. We could possibly rename checksum.py to maybe tool_info.py since it contains more than the checksums for each of the tools. The checksum validation functionality could go into the ResourceHelper class I detailed in my other comment.

dashmage · 2023-11-28T13:18:21Z

src/service.py

+        """Check the health of the exporter daemon and try to recover it if needed.
+
+        This function perform health check on exporter daemon if the exporter
+        is already installed. If it is somehow stopped, we should try to


Hmm, I wouldn't imagine the operator directly intervening with the lifecycle of the exporter service in the ideal case. Because then the operator would also know to start it back up again. But I can understand than things aren't ideal always and maybe we do need to have restarts to bring up the exporter. But this feels quite "hacky" for some reason.

dashmage · 2023-11-28T13:23:33Z

src/charm.py

        change_set = set()
        model_config: Dict[str, Optional[str]] = dict(self.model.config.items())
        for key, value in model_config.items():
            if key not in self._stored.config or self._stored.config[key] != value:  # type: ignore
                logger.info("Setting %s to: %s", key, value)
                self._stored.config[key] = value  # type: ignore
                change_set.add(key)
+        return change_set
+
+    def get_redfish_options(self) -> Dict[str, Any]:


I see, maybe we could have redfish_configs, get_redfish_configs along with get_exporter_configs? What do you think?

dashmage · 2023-11-28T13:28:20Z

src/hw_tools.py

@@ -127,6 +99,10 @@ def name(self) -> HWTool:
        """Name."""
        return self._name

+    @abstractmethod
+    def check_status(self) -> t.Dict[str, bool]:


check_status sounds quite ambiguous. Could we rename this like check_installed or install_status to make it more clear that this method lets you know if the tool was installed properly or not.

chanchiwai-ray · 2023-11-29T04:06:29Z

@dashmage , thanks for providing you thoughts, I am not against your idea of re-organizing the code base, but having everything inside a helper module isn't going to make the code base easier to understand (for example, you can broadly break down the code base into charm.py and utils.py, and put everything unrelated to the charm into utils.py), especially when they have clear functionalities and non-trivial lines of code.

That's being said the purpose of this is PR is not refactoring, I am only doing the minimal refactoring that allows us to improve the charm's life cycle, makes it easier to debug by simplifying the logic in charm.py, and improve the efficiency by reducing the calls to the get_hw_white_list to only once.

Looking back now, I think I can further reduce the refactoring, and leave the code base mostly unchanged at the expense of losing those benefits.

dashmage · 2023-11-29T04:14:04Z

@dashmage , thanks for providing you thoughts, I am not against your idea of re-organizing the code base, but having everything inside a helper module isn't going to make the code base easier to understand (for example, you can broadly break down the code base into charm.py and utils.py, and put everything unrelated to the charm into utils.py), especially when they have clear functionalities and non-trivial lines of code.

That's being said the purpose of this is PR is not refactoring, I am only doing the minimal refactoring that allows us to improve the charm's life cycle, makes it easier to debug by simplifying the logic in charm.py, and improve the efficiency by reducing the calls to the get_hw_white_list to only once.

Looking back now, I think I can further reduce the refactoring, and leave the code base mostly unchanged at the expense of losing those benefits.

This makes sense, maybe we can take up the refactoring discussion + work in another PR. It's better to have a smaller, clear PR that aims to achieve one thing 👍

chanchiwai-ray · 2023-11-29T08:50:18Z

@Pjack please let me know if I should extract only "enhance charm's life-cycle" part, and leave the refactoring to someone else.

chanchiwai-ray · 2023-11-29T10:01:47Z

Closing this. Please check out #119 instead

chanchiwai-ray and others added 7 commits November 27, 2023 11:32

Decouple hardware discovery and exporter config rendering logic.

a8ccbe9

Decouple exporter service from charm.py, and create a relation observer

dc8fdbd

for the exporter service.

Cherry-pick from 7513f18, and fix

6eb3c3e

linting. Co-authored-by: jneo8 <[email protected]>

Decouple verifiers from strategies, and move verifiers to hardware.py.

ff52219

Move resource checking functions out of hw_tools.py, and put it into

fbbd346

hw_resources.py

Refactor hw_tool.py

0ea84ed

Enhance charm's life-cycle.

b6f8a78

chanchiwai-ray requested review from a team, Pjack, aieri, agileshaw, jneo8 and rgildein November 27, 2023 04:14

chanchiwai-ray marked this pull request as draft November 27, 2023 04:15

jneo8 requested changes Nov 27, 2023

View reviewed changes

dashmage suggested changes Nov 27, 2023

View reviewed changes

jneo8 requested changes Nov 28, 2023

View reviewed changes

chanchiwai-ray added 6 commits November 28, 2023 14:11

Rename exporter.py to cos_agent.py and decouple exporter service from…

fc4a713

… from cos_agent.py

Partially reverts "Move resource checking functions out of hw_tools.p…

cb04478

…y, and put it into ...". In particular, the refactoring in strategy classes are removed, and only added `check_status` and `validate_tool` methods. This partially reverts commit fbbd346.

Some renamings in charm.py and hw_tools.py, also add logic to check the

815b481

installation status of the tools.

Remove event.defer in config change event.

a398f2f

Fix typos

afd0028

Fix nitpicks.

4089a04

chanchiwai-ray requested review from jneo8 and dashmage November 28, 2023 07:31

dashmage suggested changes Nov 28, 2023

View reviewed changes

chanchiwai-ray marked this pull request as ready for review November 29, 2023 08:49

chanchiwai-ray closed this Nov 29, 2023

chanchiwai-ray deleted the dev branch November 29, 2023 10:33

		return True


		def validate_checksum(support_version_infos: t.List[ToolVersionInfo], path: Path) -> bool:

		"""Check if the tool is valid or not."""


		class APTStrategy(APTStrategyABC):

Enhance charm's life-cycle #118

Enhance charm's life-cycle #118

Conversation

chanchiwai-ray commented Nov 27, 2023

chanchiwai-ray commented Nov 27, 2023

jneo8 left a comment

Choose a reason for hiding this comment

jneo8 Nov 27, 2023 • edited Loading

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

chanchiwai-ray Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

jneo8 Nov 29, 2023 • edited Loading

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

chanchiwai-ray Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

dashmage Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

current

hw_resources.py

hardware.py

hw_tools.py

what i'm proposing

utils.py/ helpers.py

hw_tools.py

jneo8 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

dashmage 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

jneo8 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

jneo8 Nov 27, 2023 •

edited

Loading

chanchiwai-ray Nov 28, 2023 •

edited

Loading

jneo8 Nov 29, 2023 •

edited

Loading

chanchiwai-ray Nov 28, 2023 •

edited

Loading

dashmage Nov 28, 2023 •

edited

Loading

jneo8 Nov 29, 2023 •

edited

Loading

dashmage Nov 28, 2023 •

edited

Loading