chore: review comment changes

Author: tanishagoyal2

distros/kubernetes/nvsentinel/charts/health-events-analyzer/values.yaml

@@ -51,7 +51,7 @@ config: |
   errorCount = 5
 
   [[rules]]
-  name = "RepeatedXid13"
+  name = "RepeatedXidError"
   description = "Detect occurrence of fatal XIDs 5 times within 24 hours"
   time_window = "24h"
   recommended_action = "REPORT_ISSUE"

health-events-analyzer/go.mod

@@ -16,7 +16,6 @@ require (
 	google.golang.org/grpc v1.76.0
 	google.golang.org/protobuf v1.36.10
 	k8s.io/apimachinery v0.34.1
-	k8s.io/klog v1.0.0
 )
 
 require (

health-events-analyzer/go.sum

@@ -11,7 +11,6 @@ github.com/emicklei/go-restful/v3 v3.13.0 h1:C4Bl2xDndpU6nJ4bc1jXd+uTmYPVUwkD6bF
 github.com/emicklei/go-restful/v3 v3.13.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
 github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM=
 github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ=
-github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas=
 github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI=
 github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
@@ -187,8 +186,6 @@ k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4=
 k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw=
 k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY=
 k8s.io/client-go v0.34.1/go.mod h1:kA8v0FP+tk6sZA0yKLRG67LWjqufAoSHA2xVGKw9Of8=
-k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
-k8s.io/klog v1.0.0/go.mod h1:4Bi6QPql/J/LkTDqv7R/cd3hPo4k2DG6Ptcz060Ez5I=
 k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
 k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 h1:Y3gxNAuB0OBLImH611+UDZcmKS3g6CthxToOb37KgwE=

health-events-analyzer/pkg/config/rules.go

@@ -21,7 +21,7 @@ import (
 )
 
 type SequenceStep struct {
-	Criteria   map[string]interface{} `toml:"criteria,omitempty"`
+	Criteria   map[string]interface{} `toml:"criteria"`
 	ErrorCount int                    `toml:"errorCount"`
 }
 

health-events-analyzer/pkg/publisher/publisher.go

@@ -23,6 +23,7 @@ import (
 
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 	"k8s.io/apimachinery/pkg/util/wait"
 )
 
@@ -95,21 +96,14 @@ func NewPublisher(platformConnectorClient pb.PlatformConnectorClient) *Publisher
 
 func (p *PublisherConfig) Publish(ctx context.Context, event *pb.HealthEvent,
 	recommendedAction pb.RecommenedAction, ruleName string) error {
-	newEvent := &pb.HealthEvent{
-		Version:            event.Version,
-		Agent:              "health-events-analyzer",
-		CheckName:          ruleName, // change the check name to HealthEventsAnalyzer
-		ComponentClass:     event.ComponentClass,
-		Message:            event.Message,
-		RecommendedAction:  recommendedAction, // set the rule's recommended action
-		ErrorCode:          event.ErrorCode,
-		IsHealthy:          false, // mark event as unhealthy
-		IsFatal:            true,  // mark event as fatal
-		EntitiesImpacted:   event.EntitiesImpacted,
-		Metadata:           event.Metadata,
-		GeneratedTimestamp: event.GeneratedTimestamp,
-		NodeName:           event.NodeName,
-	}
+	newEvent := proto.Clone(event).(*pb.HealthEvent)
+
+	// Override fields with new values
+	newEvent.Agent = "health-events-analyzer"
+	newEvent.CheckName = ruleName                  // change the check name to HealthEventsAnalyzer
+	newEvent.RecommendedAction = recommendedAction // set the rule's recommended action
+	newEvent.IsHealthy = false                     // mark event as unhealthy
+	newEvent.IsFatal = true                        // mark event as fatal
 
 	req := &pb.HealthEvents{
 		Version: 1,

health-events-analyzer/pkg/reconciler/parser.go

@@ -136,7 +136,7 @@ func getValueFromErrorCode(event *platform_connectors.HealthEvent, parts []strin
 	}
 
 	idx, err := strconv.Atoi(parts[0])
-	if err != nil || idx >= len(event.ErrorCode) {
+	if err != nil || idx >= len(event.ErrorCode) || idx < 0 {
 		return nil
 	}
 
@@ -162,7 +162,7 @@ func getValueFromEntitiesImpacted(event *platform_connectors.HealthEvent, parts
 		return nil
 	}
 
-	if idx, err := strconv.Atoi(parts[0]); err == nil && idx < len(event.EntitiesImpacted) {
+	if idx, err := strconv.Atoi(parts[0]); err == nil && idx >= 0 && idx < len(event.EntitiesImpacted) {
 		entity := event.EntitiesImpacted[idx]
 		subField := strings.ToLower(parts[1])
 
@@ -193,6 +193,10 @@ func getValueFromGeneratedTimestamp(event *platform_connectors.HealthEvent, part
 }
 
 func getValueFromHealthEventStatus(event storeconnector.HealthEventStatus, parts []string) any {
+	if len(parts) == 0 {
+		return nil
+	}
+
 	rootField := strings.ToLower(parts[0])
 
 	if len(parts) == 1 {

health-events-analyzer/pkg/reconciler/reconciler.go

@@ -33,11 +33,6 @@ import (
 	"go.mongodb.org/mongo-driver/mongo/options"
 )
 
-const (
-	maxRetries int           = 5
-	delay      time.Duration = 10 * time.Second
-)
-
 type CollectionInterface interface {
 	Aggregate(ctx context.Context, pipeline interface{}, opts ...*options.AggregateOptions) (*mongo.Cursor, error)
 }
@@ -129,32 +124,20 @@ func (r *Reconciler) processEvent(ctx context.Context, event bson.M) error {
 
 	var publishedNewEvent bool
 
-	for i := 1; i <= maxRetries; i++ {
-		slog.Debug("Handling event", "attempt", i, "event", healthEventWithStatus)
-
-		publishedNewEvent, err = r.handleEvent(ctx, &healthEventWithStatus)
-		if err == nil {
-			totalEventsSuccessfullyProcessed.Inc()
-
-			if publishedNewEvent {
-				slog.Info("New event successfully published.")
-				newEventsPublishedTotal.WithLabelValues(healthEventWithStatus.HealthEvent.NodeName).Inc()
-			} else {
-				slog.Info("New event is not published, rule set criteria didn't match.")
-			}
-
-			break
-		}
-
+	publishedNewEvent, err = r.handleEvent(ctx, &healthEventWithStatus)
+	if err != nil {
 		slog.Error("Error in handling the event", "event", healthEventWithStatus, "error", err)
 
 		totalEventProcessingError.WithLabelValues("handle_event_error").Inc()
-
-		time.Sleep(delay)
-	}
-
-	if err != nil {
-		slog.Error("Max attempt reached, error in handling the event", "event", healthEventWithStatus, "error", err)
+	} else {
+		totalEventsSuccessfullyProcessed.Inc()
+
+		if publishedNewEvent {
+			slog.Info("New event successfully published.")
+			newEventsPublishedTotal.WithLabelValues(healthEventWithStatus.HealthEvent.NodeName).Inc()
+		} else {
+			slog.Info("New event is not published, rule set criteria didn't match.")
+		}
 	}
 
 	duration := time.Since(startTime).Seconds()

health-events-analyzer/pkg/reconciler/reconciler_test.go

@@ -96,7 +96,7 @@ var (
 			TimeWindow:  "2m",
 			Sequence: []config.SequenceStep{{
 				Criteria: map[string]interface{}{
-					"healthevent.entitiesimpacted.0.entitytype":  "GPU	",
+					"healthevent.entitiesimpacted.0.entitytype":  "GPU",
 					"healthevent.entitiesimpacted.0.entityvalue": "this.healthevent.entitiesimpacted[0].entityvalue",
 					"healthevent.errorcode.0":                    "13",
 					"healthevent.nodename":                       "this.healthevent.nodename",

tests/helpers/healthevent.go

@@ -198,12 +198,13 @@ func TestCleanUp(ctx context.Context, GpuNodeName string, nodeCondition string,
 	client, err := c.NewClient()
 	if err != nil {
 		klog.Errorf("failed to create client during cleanup: %v", err)
-		return err
+		return fmt.Errorf("failed to create client during cleanup: %w", err)
 	}
 
 	err = CleanupNodeConditionAndUncordon(ctx, client, GpuNodeName, nodeCondition)
 	if err != nil {
 		klog.Errorf("failed to cleanup node condition and uncordon node %s: %v", GpuNodeName, err)
+		return fmt.Errorf("failed to cleanup node condition and uncordon node %s: %w", GpuNodeName, err)
 	}
 
 	klog.Infof("Successfully cleaned up node condition and uncordoned node %s", GpuNodeName)

tests/multiple_fatal_event_test.go

@@ -22,19 +22,18 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"go.mongodb.org/mongo-driver/bson"
-	"k8s.io/klog/v2"
 	"sigs.k8s.io/e2e-framework/pkg/envconf"
 	"sigs.k8s.io/e2e-framework/pkg/features"
 )
 
 func TestMultipleFatalEventRule(t *testing.T) {
 	type contextKey int
-	var GpuNodeName string
 
 	const (
-		keyGpuNodes  contextKey = iota
-		ERRORCODE_13            = "13"
-		ERRORCODE_48            = "48"
+		keyGpuNodes contextKey = iota
+		keyGpuNodeName
+		ERRORCODE_13 = "13"
+		ERRORCODE_48 = "48"
 	)
 
 	feature := features.New("TestMultipleFatalEventRule").
@@ -55,82 +54,89 @@ func TestMultipleFatalEventRule(t *testing.T) {
 	feature.Assess("Inject multiple fatal errors", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
 		gpuNodes := ctx.Value(keyGpuNodes).([]string)
 		assert.True(t, len(gpuNodes) > 0, "no gpu nodes found")
-		GpuNodeName = gpuNodes[rand.Intn(len(gpuNodes))]
-		t.Logf("Injecting fatal events to node %s", GpuNodeName)
+		gpuNodeName := gpuNodes[rand.Intn(len(gpuNodes))]
+		ctx = context.WithValue(ctx, keyGpuNodeName, gpuNodeName)
+		t.Logf("Injecting fatal events to node %s", gpuNodeName)
 
 		// inject 5 fatal errors and let the remediation cycle finish
 
 		// inject XID 13 error
-		err := helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
+		err := helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
 		assert.NoError(t, err, "failed to send healthy events")
 		time.Sleep(5 * time.Second)
 
 		// inject XID 48 error
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_48, "data/fatal-health-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_48, "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_48, "data/healthy-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_48, "data/healthy-event.json")
 		assert.NoError(t, err, "failed to send healthy events")
 		time.Sleep(5 * time.Second)
 
 		// inject XID 13 error
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
 		assert.NoError(t, err, "failed to send healthy events")
 		time.Sleep(5 * time.Second)
 
 		// inject XID 48 error
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_48, "data/fatal-health-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_48, "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_48, "data/healthy-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_48, "data/healthy-event.json")
 		assert.NoError(t, err, "failed to send healthy events")
 		time.Sleep(5 * time.Second)
 
 		// inject XID 13 error
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
-		err = helpers.SendHealthEventsToNodes([]string{GpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
+		err = helpers.SendHealthEventsToNodes([]string{gpuNodeName}, ERRORCODE_13, "data/healthy-event.json")
 		assert.NoError(t, err, "failed to send healthy events")
 		time.Sleep(5 * time.Second)
 
 		return ctx
 	})
 
 	feature.Assess("Check if health event analyzer published a new fatal event", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
+		// Get GPU node name from context
+		gpuNodeName, ok := ctx.Value(keyGpuNodeName).(string)
+		if !ok || gpuNodeName == "" {
+			t.Fatal("GPU node name not found in context - previous assess step may have failed")
+		}
+
 		// Ensure cleanup at the end of the test
 		defer func() {
-			t.Logf("Starting cleanup for node %s", GpuNodeName)
+			t.Logf("Starting cleanup for node %s", gpuNodeName)
 
-			err := helpers.TestCleanUp(ctx, GpuNodeName, "MultipleFatalError", "31", c)
-			assert.NoError(t, err, "failed to cleanup node condition and uncordon node %s", GpuNodeName)
-			t.Logf("Successfully cleaned up node condition and uncordoned node %s", GpuNodeName)
+			err := helpers.TestCleanUp(ctx, gpuNodeName, "MultipleFatalError", "31", c)
+			assert.NoError(t, err, "failed to cleanup node condition and uncordon node %s", gpuNodeName)
+			t.Logf("Successfully cleaned up node condition and uncordoned node %s", gpuNodeName)
 		}()
 
-		// inject XID 48 error to trigger the rule
-		err := helpers.SendHealthEventsToNodes([]string{GpuNodeName}, "31", "data/fatal-health-event.json")
+		// inject XID 31 error to trigger the rule
+		err := helpers.SendHealthEventsToNodes([]string{gpuNodeName}, "31", "data/fatal-health-event.json")
 		assert.NoError(t, err, "failed to send fatal events")
 		time.Sleep(10 * time.Second)
 
 		client, err := c.NewClient()
 		assert.NoError(t, err, "failed to create client")
 
 		// Check node condition for matched ruleset
-		helpers.WaitForNodeConditionWithCheckName(ctx, t, client, GpuNodeName, "MultipleFatalError")
+		helpers.WaitForNodeConditionWithCheckName(ctx, t, client, gpuNodeName, "MultipleFatalError")
 		// Check MongoDB for health event with checkName = "MultipleFatalError"
 		filter := bson.M{
-			"healthevent.nodename":  GpuNodeName,
+			"healthevent.nodename":  gpuNodeName,
 			"healthevent.checkname": "MultipleFatalError",
 		}
 
@@ -142,7 +148,7 @@ func TestMultipleFatalEventRule(t *testing.T) {
 		if healthEvent, ok := event["healthevent"].(map[string]interface{}); ok {
 			nodeName, ok := healthEvent["nodename"].(string)
 			assert.True(t, ok, "nodename should be a string")
-			assert.Equal(t, GpuNodeName, nodeName, "nodeName should be the same as the node name")
+			assert.Equal(t, gpuNodeName, nodeName, "nodeName should be the same as the node name")
 
 			checkName, ok := healthEvent["checkname"].(string)
 			assert.True(t, ok, "checkname should be a string")