POWERMON-235: Add support for Redfish #364

vimalk78 · 2024-03-02T08:53:49Z

This PR

Extends Kepler and KeplerInternal API to add Exporter.Redfish
Redfish spec has fields for adding reference to a secret which should
contain Redfish credentials.
The user supplied secret is mounted in the kepler container, and daemon set spec is changed which causes kepler containers to restart.
Added Unit tests and e2e test

sunya-ch

I think this PR is worth merging for configuring the redfish. Please kindly find my detailed comments below.

sunya-ch · 2024-03-04T04:56:41Z

pkg/components/exporter/exporter_test.go

@@ -93,82 +93,98 @@ func TestTolerations(t *testing.T) {
 	}
 }

-func TestHostPID(t *testing.T) {
+func Test_DaemonSet(t *testing.T) {


change to TestDaemonSet to keep the same format to the other test function.

sunya-ch · 2024-03-04T05:08:05Z

pkg/api/v1alpha1/kepler_internal_types.go

 type InternalExporterSpec struct {
 	// +kubebuilder:validation:Required
 	Deployment InternalExporterDeploymentSpec `json:"deployment"`
+
+	Redfish *InternalRedfishSpec `json:"redfish,omitempty"`


We are going to extend spec for configuring the other metrics as well (such as to enable/disable cgroups metrics and so on) - check #202.
Thus, I think we should move Redfish to be under metric.energy and resource utilization metrics such as cGroups should go under metric.resource.

Refer to terms in Kepler:

// https://github.com/sustainable-computing-io/kepler/blob/3d0bad753d68f74a0955f6e25d2a9d8e93507d08/pkg/collector/stats/stats.go#L47 type Stats struct { ResourceUsage map[string]*types.UInt64StatCollection EnergyUsage map[string]*types.UInt64StatCollection }

This seems to be a bigger change to the API. Can we do this api change in a separate PR?

Sure. I think we can update that on a different PR. @sthaha What do you think?

I am in favour of keeping redfish right under exporter since it isn't metrics or deployment related rather is is power meter and having exporter.redfish is acceptable. I might consider exporter.powermeter.redfish (or something else) at a later stage.

sunya-ch · 2024-03-04T05:40:12Z

This PR made a change on Kepler resource. Shall we first limit this change to only KeplerInternal?
Also, if redfish.secretRef is not set in the CR, it seems to make it not mounting since there is no default value for that.

vimalk78 · 2024-03-04T05:45:14Z

Also, if redfish.secretRef is not set in the CR, it seems to make it not mounting

thats by design. once secretRef is set, then only -redfish-cred-file-path is added to container

sthaha · 2024-03-04T22:53:31Z

pkg/components/exporter/exporter.go

+	REDFISH_ARGS          = "-redfish-cred-file-path=/etc/redfish/redfish.csv"
+	REDFISH_CSV           = "redfish.csv"
+	REDFISH_ANNOTATION    = "kepler.system.sustainable.computing.io/redfish-secret-ref"


could you please change this to follow the convention?
If these constants aren't used outside the package, lets make them private.

sthaha · 2024-03-04T22:57:59Z

pkg/controllers/kepler_internal.go

+func (r KeplerInternalReconciler) getRedfishSecret(secretName string) (*corev1.Secret, error) {
+	redfishSecret := corev1.Secret{}
+	redFishSecretName := types.NamespacedName{
+		Namespace: KeplerDeploymentNS,


shouldn't this be the namespace that you get from the kepler-internal instead of the deployment namespace of kepler ?
As a rule of thumb, internal must not use (or have any knowledge) of Kepler related config or variables.

sthaha · 2024-03-04T23:12:55Z

pkg/controllers/kepler_internal.go

+		rs = append(rs,
+			KeplerDaemonSetReconciler{
+				ki:      *ki,
+				updater: updateResource,


All single resource reconcilers should move to pkg/reconcilers to provide decoupling.
Since KeplerDaemonSetReconciler already has ki which should be be owner, it need not be passed the updater and can create a generic updater{Owner: ki, Resource: ds}.

Note: reconcileFn is a private type within the controller to ease creation of reconciler

// # pkg/reconciler/kepler_daemonset.go func (r ) Reconcile(..) { ... return Updater{Owner: ki, }.Reconcile(...) }

sthaha · 2024-03-04T23:15:41Z

pkg/controllers/kepler_internal.go

 	rs = append(rs, resourceReconcilers(updateResource, openshiftNamespacedResources(ki, cluster)...)...)
 	return rs
 }

+type KeplerDaemonSetReconciler struct {


this should move to pkg/reconciler/ where this can be unit tested as well.

sthaha · 2024-03-04T23:42:45Z

pkg/controllers/kepler_internal.go

+
+	requests := []reconcile.Request{}
+	for _, ki := range ks.Items {
+		if secret.ObjectMeta.Name == ki.Spec.Exporter.Redfish.SecretRef {


shouldn't we also check the namespace matches deployment.namespace

sthaha · 2024-03-04T23:43:45Z

pkg/controllers/kepler_internal.go

 	if Config.Cluster == k8s.OpenShift {
 		c = c.Owns(&secv1.SecurityContextConstraints{}, genChanged)
 	}
 	return c.Complete(r)
 }

+// mapSecretToRequests returns the reconcile requests for kepler-internal objects for which an associated redfish secret has been changed.
+func (r *KeplerInternalReconciler) mapSecretToRequests(ctx context.Context, object client.Object) []reconcile.Request {


Did you consider Own(ing) the secret instead, which wouldn't require us to map secrets to k-i manually?

sthaha · 2024-03-04T23:45:58Z

pkg/controllers/kepler_internal.go

+		if errors.IsNotFound(err) {
+			return nil, nil
+		}
+		return nil, err


nit: return nil, errors.IgnoreNotFound(err)

sthaha · 2024-03-05T00:22:12Z

pkg/components/exporter/exporter.go

@@ -287,7 +303,7 @@ func NewClusterRole(c components.Detail, k *v1alpha1.KeplerInternal) *rbacv1.Clu
 		},
 		Rules: []rbacv1.PolicyRule{{
 			APIGroups: []string{""},
-			Resources: []string{"nodes/metrics", "nodes/proxy", "nodes/stats", "pods"},
+			Resources: []string{"nodes/metrics", "nodes/proxy", "nodes/stats", "pods", "secrets"},


this shouldn't be needed since kepler does not need to watch secrets.

sthaha · 2024-03-05T00:26:27Z

pkg/controllers/kepler.go

+	var redfishSpec *v1alpha1.InternalRedfishSpec = nil
+	if k.Spec.Exporter.Redfish != nil {
+		redfishSpec = &v1alpha1.InternalRedfishSpec{
+			RedfishSpec: *k.Spec.Exporter.Redfish,
+		}
+	}


Since this an api added to Kepler, we should first add this as v1alpha1.RedfishSpec and resuse that in the internal. As as rule of thumb, Internal can resue Kepler but not the other way round.

sthaha · 2024-03-05T00:40:58Z

pkg/api/v1alpha1/kepler_internal_types.go

+type InternalRedfishSpec struct {
+	RedfishSpec `json:",inline"`
+}
+


We shouldn't need a separate internal redfish spec since the one in kepler can be reused.

sthaha · 2024-03-05T00:44:59Z

pkg/api/v1alpha1/kepler_types.go

+	// +optional
+	// +kubebuilder:validation:Pattern=`\d+`
+	// +kubebuilder:default:="60"
+	ProbeInterval string `json:"probeIntervalSec,omitempty"`


can we use metav1.Duration instead and convert to seconds when the configmap is created?
also rename the field to json:probeInterval ?

https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/duration.go

sthaha · 2024-03-05T01:06:34Z

pkg/controllers/kepler_internal.go

+func (r KeplerDaemonSetReconciler) Reconcile(ctx context.Context, cli client.Client, s *runtime.Scheme) reconciler.Result {
+
+	secretRef := r.ki.Spec.Exporter.Redfish.SecretRef
+	secret, err := r.getRedfishSecret(ctx, cli, secretRef)


I think we should fail reconcile, set the status to available false, if a secret is specified and is missing or invalid (missing the redfish.csv) . Otherwise there is no way for user to know if kepler is using redfish or not.

sthaha · 2024-03-05T01:09:48Z

@vimalk78 nice work overall. It worked pretty well in my test. But few comments to address.
The main one being that secret must be present for kepler deployment to continue and the reason for failure to deploy must be reported back in Availability status.

sthaha · 2024-03-05T01:29:54Z

pkg/controllers/kepler_internal.go

@@ -74,12 +77,39 @@ func (r *KeplerInternalReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&rbacv1.ClusterRoleBinding{}, genChanged).
 		Owns(&rbacv1.ClusterRole{}, genChanged)

+	c = c.Watches(&corev1.Secret{},
+		handler.EnqueueRequestsFromMapFunc(r.mapSecretToRequests))


Also, handler.ResourceVersionChanged{}

sthaha · 2024-03-05T01:35:48Z

pkg/controllers/kepler_internal.go

+	}
+
+	ks := v1alpha1.KeplerInternalList{}
+	if err := r.List(ctx, &ks); err != nil {


I think we can improve the perf a bit more by using the fieldSelector
see: https://book.kubebuilder.io/reference/watching-resources/externally-managed

sthaha · 2024-03-05T01:36:26Z

pkg/controllers/kepler_internal.go

+	secret, ok := object.(*corev1.Secret)
+	if !ok {
+		return nil
+	}


This should not be needed since the watch is already set for v1.Secret and we can make use of GetName() and GetNamespace()

- Extended Kepler and KeplerInternal API to add `Exporter.Redfish` - Redfish spec has fields for adding reference to a secret which should contain Redfish credentials. - The user supplied secret is mounted in the kepler container. - Added Unit tests and e2e test Signed-off-by: Vimal Kumar <[email protected]>

sthaha · 2024-03-10T23:01:02Z

config/rbac/role.yaml

@@ -9,6 +9,7 @@ rules:
  resources:
  - daemonsets
  - deployments
+  - secrets


we don't create secrets do we ?

sthaha · 2024-03-10T23:03:20Z

pkg/components/exporter/exporter.go

+	ds.Spec.Template.Annotations = map[string]string{
+		REDFISH_ANNOTATION: secret.ResourceVersion,
+	}


I think it may be sufficient to just annotate the ds instead of the template. Lets handle this in a different PR.

sthaha · 2024-03-10T23:04:59Z

pkg/controllers/kepler_internal.go

 //+kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,verbs=list;watch;create;update;patch;delete;use
 //+kubebuilder:rbac:groups=monitoring.coreos.com,resources=servicemonitors;prometheusrules,verbs=list;watch;create;update;patch;delete

 // RBAC required by Kepler exporter
-//+kubebuilder:rbac:groups=core,resources=nodes/metrics;nodes/proxy;nodes/stats,verbs=get;list;watch
+//+kubebuilder:rbac:groups=core,resources=nodes/metrics;nodes/proxy;nodes/stats;secrets,verbs=get;list;watch


This shouldn't be necessary since Kepler exporter does not watch or create secrets.

sthaha · 2024-03-10T23:06:59Z

tests/run-e2e.sh

@@ -302,7 +302,7 @@ restart_operator() {
 	info "scale down Operator"
 	run kubectl scale -n "$OPERATORS_NS" --replicas=0 "$deployment"
 	run kubectl wait -n "$OPERATORS_NS" --for=delete \
-		pods -l app.kubernetes.io/component=operator --timeout=60s
+		pods -l app.kubernetes.io/component=manager --timeout=60s


vimalk78 force-pushed the add-redfish-credentials branch 10 times, most recently from 2fde234 to d1ab5ab Compare March 3, 2024 16:15

vimalk78 requested review from sthaha and sunya-ch March 4, 2024 04:46

sunya-ch reviewed Mar 4, 2024

View reviewed changes

vimalk78 force-pushed the add-redfish-credentials branch from d1ab5ab to 4fb666d Compare March 4, 2024 05:18

sthaha reviewed Mar 4, 2024

View reviewed changes

sthaha requested changes Mar 5, 2024

View reviewed changes

sthaha reviewed Mar 5, 2024

View reviewed changes

vimalk78 force-pushed the add-redfish-credentials branch from 4fb666d to 8ce215e Compare March 8, 2024 11:40

sthaha reviewed Mar 10, 2024

View reviewed changes

config/rbac/role.yaml

@@ -9,6 +9,7 @@ rules:

resources:

- daemonsets

- deployments

- secrets

Copy link

Collaborator

sthaha Mar 10, 2024

Choose a reason for hiding this comment

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

we don't create secrets do we ?

sthaha reviewed Mar 10, 2024

View reviewed changes

sthaha approved these changes Mar 10, 2024

View reviewed changes

sthaha merged commit c46fb30 into sustainable-computing-io:v1alpha1 Mar 10, 2024
9 checks passed

sthaha mentioned this pull request Mar 11, 2024

feat: mount redfish credentials #191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POWERMON-235: Add support for Redfish #364

POWERMON-235: Add support for Redfish #364

vimalk78 commented Mar 2, 2024 •

edited

Loading

sunya-ch left a comment •

edited

Loading

sunya-ch Mar 4, 2024

sunya-ch Mar 4, 2024

vimalk78 Mar 4, 2024

sunya-ch Mar 4, 2024

sthaha Mar 5, 2024

sunya-ch commented Mar 4, 2024 •

edited

Loading

vimalk78 commented Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 4, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha commented Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024

sthaha Mar 5, 2024 •

edited

Loading

sthaha Mar 10, 2024

sthaha Mar 10, 2024

sthaha Mar 10, 2024

sthaha Mar 10, 2024

POWERMON-235: Add support for Redfish #364

POWERMON-235: Add support for Redfish #364

Conversation

vimalk78 commented Mar 2, 2024 • edited Loading

sunya-ch left a comment • edited Loading

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

sunya-ch commented Mar 4, 2024 • edited Loading

vimalk78 commented Mar 4, 2024

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

sthaha commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Mar 5, 2024 • edited Loading

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

vimalk78 commented Mar 2, 2024 •

edited

Loading

sunya-ch left a comment •

edited

Loading

sunya-ch commented Mar 4, 2024 •

edited

Loading

sthaha Mar 5, 2024 •

edited

Loading