feat: add kepler-internal CRD #306

sthaha · 2023-11-13T02:14:24Z

This commit adds a KeplerInternal CRD which is a internal API of Kepler Operator. Kepler CRD is now a facade for KeplerInternal Creating a Kepler now creates a corresponding keplerinternal CR with appropriate spec initialised. The KeplerInternal Controller responds creation of kepler-internal by deploying kepler.

This separation allows for providing stable API - Kepler or other higher level api (such as powermon.openshift.io - PowerMonitoring) which provides a limited set of stable functionalities to users.

In this PR, I have added the provision to set export.deployment.Image in the keplerinternal API.

NOTE:* KeplerInternal API can break at any point in time, so its usage is highly discouraged other than for testing experimental features.

Kepler container is container[0] and not 1 which was a change introduced in ae09380 Signed-off-by: Sunil Thaha <[email protected]>

This commit adds a `KeplerInternal` CRD which is a internal API of Kepler Operator. Kepler CRD is now a facade for `KeplerInternal` Creating a `Kepler` now creates a corresponding `keplerinternal` CR with appropriate spec initialised. The KeplerInternal Controller responds creation of `kepler-internal` by deploying kepler. This separation allows for providing stable API - Kepler or other higher level api (such as `powermon.openshift.io - PowerMonitoring`) which provides a limited set of stable functionalities to users. NOTE: KeplerInternal API can break at any point in time, so its usage is highly discouraged other than for testing experimental features. Signed-off-by: Sunil Thaha <[email protected]>

Signed-off-by: Sunil Thaha <[email protected]>

sthaha · 2023-11-17T04:36:11Z

@vprashar2929 could you please give this a go on openshift as well?

sthaha · 2023-11-17T04:57:46Z

Lets wait for @vprashar2929 to run the tests before merging this 👼

bundle/manifests/kepler.system.sustainable.computing.io_keplerinternals.yaml

vprashar2929 · 2023-11-17T11:08:13Z

When kepler deployed using Kepler API the instance also visible in KeplerInternal as well

~ ❯ oc get kepler -o wide ; oc get keplerinternals -o wide                                                                                         ⎈ 
NAME     PORT   DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   AGE     NODE-SELECTOR                  TOLERATIONS
kepler   9103   6         6         6       6            6           2m29s   {"kubernetes.io/os":"linux"}   [{"operator":"Exists"}]
NAME     PORT   DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   AGE     IMAGE                                                   NODE-SELECTOR                  TOLERATIONS
kepler   9103   6         6         6       6            6           2m33s   quay.io/sustainable_computing_io/kepler:release-0.6.1   {"kubernetes.io/os":"linux"}   [{"operator":"Exists"}]

sthaha · 2023-11-20T01:41:06Z

When kepler deployed using Kepler API the instance also visible in KeplerInternal as well

Like the PR mentions, Kepler is now a facade over KeplerInternal, so when user creates Kepler, kepler-controller creates KeplerInternal and then kepler-internal controller creates the daemonset, etc ..

config/crd/bases/kepler.system.sustainable.computing.io_keplerinternals.yaml

vimalk78 · 2023-11-23T04:29:49Z

config/crd/bases/kepler.system.sustainable.computing.io_keplerinternals.yaml

+          metadata:
+            type: object
+          spec:
+            description: KeplerInternalSpec defines the desired state of Kepler


desired state of KeplerInternal

vimalk78 · 2023-11-23T04:31:00Z

config/crd/bases/kepler.system.sustainable.computing.io_keplerinternals.yaml

+                properties:
+                  deployment:
+                    properties:
+                      image:


add description? since users can set the image in KeplerInternal which is not allowed in Kepler

vimalk78 · 2023-11-23T04:40:00Z

config/manifests/bases/kepler-operator.clusterserviceversion.yaml

+    - description: KeplerInternal is the Schema for internal/unsupported API
+      displayName: KeplerInternal
+      kind: KeplerInternal
+      name: keplerinternals.kepler.system.sustainable.computing.io
+      version: v1alpha1


add statusDescriptors for KeplerInternal same as that of Kepler ?

