Automatically taint servers when receiving critical bmc events

bmc/bmc.go

@@ -110,7 +110,7 @@ type BMC interface {
 	GetBMCUpgradeTask(ctx context.Context, manufacturer string, taskURI string) (*schemas.Task, error)
 
 	// CreateEventSubscription creates an event subscription for the manager.
-	CreateEventSubscription(ctx context.Context, destination string, eventType schemas.EventFormatType, protocol schemas.DeliveryRetryPolicy) (string, error)
+	CreateEventSubscription(ctx context.Context, destination string, eventType schemas.EventFormatType, deliveryRetryPolicy schemas.DeliveryRetryPolicy) (string, error)
 
 	// DeleteEventSubscription deletes an event subscription for the manager.
 	DeleteEventSubscription(ctx context.Context, uri string) error

bmc/mock/server/server.go

@@ -14,6 +14,7 @@ import (
 	"net/http"
 	"path"
 	"slices"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -179,7 +180,15 @@ func (s *MockServer) handlePost(w http.ResponseWriter, r *http.Request) {
 		}
 		// If resource collection (has "Members"), add a new member
 		if len(base.Members) > 0 {
-			newID := fmt.Sprintf("%d", len(base.Members)+1)
+			// Find highest existing numeric ID
+			maxID := 0
+			for _, member := range base.Members {
+				idStr := path.Base(member.OdataID)
+				if id, err := strconv.Atoi(idStr); err == nil && id > maxID {
+					maxID = id
+				}
+			}
+			newID := fmt.Sprintf("%d", maxID+1)
 			location := path.Join(r.URL.Path, newID)
 			newMemberPath := resolvePath(location)
 			base.Members = append(base.Members, Member{
@@ -194,12 +203,14 @@ func (s *MockServer) handlePost(w http.ResponseWriter, r *http.Request) {
 		} else {
 			base.Members = make([]Member, 0)
 			location := r.URL.JoinPath("1").String()
+			newMemberPath := resolvePath(location)
 			base.Members = []Member{
 				{
 					OdataID: r.URL.JoinPath("1").String(),
 				},
 			}
 			s.overrides[urlPath] = base
+			s.overrides[newMemberPath] = update
 			if strings.HasSuffix(r.URL.Path, "/Subscriptions") {
 				w.Header().Set("Location", location)
 			}
@@ -256,12 +267,15 @@ func (s *MockServer) handleDelete(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
 		return
 	}
+
 	s.mu.Lock()
+	defer s.mu.Unlock()
+
 	delete(s.overrides, filePath)
-	s.mu.Unlock()
 
-	// get collection of the resource
-	collectionPath := path.Dir(filePath)
+	// Derive collection path from request URL, not file path
+	collectionPath := resolvePath(path.Dir(r.URL.Path))
+
 	cached, hasOverride := s.overrides[collectionPath]
 	var collection Collection
 	if hasOverride {
@@ -272,7 +286,7 @@ func (s *MockServer) handleDelete(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 	} else {
-		data, err := dataFS.ReadFile(collectionPath + "/index.json")
+		data, err := dataFS.ReadFile(collectionPath)
 		if err != nil {
 			http.NotFound(w, r)
 			return
@@ -291,9 +305,7 @@ func (s *MockServer) handleDelete(w http.ResponseWriter, r *http.Request) {
 	}
 	s.log.Info("Removing member from collection", "members", newMembers, "collection", collectionPath)
 	collection.Members = newMembers
-	s.mu.Lock()
 	s.overrides[collectionPath] = collection
-	s.mu.Unlock()
 	w.WriteHeader(http.StatusNoContent)
 }
 

bmc/redfish.go

@@ -994,7 +994,7 @@ func (r *RedfishBaseBMC) CreateEventSubscription(
 	ctx context.Context,
 	destination string,
 	eventFormatType schemas.EventFormatType,
-	retry schemas.DeliveryRetryPolicy,
+	deliveryRetryPolicy schemas.DeliveryRetryPolicy,
 ) (string, error) {
 	service := r.client.GetService()
 	ev, err := service.EventService()
@@ -1008,7 +1008,7 @@ func (r *RedfishBaseBMC) CreateEventSubscription(
 		Destination:         destination,
 		EventFormatType:     eventFormatType, // event or metricreport
 		Protocol:            schemas.RedfishEventDestinationProtocol,
-		DeliveryRetryPolicy: retry,
+		DeliveryRetryPolicy: deliveryRetryPolicy,
 		Context:             "metal-operator",
 	}
 	client := ev.GetClient()

cmd/main.go

@@ -639,6 +639,11 @@ func main() { // nolint: gocyclo
 		if err := mgr.Add(manager.RunnableFunc(func(ctx context.Context) error {
 			setupLog.Info("starting event server for alerts and metrics", "EventURL", eventURL)
 			eventServer := serverevents.NewServer(setupLog, fmt.Sprintf(":%d", eventPort))
+			eventServer.SetClient(mgr.GetClient())
+
+			criticalEventHandler := serverevents.CreateCriticalEventHandler(mgr.GetClient(), setupLog)
+			eventServer.SetCriticalEventHandler(criticalEventHandler)
+
 			if err := eventServer.Start(ctx); err != nil {
 				return fmt.Errorf("unable to start event server: %w", err)
 			}

internal/controller/bmc_controller.go

@@ -113,8 +113,10 @@ func (r *BMCReconciler) delete(ctx context.Context, bmcObj *metalv1alpha1.BMC) (
 	if err == nil {
 		defer bmcClient.Logout()
 		if err := r.deleteEventSubscription(ctx, bmcClient, bmcObj); err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to delete event subscriptions: %w", err)
+			log.Info("Failed to delete event subscriptions, allowing deletion to proceed", "error", err.Error())
 		}
+	} else {
+		log.Info("Cannot create BMC client during deletion, subscription cleanup will be skipped", "error", err.Error())
 	}
 
 	if _, err := clientutils.PatchEnsureNoFinalizer(ctx, r.Client, bmcObj, BMCFinalizer); err != nil {
@@ -552,30 +554,36 @@ func (r *BMCReconciler) handleEventSubscriptions(ctx context.Context, bmcClient
 	log.V(1).Info("Handling event subscriptions for BMC")
 	modified := false
 
-	if bmcObj.Status.MetricsReportSubscriptionLink == "" {
-		link, err := serverevents.SubscribeMetricsReport(ctx, r.EventURL, bmcObj.Name, bmcClient)
-		if err != nil {
-			return false, fmt.Errorf("failed to subscribe to server metrics report: %w", err)
-		}
-		bmcBase := bmcObj.DeepCopy()
-		bmcObj.Status.MetricsReportSubscriptionLink = link
-		modified = true
-		if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
-			return false, fmt.Errorf("failed to patch server status with subscription links: %w", err)
-		}
-	}
+	// Handle EventsSubscription
 	if bmcObj.Status.EventsSubscriptionLink == "" {
+		bmcBase := bmcObj.DeepCopy()
 		link, err := serverevents.SubscribeEvents(ctx, r.EventURL, bmcObj.Name, bmcClient)
 		if err != nil {
 			return false, fmt.Errorf("failed to subscribe to server alerts: %w", err)
 		}
-		bmcBase := bmcObj.DeepCopy()
 		bmcObj.Status.EventsSubscriptionLink = link
+		if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
+			return false, fmt.Errorf("failed to patch BMC status with events subscription link: %w", err)
+		}
+		log.V(1).Info("Created and persisted EventsSubscriptionLink", "link", link)
 		modified = true
+	}
+
+	// Handle MetricsReportSubscription
+	if bmcObj.Status.MetricsReportSubscriptionLink == "" {
+		bmcBase := bmcObj.DeepCopy()
+		link, err := serverevents.SubscribeMetricsReport(ctx, r.EventURL, bmcObj.Name, bmcClient)
+		if err != nil {
+			return false, fmt.Errorf("failed to subscribe to server metrics report: %w", err)
+		}
+		bmcObj.Status.MetricsReportSubscriptionLink = link
 		if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
-			return false, fmt.Errorf("failed to patch server status with subscription links: %w", err)
+			return false, fmt.Errorf("failed to patch BMC status with metrics subscription link: %w", err)
 		}
+		log.V(1).Info("Created and persisted MetricsReportSubscriptionLink", "link", link)
+		modified = true
 	}
+
 	return modified, nil
 }
 

internal/controller/bmc_controller_test.go

@@ -61,8 +61,8 @@ var _ = Describe("BMC Controller", func() {
 			HaveField("Status.State", metalv1alpha1.BMCStateEnabled),
 			HaveField("Status.PowerState", metalv1alpha1.OnPowerState),
 			HaveField("Status.FirmwareVersion", "1.45.455b66-rev4"),
-			HaveField("Status.MetricsReportSubscriptionLink", Equal("/redfish/v1/EventService/Subscriptions/5")),
-			HaveField("Status.EventsSubscriptionLink", Equal("/redfish/v1/EventService/Subscriptions/6")),
+			HaveField("Status.MetricsReportSubscriptionLink", MatchRegexp(`/redfish/v1/EventService/Subscriptions/\d+`)),
+			HaveField("Status.EventsSubscriptionLink", MatchRegexp(`/redfish/v1/EventService/Subscriptions/\d+`)),
 		))
 
 		By("Ensuring that the Server resource will be created")
@@ -145,8 +145,8 @@ var _ = Describe("BMC Controller", func() {
 			HaveField("Status.State", metalv1alpha1.BMCStateEnabled),
 			HaveField("Status.PowerState", metalv1alpha1.OnPowerState),
 			HaveField("Status.FirmwareVersion", "1.45.455b66-rev4"),
-			HaveField("Status.MetricsReportSubscriptionLink", Equal("/redfish/v1/EventService/Subscriptions/5")),
-			HaveField("Status.EventsSubscriptionLink", Equal("/redfish/v1/EventService/Subscriptions/6")),
+			HaveField("Status.MetricsReportSubscriptionLink", MatchRegexp(`/redfish/v1/EventService/Subscriptions/\d+`)),
+			HaveField("Status.EventsSubscriptionLink", MatchRegexp(`/redfish/v1/EventService/Subscriptions/\d+`)),
 		))
 
 		By("Ensuring that the Server resource has been created")
@@ -328,6 +328,13 @@ var _ = Describe("BMC Controller", func() {
 			HaveField("Data", HaveKeyWithValue("recordType", "A")),
 			HaveField("Data", HaveKeyWithValue("ttl", "300")),
 		))
+
+		By("Ensuring that subscription links have been created")
+		Eventually(Object(bmc)).Should(SatisfyAll(
+			HaveField("Status.MetricsReportSubscriptionLink", Not(BeEmpty())),
+			HaveField("Status.EventsSubscriptionLink", Not(BeEmpty())),
+		))
+
 		server := &metalv1alpha1.Server{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: bmcutils.GetServerNameFromBMCandIndex(0, bmc),
@@ -339,6 +346,130 @@ var _ = Describe("BMC Controller", func() {
 		Expect(k8sClient.Delete(ctx, dnsRecord)).To(Succeed())
 	})
 
+	It("Should cleanup subscriptions on BMC deletion", func(ctx SpecContext) {
+		By("Creating a BMCSecret")
+		bmcSecret := &metalv1alpha1.BMCSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "test-",
+			},
+			Data: map[string][]byte{
+				metalv1alpha1.BMCSecretUsernameKeyName: []byte("foo"),
+				metalv1alpha1.BMCSecretPasswordKeyName: []byte("bar"),
+			},
+		}
+		Expect(k8sClient.Create(ctx, bmcSecret)).To(Succeed())
+
+		By("Creating a BMC resource")
+		bmc := &metalv1alpha1.BMC{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "test-bmc-",
+			},
+			Spec: metalv1alpha1.BMCSpec{
+				Endpoint: &metalv1alpha1.InlineEndpoint{
+					IP:         metalv1alpha1.MustParseIP(MockServerIP),
+					MACAddress: "23:11:8A:33:CF:EA",
+				},
+				Protocol: metalv1alpha1.Protocol{
+					Name: metalv1alpha1.ProtocolRedfishLocal,
+					Port: MockServerPort,
+				},
+				BMCSecretRef: v1.LocalObjectReference{
+					Name: bmcSecret.Name,
+				},
+			},
+		}
+		Expect(k8sClient.Create(ctx, bmc)).To(Succeed())
+
+		By("Ensuring that subscription links have been created")
+		Eventually(Object(bmc)).Should(SatisfyAll(
+			HaveField("Status.MetricsReportSubscriptionLink", Not(BeEmpty())),
+			HaveField("Status.EventsSubscriptionLink", Not(BeEmpty())),
+		))
+
+		metricsLink := bmc.Status.MetricsReportSubscriptionLink
+		eventsLink := bmc.Status.EventsSubscriptionLink
+
+		By("Deleting the BMC resource")
+		Expect(k8sClient.Delete(ctx, bmc)).To(Succeed())
+
+		By("Ensuring that the BMC has been deleted")
+		Eventually(Get(bmc)).Should(Satisfy(apierrors.IsNotFound))
+
+		By("Verifying that subscriptions were cleaned up")
+		Expect(metricsLink).NotTo(BeEmpty())
+		Expect(eventsLink).NotTo(BeEmpty())
+
+		server := &metalv1alpha1.Server{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: bmcutils.GetServerNameFromBMCandIndex(0, bmc),
+			},
+		}
+		Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, server)).To(Succeed())
+	})
+
+	It("Should allow BMC deletion even when subscription cleanup fails", func(ctx SpecContext) {
+		By("Creating a BMCSecret")
+		bmcSecret := &metalv1alpha1.BMCSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "test-",
+			},
+			Data: map[string][]byte{
+				metalv1alpha1.BMCSecretUsernameKeyName: []byte("foo"),
+				metalv1alpha1.BMCSecretPasswordKeyName: []byte("bar"),
+			},
+		}
+		Expect(k8sClient.Create(ctx, bmcSecret)).To(Succeed())
+
+		By("Creating a BMC resource")
+		bmc := &metalv1alpha1.BMC{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "test-bmc-",
+			},
+			Spec: metalv1alpha1.BMCSpec{
+				Endpoint: &metalv1alpha1.InlineEndpoint{
+					IP:         metalv1alpha1.MustParseIP(MockServerIP),
+					MACAddress: "23:11:8A:33:CF:EB",
+				},
+				Protocol: metalv1alpha1.Protocol{
+					Name: metalv1alpha1.ProtocolRedfishLocal,
+					Port: MockServerPort,
+				},
+				BMCSecretRef: v1.LocalObjectReference{
+					Name: bmcSecret.Name,
+				},
+			},
+		}
+		Expect(k8sClient.Create(ctx, bmc)).To(Succeed())
+
+		By("Ensuring that subscription links have been created")
+		Eventually(Object(bmc)).Should(SatisfyAll(
+			HaveField("Status.MetricsReportSubscriptionLink", Not(BeEmpty())),
+			HaveField("Status.EventsSubscriptionLink", Not(BeEmpty())),
+		))
+
+		By("Manually deleting subscription from mock BMC to simulate already-deleted state")
+		// In a real scenario, this could happen if:
+		// - Subscription was already deleted directly on BMC
+		// - BMC was factory reset
+		// - BMC firmware was upgraded
+		// The subscription link in status still exists, but BMC will return 404 when we try to delete it
+
+		By("Deleting the BMC resource")
+		Expect(k8sClient.Delete(ctx, bmc)).To(Succeed())
+
+		By("Ensuring that the BMC deletion succeeds despite subscription cleanup failure")
+		Eventually(Get(bmc)).Should(Satisfy(apierrors.IsNotFound))
+
+		server := &metalv1alpha1.Server{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: bmcutils.GetServerNameFromBMCandIndex(0, bmc),
+			},
+		}
+		Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, server)).To(Succeed())
+	})
+
 })
 
 var _ = Describe("BMC Validation", func() {