majduk/charm-k8s-mongodb

Kubernetes Charm/Operator for MongoDB

charm-k8s-mongodb's Issues

Image No-HA.png seems improperly labeled

In https://github.com/majduk/charm-k8s-mongodb/blob/master/img/No-HA.png
The "mongodb-k8s" object points at the "mongodb-k8s-operator" pod, but the "mongodb-k8s-operator" points at the "mongodb-k8s" pod.

given that in:
https://github.com/majduk/charm-k8s-mongodb/blob/master/img/HA.png

those are reversed, I think the former was just a typo.

Should use charmcraft to build the charm

This way you could stop fixing the Python path to include lib in src/charm.py, have dependencies in a requirements.txt file, etc...

The config.py file should not be executable

But it is:

$ ll config.yaml
-rwxrwxr-x 1 facundo facundo 765 jun 23 14:16 config.yaml

Scale down does not work

The charm needs to properly handle scale down scenario.

Please add docstring to the classes and methods in charm.py and other files under `src`

Add k8s library

https://github.com/chipaca/ops-lib-k8s/
k8s = ops.lib.use("k8s", 0, "mailto:[email protected]")

Please use "store", not "state", for StoredState

We're trying to get away from calling it "state" as it doesn't store the state of the charm.

Use multiple naming in for loops

This way, in src/charm.py line 61, instead of

        for delegator in delegators:
            self.framework.observe(delegator[0], delegator[1])

...you can do...

        for event, method in delegators:
            self.framework.observe(event, method)

Reconsider the "Observers" infrastructure

It looks there are several observers that inherit a base class... that does nothing except hold the attributes. It doesn't make sense to have all this infrastructure and restrictions only to hold the attributes... which forces you to pass them, not really other benefit.

So, you could go from having this in MongoDbCharm...

    def on_update_status_delegator(self, event):
        logger.info('on_update_status_delegator({})'.format(event))
        return StatusObserver(
            self._framework_wrapper,
            self._resources,
            self._pod,
            self._mongo_builder).handle(event)

...and this in observers.py...

class StatusObserver(BaseObserver):

    def handle(self, event):
        if self._pod.is_ready:
            logger.info('Pod is ready')
            self._framework.unit_status_set(ActiveStatus())
            return
        self._framework.unit_status_set(BlockedStatus('Pod is not ready'))
        logger.info('Pod is not ready')

...to only this in MongoDbCharm:

    def on_update_status_delegator(self, event):
        logger.info('on_update_status_delegator({})'.format(event))
        if self._pod.is_ready:
            logger.info('Pod is ready')
            self._framework.unit_status_set(ActiveStatus())
            return
        self._framework.unit_status_set(BlockedStatus('Pod is not ready'))
        logger.info('Pod is not ready')

It's not shorter and simpler, its also easier to spot little issues, like, why are you passing self._mongo_builder to StatusObserver?

Let the logging module to build the messages

Example, in charm.py, convert...

        logger.info('on_remove_pvc_action_delegator({})'.format(event))

...to...

        logger.info('on_remove_pvc_action_delegator(%s)', event)

This is for performance and robustness.

For performance because if the logger decides that the message will not be sent, the string is not built at all.

For robustness because in the event of introducing a bug in the line, the logging infrastructure will not crash, but report the problem. For example, in this case: logger.info("Foo %s: %d", self.quantity).

Reconsider the "FrameworkWrapper" infrastructure

It's a layer above many things. A quite lightweight one, though, so there's not a lot of work hidden in that layer, but as it's a layer above several other things, to where each property belongs is not easily discoverable.

Most of the FrameworkWrapper attributes are from the Model, so they can always be accessed doing self.framework.model in the charm. But note that the charm already has shortcuts like app or unit (check the docs here).

So, for example you're currently doing...

        self._pod = K8sPod(self._framework_wrapper.app_name)

...and it's shorter and more accurate to do...

        self._pod = K8sPod(self.app.name)

The exceptional attribute is goal_state_units, which is probably better to be a helper function in the charm itself.

tox flake8 unhappy

