[DPE-6101] Add first batch of the charm #3

phvalguima · 2024-12-19T16:08:59Z

Adds the first batch of the charm. This PR should be considered once the py wrapper and README have been merged.

It includes:

the benchmark lib: for now, it is added as a folder within src: src/benchmark
the charm itself: is composed of any of the src/*.py files

Once we have merged all the changes for this charm, we can break the lib into a separate repo. That would mean essentially src/benchmark/* + templates/ would go away to a different repository, alongside its corresponding tests.

deusebio

The code is quite well organized and structured. I have really appreciated in general that there are a number of re-usable concepts and code in the benchmark submodule

(issue) I only have a couple of general issues I'd like to raise sooner rather than later as they may require a bit of restructure and re-organization, before doing a more detailed review. In general I would nudge here to the general structure/pattern a bit more. Some points (in order of importance):

We break the clean dependency structure core > managers > handlers in a number of places
I'd like to understand better the Charm inheritance pattern and what are the driving reasons for it. It seems to me that we could just use the usual handlers but I may be missing some points/reasons
I believe it would be also good to split core, managers and handlers in the custom code too. Right now we are doing this for the benchmark module, but I'd like to do this also for the Kafka specifics bits

src/charm.py

src/benchmark/base_charm.py

src/charm.py

src/benchmark/managers/config.py

src/benchmark/managers/lifecycle.py

src/charm.py

This reverts commit 032caa8.

This reverts commit 1474fba.

marcoppenheimer

OK, done a first pass.
I think it's probably too big a beast to be more detailed right now. I think what we can do is focus on addressing the current comments now, and 'merging' this as is.
Then, I'll set some time up for me to go through the code line by line, open a PR with thoughts in the diff and we can discuss the rest there, what do you think?

actions.yaml

charmcraft.yaml

marcoppenheimer · 2025-01-08T17:06:49Z

charmcraft.yaml

+    source: .
+    override-build: |
+      # Ship the charm contents
+      curl -sSL https://code.launchpad.net/~pguimaraes/openmessaging-benchmark/+git/openmessaging-benchmark/+artifact/280249/+files/openmessaging-benchmark-0.0.1-ubuntu0-20241119152607-linux-x64.tar.gz | tar -zxvf -


question: What's the plan for long-term housing of this?

My idea is to keep it as a launchpad project, just like other of our build from source. @deusebio wdyt?

marcoppenheimer · 2025-01-08T17:09:27Z

config.yaml

+  test_name:
+    default: ""
+    type: string
+    description: |
+      Used to identify the test. MUST NOT be empty.


todo: If this needs to be enforced, have a look at what we do with StructuredConfig on the Kafka charm for config validation.

Pydantic model for config validation

Usage in the charm

Upon further review of this file, you should definitely implement validators for lots of these config options imo.

@marcoppenheimer can we break this into a next PR? There are other changes that will be needed, e.g. add a lib/ folder and its content.

Can we break this into a follow-up PR? Added as a TODO here: #7

src/benchmark/base_charm.py

marcoppenheimer · 2025-01-08T19:01:54Z

src/benchmark/core/models.py

+    def get(self) -> DPBenchmarkBaseDatabaseModel | None:
+        """Returns the value of the key."""
+        if not self.relation or not (endpoints := self.remote_data.get("endpoints")):
+            return None
+
+        unix_socket = None
+        if endpoints.startswith("file://"):
+            unix_socket = endpoints[7:]
+        try:
+            return DPBenchmarkBaseDatabaseModel(
+                hosts=endpoints.split(),
+                unix_socket=unix_socket,
+                username=self.data.get("username"),
+                password=self.data.get("password"),
+                db_name=self.remote_data.get(self.database_key),
+                tls=self.tls,
+                tls_ca=self.tls_ca,
+            )
+        except error_wrappers.ValidationError as e:
+            logger.warning(f"Failed to validate the database model: {e}")
+            entries = [entry.get("loc")[0] for entry in e.errors()]
+            raise DPBenchmarkMissingOptionsError(f"{entries}")


question: I was initially confused about the get here, and it got me thinking.
Given that this is so coupled with DPBenchMarkBaseDatabaseModel, would it make more sense to remove the Pydantic model entirely, and just have those attributes part of this object here?

The reason I mentioned it:

db_state = DatabaseState(*args, **kwargs) # strange access pattern username = db_state.get().username # seems easier username = db_state.username

We can still raise on access if necessary, or just return a Falsey/None.

So, here is my take:

You can still do: db_state = DatabaseState(*args, **kwargs), but you have to take into account the ValidationError, as expected

The get() adds a new feature: it converts this exception into sth more "acceptable" for the reminder of the code

The username = db_state.username would mean a bunch of properties and setters. I feel like some 20 lines of code will end up into some 60

I am a bit uncomfortable removing this logic from a pydantic model.

Let's keep it for now. I think that the ValidationError should be raised on CharmBase.__init__ as part of the structured_config mentioned in a previous comment.
AKA - if the config is borked, the charm just fails. I don't think we should be validating these things during event execution, so we wouldn't need this strange access pattern.

As you mentioned, we can look at structured_config in another PR, but when we do, come back to here and see if it still makes sense?

src/benchmark/core/pebble_workload_base.py

marcoppenheimer · 2025-01-08T19:06:42Z

src/benchmark/core/pebble_workload_base.py

+    ) -> str | None:
+        """Executes a command on the workload substrate.
+
+        Returns None if the command failed to be executed.


todo: I think returning None if it didn't execute is wrong. Commands with no stdout would return "", which is also falsey, meaning that we couldn't pick up on failures if we do something like workload.exec(["ls", ">", "/dev/null", "2>&1"])

Not sure I follow... What matters here is the returncode as the output is actually sent to log files.

src/benchmark/managers/lifecycle.py

deusebio

Had a look. My main concern with the unclear dependency tree is resolved, so I'm happy to approve. I provide still a couple of suggestions, but these are not super critical items, and they can also followed up in separate PRs, as you prefer. If these are things we agree to follow up (but outside of the scope of this PR), please place an improvement item in Jira and refer to the Jira ticket in the comment and/or in the code (such that it does not fall through the cracks).

deusebio · 2025-01-13T08:46:46Z

src/benchmark/managers/config.py

+            parallel_processes=self.config.get("parallel_processes"),
+            threads=self.config.get("threads"),
+            duration=self.config.get("duration"),
+            run_count=self.config.get("run_count"),


suggestion I would rather use some more structured representation instead of dict. The parsing of these should not be done within a manager, but we should rather use dataclass/pydantic models. For configs, in BigData we use the StructuredConfig pattern.

It can also be addressed in a separate PR though.

deusebio · 2025-01-13T08:48:42Z

src/charm.py

+  max.partition.fetch.bytes=10485760
+"""
+
+KAFKA_WORKLOAD_PARAMS_TEMPLATE = """name: {{ partitionsPerTopic }} producer / {{ partitionsPerTopic }} consumers on 1 topic


suggestions, minor maybe we could use proper template, like YAML static files in a resouce folder, such that these are even a bit more readable, rather than embedded in the code as strings.

marcoppenheimer

Looks good! As long as the outstanding comments are addressed in the next PR, I'm really happy with how this is looking 👍🏾

Add first batch of the charm

e7ba862

phvalguima requested review from deusebio, imanenami and marcoppenheimer December 19, 2024 16:09

deusebio reviewed Dec 20, 2024

View reviewed changes

phvalguima added 4 commits January 7, 2025 12:16

Add TLS support

1474fba

Add export as a plugin for poetry v2

032caa8

Revert "Add export as a plugin for poetry v2"

6b69795

This reverts commit 032caa8.

Revert "Add TLS support"

e6ef3c8

This reverts commit 1474fba.

marcoppenheimer reviewed Jan 8, 2025

View reviewed changes

phvalguima added 9 commits January 10, 2025 16:06

Merge remote-tracking branch 'origin' into DPE-6101-add-charm

9a06f93

Update poetry

56e5aac

Add external plugin to poetry

c165009

Fixes unit test and removed unused workload_lifecycle.py

8cbed9e

Remove events from manager classes

a5ee472

Fix the pyright errors

5af6486

Add Poetry dependencies support

d1d7062

Remove true returns in pebble workload

ad57e37

Add match / case instead of multiple if's

971c92f

phvalguima mentioned this pull request Jan 12, 2025

[DPE-6296] Pyright fixes + structured_config additions + break down of actions.py #7

Merged

phvalguima requested review from deusebio and marcoppenheimer January 13, 2025 06:33

deusebio approved these changes Jan 13, 2025

View reviewed changes

marcoppenheimer approved these changes Jan 13, 2025

View reviewed changes

phvalguima merged commit 030235e into main Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-6101] Add first batch of the charm #3

[DPE-6101] Add first batch of the charm #3

phvalguima commented Dec 19, 2024 •

edited

Loading

deusebio left a comment

marcoppenheimer left a comment

marcoppenheimer Jan 8, 2025

phvalguima Jan 10, 2025

marcoppenheimer Jan 8, 2025

marcoppenheimer Jan 8, 2025

phvalguima Jan 12, 2025

phvalguima Jan 12, 2025

marcoppenheimer Jan 8, 2025

marcoppenheimer Jan 8, 2025

phvalguima Jan 12, 2025

marcoppenheimer Jan 13, 2025

marcoppenheimer Jan 8, 2025

phvalguima Jan 12, 2025

deusebio left a comment

deusebio Jan 13, 2025

deusebio Jan 13, 2025

marcoppenheimer left a comment

[DPE-6101] Add first batch of the charm #3

[DPE-6101] Add first batch of the charm #3

Conversation

phvalguima commented Dec 19, 2024 • edited Loading

deusebio left a comment

Choose a reason for hiding this comment

marcoppenheimer 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

deusebio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoppenheimer left a comment

Choose a reason for hiding this comment

phvalguima commented Dec 19, 2024 •

edited

Loading