k8s_event module #22

VenkatRamaraju · 2021-09-13T18:03:40Z

k8s_events.py: Allows users to more easily emit events for their managed objects.

Moved the find_resource and get_api_client functions from k8s_status.py to api_utils.py so that both k8s_events.py and k8s_status.py can leverage those functions without redundancy.

fabianvf · 2021-09-16T18:17:41Z

plugins/modules/k8s_event.py

+        return argumentSpec
+
+    def execute_module(self):
+        self.client = DynamicClient(kubernetes.config.new_client_from_config())


This is not going to cover enough cases, see the client construction in https://github.com/operator-framework/operator-sdk-ansible-util/blob/master/plugins/modules/k8s_status.py#L281-L342

Probably the best thing to do would be to move that function from k8s_status to a new file under module_utils, and then import it from both (you could do the same with find_resource)

Still need to remove this line

forgot to remove this line. Just did.

fabianvf · 2021-09-16T18:18:50Z

plugins/modules/k8s_event.py

+
+        # Add more fields
+        if self.params['setTimestamp']:
+            self.params["name"] = self.params["name"] + "-" + str(now)


I think they usually append it with a . rather than a -

fabianvf · 2021-09-16T18:19:56Z

plugins/modules/k8s_event.py

+  involvedObject:
+    type: dict
+    description: The object that this event is about.
+  setTimestamp:


appendTimestamp might be a more fitting name

fabianvf · 2021-09-16T18:20:39Z

plugins/modules/k8s_event.py

+  setTimestamp:
+    type: bool
+    description: Event name should have timestamp appended to it
+  api_key:


I think all these are actually documented in the auth doc fragment, see how the k8s_status module uses them (https://github.com/operator-framework/operator-sdk-ansible-util/blob/master/plugins/modules/k8s_status.py#L29-L31)

fabianvf · 2021-09-16T18:23:49Z

plugins/modules/k8s_event.py

+            "type": self.params.get("type"),
+        }
+
+        self.exit_json(**event)


I think you're returning without actually sending the event object to the API

fabianvf · 2021-09-17T16:42:49Z

plugins/module_utils/__init__.py

@@ -1,0 +1,88 @@
+import kubernetes


I'd put these into a more descriptively named file, even if it's just api_utils.py or something

fabianvf · 2021-09-17T16:43:38Z

plugins/modules/k8s_event.py

+
+extends_documentation_fragment:
+    - operator_sdk.util.osdk_auth_options
+    - operator_sdk.util.osdk_name_options


I don't think you want the name fragment included, that would add api_version and kind and name and namespace are double documented

fabianvf · 2021-09-17T16:44:46Z

plugins/modules/k8s_event.py

+        return argumentSpec
+
+    def execute_module(self):
+        self.client = DynamicClient(kubernetes.config.new_client_from_config())


Still need to remove this line

fabianvf · 2021-09-17T16:45:57Z

plugins/modules/k8s_event.py

+import copy
+import datetime
+import traceback
+import plugins.module_utils as moduleUtils


you'll need to import it like the other module_utils on line 176, ie

from ansible_collections.operator_sdk.util.plugins.module_utils.args_common import (AUTH_ARG_SPEC)

and it will need to be inside that try block, which prevents the module from failing on initialization (we want them to fail on execution so that we can report the message to the user)

fabianvf · 2021-09-17T16:48:21Z

plugins/modules/k8s_event.py

+        return argumentSpec
+
+    def execute_module(self):
+        self.client = moduleUtils.get_api_client(None)


you'll want to pass self in so that the function can pull the proper variables out of the module

VenkatRamaraju · 2021-09-20T06:00:04Z

plugins/modules/k8s_event.py

+        instance = self.client.resources.get(api_version='v1', kind='Event')
+        try:
+            instance.patch(body=event)
+        except openshift.dynamic.exceptions.NotFoundError:
+            instance.create(body=event)
+
+        return instance


@fabianvf - Just wanted to make sure this is how the object is sent to the API. Let me know if any changes need to be made.

you'll want to return what you've documented you'll return here: https://github.com/operator-framework/operator-sdk-ansible-util/pull/22/files#diff-b2c14be32fda6eb09623e5d6d9be6350eaea5261d3c51eb8c6267fa653bc8072R131-R166

You'll also need to return whether the task resulted in a change or not (for Ansible), see the k8s_status module: https://github.com/operator-framework/operator-sdk-ansible-util/blob/master/plugins/modules/k8s_status.py#L465