./src/k8s.py:65:80: E501 line too long (91 > 79 characters)
./test/oci_image_resource_test.py:54:0: F541 f-string is missing placeholders

This at least gets me past the lint issues:

diff --git a/src/k8s.py b/src/k8s.py
index 3776238..3be61ef 100644
--- a/src/k8s.py
+++ b/src/k8s.py
@@ -62,7 +62,8 @@ class K8sPod:

     def map_unit_to_pvc(self):
         if self.is_running:
-            return self._status['spec']['volumes'][0]['persistentVolumeClaim']['claimName']
+            volume = self._status['spec']['volumes'][0]
+            return volume['persistentVolumeClaim']['claimName']
         return None

     @property
diff --git a/test/oci_image_resource_test.py b/test/oci_image_resource_test.py
index 90edca4..78b0495 100644
--- a/test/oci_image_resource_test.py
+++ b/test/oci_image_resource_test.py
@@ -49,7 +49,7 @@ class OCIImageResourceTest(unittest.TestCase):

         mock_path_obj = MagicMock(Path)
         mock_path_obj.exists.return_value = True
-        mock_path_obj.read_text.return_value = f"""
+        mock_path_obj.read_text.return_value = """
         \0GIBRISH
         """

charm_test.py: 77 no attribute 'remove_pvc_action'

https://github.com/majduk/charm-k8s-mongodb/blob/master/test/charm_test.py#L71

test/charm_test.py:77:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <charm.MongoDbCharm object at 0x7f93a283bb10>, args = (<ops.framework.Framework object at 0x7f93a284ef10>, None)

    def __init__(self, *args):
        super().__init__(*args)
        self._framework_wrapper = FrameworkWrapper(self.framework, self._state)
        self._resources = {
            'mongodb-image': OCIImageResource('mongodb-image')
        }

        self._mongo_builder = MongoBuilder(
            self._framework_wrapper.app_name,
            self._framework_wrapper.config,
            self._resources,
            self._framework_wrapper.goal_state_units
        )

        if self._framework_wrapper.config['enable-sidecar']:
            self._resources['mongodb-sidecar-image'] = OCIImageResource(
                'mongodb-sidecar-image')

        self._pod = K8sPod(self._framework_wrapper.app_name)
        self._pvc = K8sPvc(self._framework_wrapper.app_name)
        self._mongodb = MongoDbServer(self, "mongo")

        self._k8s_builder = K8sBuilder(self._pvc)

        delegators = [
            (self.on.start, self.on_config_changed_delegator),
            (self.on.upgrade_charm, self.on_config_changed_delegator),
            (self.on.config_changed, self.on_config_changed_delegator),
            (self.on.update_status, self.on_update_status_delegator),
            (self._mongodb.on.new_client, self.on_new_client_delegator),
>           (self.on.remove_pvc_action, self.on_remove_pvc_action_delegator),
        ]
E       AttributeError: 'CharmEvents' object has no attribute 'remove_pvc_action'

src/charm.py:59: AttributeError

The issue is that remove_pvc_action is something that is dynamically added to the events object based on reading actions.yaml.

In the test Harness() you can pass in an actions.yaml or it will find an actions.yaml in the parent directory of the Charm class.

From what I can tell, this is creating its own Framework with its own CharmMeta here:

charm-k8s-mongodb/test/charm_test.py

Lines 55 to 60 in 09094b5

	raw_meta = {
	'provides': {'mongo': {"interface": "mongodb"}}
	}
	framework = Framework(self.tmpdir / "framework.data.{}"
	.format(str(uuid4)),
	self.tmpdir, CharmMeta(raw=raw_meta), model)

However, while it is providing content for metadata.yaml it isn't providing content for actions.yaml.

I'd recommend using CharmMeta.from_yaml() which is a slightly cleaner way of initializing a CharmMeta object. Eg:

meta = CharmMeta.from_yaml(metadata='''
name: mongodb
provides:
  mongo:
    interface: mongodb
''', actions='''
remove-pvc:
''')

You can also pass in file-like objects / read actions.yaml yourself.