Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow multiple kepler deployments using kepler-internal #309

Merged
merged 4 commits into from
Nov 30, 2023
Merged

feat: allow multiple kepler deployments using kepler-internal #309

merged 4 commits into from
Nov 30, 2023

Conversation

sthaha
Copy link
Collaborator

@sthaha sthaha commented Nov 21, 2023 •
edited
Loading

Important Changes:

  • allows setting deployment-namespace instead of hardcoded openshift-kepler-operator
  • modifies the default kepler deployment namespace to kepler-operator
  • Allows watching additional namespaces using watch-namepaces=foo,bar --watch-namespaces=baz (multiple list)
  • Adds status.exporter to indicate kepler deployment conditions

** Testing the PR locally**
To give this a spin on kind cluster created using make fresh

operator-sdk run bundle quay.io/sthaha/kepler-operator-bundle:0.0.0-kepler-internal \
  --namespace=operators \
  --use-http

@sthaha sthaha marked this pull request as draft November 21, 2023 12:18
@sthaha sthaha force-pushed the feat-multiple-kepler branch from 0336482 to 4b05247 Compare November 21, 2023 23:15
@sthaha sthaha changed the title WIP: Allow multiple kepler deployments feat: allow multiple kepler deployments using kepler-internal Nov 22, 2023
@sthaha sthaha marked this pull request as ready for review November 22, 2023 02:30
@sthaha sthaha requested a review from vimalk78 November 22, 2023 03:51
@vprashar2929
Copy link
Collaborator

In case of multiple kepler deployments we have to figure out openshift dashboard's deployments and also revise our prometheus rules accordingly.

@sthaha
Copy link
Collaborator Author

sthaha commented Nov 22, 2023

In case of multiple kepler deployments we have to figure out openshift dashboard's deployments and also revise our prometheus rules accordingly.

Thats not a supported scenario to start with. I am thinking of adding a

spec:
  dashboards: 
      enabled: t|f
  exporter:
     ....

to KeplerInternal where dashboards.enabled defaults to false. I also think the prometheusrule may be giving wrong values since we are not filter out by service.

@sthaha sthaha force-pushed the feat-multiple-kepler branch 2 times, most recently from 729a95a to 57abbd9 Compare November 23, 2023 02:23
@sthaha
Copy link
Collaborator Author

sthaha commented Nov 23, 2023

@vprashar2929 I have added an openshift specific api now to enable / disable dashboards.

@sthaha sthaha force-pushed the feat-multiple-kepler branch 4 times, most recently from d1d9e4f to 0349e21 Compare November 29, 2023 09:08
@sthaha sthaha mentioned this pull request Nov 29, 2023
Closed
@sthaha
fix(test): local e2e test now validates the correct container
c24b7a5 
Kepler container is container[0] and not 1 which was a change introduced
in ae09380

Signed-off-by: Sunil Thaha <[email protected]>
@sthaha
feat: add kepler-internal CRD
b260064 
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]>
@sthaha
feat: Allow image to be specified in Internal API
4ba0464 
Signed-off-by: Sunil Thaha <[email protected]>
@sthaha sthaha force-pushed the feat-multiple-kepler branch 4 times, most recently from 0c5bd59 to 539be06 Compare November 30, 2023 03:17
vimalk78
vimalk78 reviewed Nov 30, 2023
View reviewed changes
cmd/manager/main.go
flag.StringVar(&controllers.KeplerDeploymentNS, "deployment-namespace", controllers.KeplerDeploymentNS,
"Namespace where kepler and its components are deployed.")

flag.CommandLine.Var(flag.Value(&additionalNamespaces), "watch-namespaces",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we elaborate a bit how this parameter is to be used? is this for running multiple kepler-exporters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also thinking of allowing a * as one of the options to allow watching all namespaces but lets do it when we really find a need.

vimalk78
vimalk78 reviewed Nov 30, 2023
View reviewed changes
pkg/components/exporter/exporter.go
nodeSelector := deployment.NodeSelector
tolerations := deployment.Tolerations
port := deployment.Port
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for multiple kepler deployments, the port also has to be different. Users must ensure that.

vimalk78
vimalk78 reviewed Nov 30, 2023
View reviewed changes
tests/e2e/kepler_test.go Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a similar e2e test for keplerinternal ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be in another PR.

@sthaha sthaha mentioned this pull request Nov 30, 2023
Closed
@sthaha sthaha force-pushed the feat-multiple-kepler branch from 539be06 to 16a3074 Compare November 30, 2023 09:21
@sthaha
feat: allow multiple kepler instance to be deployed
17a01c9 
This commit adds a `namespace` to kepler-internal spec which is the
namespace to which kepler will be deployed.

Signed-off-by: Sunil Thaha <[email protected]>
@sthaha sthaha force-pushed the feat-multiple-kepler branch from 16a3074 to 17a01c9 Compare November 30, 2023 09:29
vimalk78
vimalk78 approved these changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@vimalk78 vimalk78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vimalk78 vimalk78 merged commit 6b27889 into sustainable-computing-io:v1alpha1 Nov 30, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vprashar2929 vprashar2929 vprashar2929 left review comments

@vimalk78 vimalk78 vimalk78 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sthaha @vprashar2929 @vimalk78