vimalk78 · 2023-11-23T04:47:01Z

config/crd/bases/kepler.system.sustainable.computing.io_keplerinternals.yaml

+                    type: object
+                type: object
+            type: object
+          status:


question: the status field today is KeplerStatus, for deployment of exporter pods via daemonset.
In the future when model-server is added, a new type ExporterStatus is to be added and all exporter status fields moved there? somewhere need to differentiate exporter status from future added status.

vimalk78 · 2023-11-23T04:57:07Z

config/crd/bases/kepler.system.sustainable.computing.io_keplerinternals.yaml

+                type: object
+            type: object
+          status:
+            description: KeplerStatus defines the observed state of Kepler


nit: the type for status is getting reused here, but description says observed state of Kepler

vimalk78 · 2023-11-23T05:10:17Z

pkg/controllers/kepler_internal.go

+	if kepler == nil {
+		// no kepler found , so stop here
+		logger.V(6).Info("Kepler Nil")
+		return ctrl.Result{}, nil
+	}


nit: rename variable to ki . should the log say KeplerInternal Nil

vimalk78 · 2023-11-23T05:32:08Z

pkg/controllers/kepler_internal.go

+		return ctrl.Result{}, nil
+	}
+
+	logger.V(6).Info("Running sub reconcilers", "kepler", kepler.Spec)


running kepler internal ?

same comment for other logs in this file, should it say kepler or keplerinternal ?

vimalk78 · 2023-11-23T05:44:29Z

pkg/controllers/kepler_test.go

@@ -0,0 +1,61 @@
+package controllers


add license comment header

vimalk78 · 2023-11-23T05:44:52Z

pkg/controllers/kepler_internal.go

@@ -0,0 +1,403 @@
+package controllers


add license comment header

sthaha · 2023-11-29T09:19:20Z

@vimalk78 @sunya-ch , I am closing this PR in favour of #309

sthaha force-pushed the feat-kepler-internal-api branch 3 times, most recently from 05a0493 to 1addde7 Compare November 14, 2023 02:40

fix(test): local e2e test now validates the correct container

3fd788d

Kepler container is container[0] and not 1 which was a change introduced in ae09380 Signed-off-by: Sunil Thaha <[email protected]>

sthaha force-pushed the feat-kepler-internal-api branch 3 times, most recently from 97c807f to 276e80f Compare November 17, 2023 02:12

sthaha force-pushed the feat-kepler-internal-api branch 3 times, most recently from 7fa8d1e to b72ca9e Compare November 17, 2023 03:03

feat: Allow image to be specified in Internal API

cee1bb3

Signed-off-by: Sunil Thaha <[email protected]>

sthaha force-pushed the feat-kepler-internal-api branch from b72ca9e to cee1bb3 Compare November 17, 2023 03:45

sthaha requested a review from vimalk78 November 17, 2023 04:35

sthaha added the do-not-merge label Nov 17, 2023

vprashar2929 reviewed Nov 17, 2023

View reviewed changes

bundle/manifests/kepler.system.sustainable.computing.io_keplerinternals.yaml Show resolved Hide resolved

vimalk78 reviewed Nov 23, 2023

View reviewed changes

sthaha closed this Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add kepler-internal CRD #306

feat: add kepler-internal CRD #306

sthaha commented Nov 13, 2023 •

edited

Loading

sthaha commented Nov 17, 2023

sthaha commented Nov 17, 2023

vprashar2929 commented Nov 17, 2023

sthaha commented Nov 20, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

vimalk78 Nov 23, 2023

sthaha Nov 23, 2023

vimalk78 Nov 23, 2023

sthaha commented Nov 29, 2023

feat: add kepler-internal CRD #306

feat: add kepler-internal CRD #306

Conversation

sthaha commented Nov 13, 2023 • edited Loading

sthaha commented Nov 17, 2023

sthaha commented Nov 17, 2023

vprashar2929 commented Nov 17, 2023

sthaha commented Nov 20, 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

sthaha commented Nov 29, 2023

sthaha commented Nov 13, 2023 •

edited

Loading