so by returning instance what you're actually doing is returning the v1.Event api, what you want to return is the result of the patch or create. You'll also need to get the object from the api so that you can compare it to the result of the patch (if they are identical then you'd return changed=False. So the logic would look more like (this is pseudocode):

v1_events = self.client.resources.get(api_version='v1', kind='Event') try: instance = v1_events.get(name=name, namespace=namespace) except openshift.dynamic.exceptions.NotFoundError: result = instance.create(body=event) return dict(result=result.to_dict(), changed=True) result = instance.patch(body=event) changed = some_comparison_method(instance, result) return dict(result=result.to_dict(), changed=changed)

Yup, just updated the documentation return section to match the event object.

Also, do you you mean v1_events.create(body=event) in the Except block? I don't think we can reference instance in the Except block if it is declared in the try block. If yes, I just made those changes.

fabianvf · 2021-09-20T18:14:23Z

plugins/modules/k8s_event.py

+            "type": self.params.get("type"),
+        }
+
+        instance = self.client.resources.get(api_version='v1', kind='Event')


instance is not really true, this is not an instance of an event, this is actually the api client for the v1.Event api

fabianvf · 2021-09-20T18:23:12Z

plugins/modules/k8s_event.py

+        instance = self.client.resources.get(api_version='v1', kind='Event')
+        try:
+            instance.patch(body=event)
+        except openshift.dynamic.exceptions.NotFoundError:
+            instance.create(body=event)
+
+        return instance


you'll want to return what you've documented you'll return here: https://github.com/operator-framework/operator-sdk-ansible-util/pull/22/files#diff-b2c14be32fda6eb09623e5d6d9be6350eaea5261d3c51eb8c6267fa653bc8072R131-R166

You'll also need to return whether the task resulted in a change or not (for Ansible), see the k8s_status module: https://github.com/operator-framework/operator-sdk-ansible-util/blob/master/plugins/modules/k8s_status.py#L465

so by returning instance what you're actually doing is returning the v1.Event api, what you want to return is the result of the patch or create. You'll also need to get the object from the api so that you can compare it to the result of the patch (if they are identical then you'd return changed=False. So the logic would look more like (this is pseudocode):

v1_events = self.client.resources.get(api_version='v1', kind='Event') try: instance = v1_events.get(name=name, namespace=namespace) except openshift.dynamic.exceptions.NotFoundError: result = instance.create(body=event) return dict(result=result.to_dict(), changed=True) result = instance.patch(body=event) changed = some_comparison_method(instance, result) return dict(result=result.to_dict(), changed=changed)

fabianvf

You'll also need to add some end-to-end molecule tests that exercise this module to the molecule/default scenario (create a new task file in molecule/default/tasks and import it from molecule/default/converge.yml)

fabianvf · 2021-09-21T16:41:06Z

plugins/modules/k8s_event.py

+        try:
+            instance = v1_events.get(name=self.params.get("name"), namespace=self.params.get("namespace"))
+        except openshift.dynamic.exceptions.NotFoundError:
+            result = v1_events.create(body=event, namespace=self.params.get("namespace"))


we'll also want to wrap the api calls in try...except blocks so we can fail gracefully

fabianvf · 2021-09-21T16:41:46Z

plugins/modules/k8s_event.py

+
+        result = instance.patch(body=event, namespace=self.params.get("namespace"))
+        changed = instance.to_dict() == result.to_dict()
+        return dict(result=result.to_dict(), changed=changed)


to_dict is a little expensive so we should only call it once

VenkatRamaraju · 2021-09-22T17:35:24Z

plugins/modules/k8s_event.py

+
+        if self.params['appendTimestamp']:
+            event = {
+                "kind": "Event",
+                "eventTime": None,
+                "message": self.params.get("message"),
+                "metadata": metadata,
+                "reason": self.params.get("reason"),
+                "reportingComponent": self.params.get("reportingComponent"),
+                "source": self.params.get("source"),
+                "type": self.params.get("type"),
+            }
+
+            created_event = v1_events.create(body=event, namespace=self.params.get("namespace"))
+            return dict(result=created_event.to_dict(), changed=True)


@fabianvf - just wanted to make sure you saw this change. I created a new event and instantly returned it if the appendTimestamp is set to true, since it'll be unique in that case.

fabianvf · 2021-09-23T15:54:38Z

molecule/default/tasks/k8s_events.yml

+
+- assert:
+    that:
+      - event_obj.name == 'test-name'


Rather than asserting these on the event returned by the task, you should use the community.kubernetes.k8s_info module to retrieve the event from the API and run the asserts on that response (just to ensure the new module can't interfere with the assertions with incorrect return values or something like that)

fabianvf · 2021-09-23T15:55:07Z

plugins/modules/k8s_event.py

+
+extends_documentation_fragment:
+    - operator_sdk.util.osdk_auth_options
+    - operator_sdk.util.osdk_name_options


fabianvf · 2021-09-23T15:57:14Z

plugins/modules/k8s_event.py

+        "name": {"type": "str", "required": True},
+        "namespace": {"type": "str", "required": True},
+    },
+    "api": {},


What is this field used for?

fabianvf · 2021-09-23T15:57:45Z

plugins/modules/k8s_event.py

+        "namespace": {"type": "str", "required": True},
+    },
+    "api": {},
+    "api_version": {"default": "v1"},


you don't need these fields, the only valid api_version is v1 and the only valid kind is Event

fabianvf · 2021-09-23T15:58:03Z

plugins/modules/k8s_event.py

+        return argumentSpec
+
+    def execute_module(self):
+        self.client = DynamicClient(kubernetes.config.new_client_from_config())


fabianvf · 2021-09-23T15:58:58Z

plugins/modules/k8s_event.py

+        if self.params['appendTimestamp']:
+            event = {
+                "kind": "Event",
+                "eventTime": None,


We should probably be setting this

fabianvf · 2021-09-23T15:59:32Z

plugins/modules/k8s_event.py

+                "type": self.params.get("type"),
+            }
+
+            created_event = v1_events.create(body=event, namespace=self.params.get("namespace"))


this'll need to be in a try .. except block

fabianvf · 2021-09-23T16:00:31Z

plugins/modules/k8s_event.py

+        event = {
+            "kind": "Event",
+            "count": prior_count,
+            "eventTime": None,


fabianvf · 2021-09-23T16:00:54Z

plugins/modules/k8s_event.py

+            except Exception as err:
+                self.fail_json(msg="Unable to create event: {0}".format(err))
+
+        result = v1_events.patch(body=event, namespace=self.params.get("namespace"))


This will also need to be in a try ... except block

VenkatRamaraju · 2021-09-23T21:04:21Z

Addressed all changes, working on molecule tests now.

VenkatRamaraju force-pushed the master branch 2 times, most recently from 27ae229 to 598f13e Compare September 14, 2021 16:58

fabianvf suggested changes Sep 16, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch 3 times, most recently from 3dead8d to 8919e72 Compare September 17, 2021 16:52

fabianvf suggested changes Sep 17, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch 3 times, most recently from 08f0716 to f848f17 Compare September 20, 2021 04:43

VenkatRamaraju requested a review from fabianvf September 20, 2021 05:59

VenkatRamaraju commented Sep 20, 2021

View reviewed changes

fabianvf reviewed Sep 20, 2021

View reviewed changes

fabianvf suggested changes Sep 20, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch from d5a2c28 to 77fe9fd Compare September 20, 2021 21:33

VenkatRamaraju requested a review from fabianvf September 20, 2021 22:12

fabianvf suggested changes Sep 21, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch 2 times, most recently from 4e81714 to f3f1260 Compare September 22, 2021 16:31

VenkatRamaraju requested a review from fabianvf September 22, 2021 17:32

VenkatRamaraju commented Sep 22, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch from d2aa70e to 4d3d208 Compare September 23, 2021 00:36

fabianvf suggested changes Sep 23, 2021

View reviewed changes

VenkatRamaraju force-pushed the master branch from ea20b04 to ebf4d3f Compare September 23, 2021 17:42

VenkatRamaraju requested a review from fabianvf September 23, 2021 21:04

VenkatRamaraju force-pushed the master branch 4 times, most recently from c6f1e11 to 84fb24e Compare September 24, 2021 17:57

Module

4c5a22d

VenkatRamaraju force-pushed the master branch from 84fb24e to 4c5a22d Compare September 24, 2021 18:07

fabianvf approved these changes Sep 27, 2021

View reviewed changes

fabianvf merged commit c6a9d40 into operator-framework:master Sep 27, 2021

fabianvf mentioned this pull request Nov 3, 2021

Add k8s_event module to collection #2

Closed

k8s_event module #22

k8s_event module #22

Uh oh!

Conversation

VenkatRamaraju commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VenkatRamaraju Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianvf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VenkatRamaraju Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VenkatRamaraju commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

VenkatRamaraju commented Sep 13, 2021 •

edited

Loading

VenkatRamaraju Sep 23, 2021 •

edited

Loading

VenkatRamaraju Sep 22, 2021 •

edited

Loading