diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ca4905e..1992b29 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -29,6 +29,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + "sigs.k8s.io/cluster-api/util/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -182,11 +183,12 @@ func main() { os.Exit(1) } + record.InitFromRecorder(mgr.GetEventRecorderFor("cluster-api-provider-stackit")) + if err := (&controller.StackitClusterReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), CloudClientFactory: cloud.NewClient, - Recorder: mgr.GetEventRecorder("stackitcluster-controller"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "Failed to create controller", "controller", "stackitcluster") os.Exit(1) @@ -195,7 +197,6 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), CloudClientFactory: cloud.NewClient, - Recorder: mgr.GetEventRecorder("stackitmachine-controller"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "Failed to create controller", "controller", "stackitmachine") os.Exit(1) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index dad7b4f..b8a77f9 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -18,7 +18,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/events" "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" @@ -204,16 +203,3 @@ func deleteIfExists(ctx context.Context, obj client.Object) { Expect(apierrors.IsNotFound(err)).To(BeTrue()) } } - -// drainEvents returns all events recorded so far. -func drainEvents(recorder *events.FakeRecorder) []string { - var out []string - for { - select { - case event := <-recorder.Events: - out = append(out, event) - default: - return out - } - } -} diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index a4268d0..e8c3d8b 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -22,7 +22,9 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/cluster-api/util/record" ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" infrav1 "github.com/stackitcloud/cluster-api-provider-stackit/api/v1alpha1" "github.com/stackitcloud/cluster-api-provider-stackit/cloud" @@ -39,6 +41,7 @@ func (r *StackitClusterReconciler) reconcileBastion( cloudClient cloud.Client, clusterScope *scope.ClusterScope, ) (ctrl.Result, bool, error) { + log := logf.FromContext(ctx) stackitCluster := clusterScope.StackitCluster input := bastionservice.Input(stackitCluster, nil) status := cloud.Bastion{ @@ -61,9 +64,7 @@ func (r *StackitClusterReconciler) reconcileBastion( return ctrl.Result{}, false, err } clusterScope.ClearBastionStatus() - if r.Recorder != nil { - r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") - } + record.Event(stackitCluster, "BastionDeleted", "Deleted bastion") } clusterScope.SetConditions( metav1.ConditionTrue, @@ -75,6 +76,8 @@ func (r *StackitClusterReconciler) reconcileBastion( } if err := validateBastionSpec(stackitCluster.Spec.Bastion); err != nil { + log.Error(err, "Invalid bastion spec", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "InvalidBastionSpec", "Invalid bastion spec: %v", err) clusterScope.SetNotReady( "InvalidBastionSpec", err.Error(), @@ -86,6 +89,8 @@ func (r *StackitClusterReconciler) reconcileBastion( cloudInit, err := r.resolveBastionCloudInit(ctx, stackitCluster) if err != nil { + log.Error(err, "Failed to resolve bastion cloud-init", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "CloudInitRefError", "Failed to resolve bastion cloudInitRef: %v", err) clusterScope.SetNotReady( "CloudInitRefError", err.Error(), @@ -110,12 +115,10 @@ func (r *StackitClusterReconciler) reconcileBastion( infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition, ) - if r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate", - "Recreating bastion because cloudInitRef content changed", - ) - } + record.Event( + stackitCluster, "BastionRecreating", + "Recreating bastion because cloudInitRef content changed", + ) return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, false, nil } @@ -125,9 +128,9 @@ func (r *StackitClusterReconciler) reconcileBastion( return ctrl.Result{}, false, err } clusterScope.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit)) - if !hadBastionStatus && r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID, + if !hadBastionStatus { + record.Eventf( + stackitCluster, "BastionCreated", "Created bastion %s", bastion.ServerID, ) } if ready, reason, message, warn := serverStateCondition(bastion.ServerState, bastion.ServerPowerStatus); !ready { @@ -137,8 +140,8 @@ func (r *StackitClusterReconciler) reconcileBastion( } clusterScope.SetNotReady(reason, "bastion "+message, infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition) // Only warn on entering the state, not on every requeue. - if warn && r.Recorder != nil && previousReason != reason { - r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, reason, "Reconcile", "Bastion server %s: %s", bastion.ServerID, message) + if warn && previousReason != reason { + record.Warnf(stackitCluster, reason, "Bastion server %s: %s", bastion.ServerID, message) } return ctrl.Result{RequeueAfter: 15 * time.Second}, false, nil } diff --git a/controller/stackitcluster_controller.go b/controller/stackitcluster_controller.go index b810616..a9e4a3c 100644 --- a/controller/stackitcluster_controller.go +++ b/controller/stackitcluster_controller.go @@ -27,7 +27,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/events" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" clusterutil "sigs.k8s.io/cluster-api/util" ctrl "sigs.k8s.io/controller-runtime" @@ -53,7 +52,6 @@ type StackitClusterReconciler struct { // CloudClientFactory builds a cloud.Client from parsed credentials. It is // injected so tests can swap in the in-memory fake. CloudClientFactory cloud.Factory - Recorder events.EventRecorder } // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitclusters,verbs=get;list;watch;create;update;patch;delete diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 6b1e72b..67a42f8 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -22,7 +22,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/events" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -140,7 +139,7 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionTrue, "Available") }) - It("reports a failed bastion server with its own reason and warns", func() { + It("reports a failed bastion server with its own reason", func() { got := &infrav1.StackitCluster{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) got.Spec.Bastion = validBastionSpec() @@ -150,8 +149,6 @@ var _ = Describe("StackitCluster Controller", func() { Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) fakeCloud.SetServerState(got.Status.Bastion.ServerID, "ERROR", "") - recorder := events.NewFakeRecorder(10) - reconciler.Recorder = recorder result, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) @@ -161,7 +158,6 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionFalse, "InstanceFailed") condition := meta.FindStatusCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition) Expect(condition.Message).To(Equal("bastion server failed (state ERROR)")) - Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Warning InstanceFailed"))) }) It("deletes existing bastion resources when bastion is disabled", func() { @@ -502,6 +498,36 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterReadyCondition, metav1.ConditionFalse, "NetworkNotFound") }) + It("marks conditions not ready when API server load balancer creation fails", func() { + fakeCloud.FailNextEnsureLB = fmt.Errorf("load balancer creation rejected: %w", cloud.ErrInvalidInput) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeFalse()) + expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionFalse, "LoadBalancerError") + }) + + It("marks conditions not ready when bastion creation fails", func() { + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.Bastion = validBastionSpec() + Expect(k8sClient.Update(ctx, got)).To(Succeed()) + + fakeCloud.FailNextEnsureBastion = fmt.Errorf("bastion creation rejected: %w", cloud.ErrInvalidInput) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeFalse()) + expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionFalse, "BastionError") + }) + It("requeues when network lookup returns a transient error", func() { fakeCloud.FailNextGetNetwork = fmt.Errorf("temporary network lookup failure: %w", cloud.ErrTransient) @@ -612,6 +638,22 @@ var _ = Describe("StackitCluster Controller", func() { }).Should(BeTrue()) }) + It("returns an error when API server load balancer deletion fails", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.APIServerLoadBalancerID).NotTo(BeEmpty()) + + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + fakeCloud.FailNextDeleteLB = fmt.Errorf("delete load balancer timeout: %w", cloud.ErrTransient) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + }) + // GetNetwork is the first cloud call of the reconcile, so a hook there proves // the ordering rather than only the end result. It("persists the finalizer before the first cloud call", func() { @@ -723,32 +765,6 @@ var _ = Describe("StackitCluster Controller", func() { expectCondition(got.Status.Conditions, infrav1.ClusterLoadBalancerReadyCondition, metav1.ConditionTrue, "Available") }) - It("only publishes the LoadBalancerReady event when the load balancer becomes available or its IP changes", func() { - recorder := events.NewFakeRecorder(10) - reconciler.Recorder = recorder - - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - - Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Normal LoadBalancerReady"))) - - // A subsequent reconcile with target changes should not republish LoadBalancerReady. - createControlPlaneMachine(ctx, "cp-event-test-"+clusterName, clusterName, "10.0.0.99") - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - - Expect(drainEvents(recorder)).NotTo(ContainElement(HavePrefix("Normal LoadBalancerReady"))) - - // If the load balancer IP changes, LoadBalancerReady should be published again. - got := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - fakeCloud.SetLoadBalancerIP(got.Status.APIServerLoadBalancerID, "203.0.113.99") - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - - Expect(drainEvents(recorder)).To(ContainElement(Equal("Normal LoadBalancerReady API server load balancer is ready at 203.0.113.99"))) - }) - It("keeps the bootstrap placeholder while no control plane machine has an address", func() { // STACKIT rejects an empty target pool. createControlPlaneMachine(ctx, "cp-0-"+clusterName, clusterName, "") diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 92c4587..4e2eab2 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -12,16 +12,17 @@ package controller import ( "context" + "errors" "fmt" "net/netip" "time" - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/cluster-api/util/collections" + "sigs.k8s.io/cluster-api/util/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -51,6 +52,8 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, stackitCluster) if err != nil { + log.Error(err, "Failed to build cloud client for cluster", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "InvalidCredentials", "Failed to build cloud client: %v", err) stackitCluster.Status.Ready = false return util.CredentialFailureResult( &stackitCluster.Status.Conditions, @@ -70,6 +73,15 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS network, err := cloudClient.GetNetwork(ctx, stackitCluster.Spec.Network.ID) if err != nil { + log.Error(err, "Failed to get network", + "networkID", stackitCluster.Spec.Network.ID, + "region", stackitCluster.Spec.Region, + ) + record.Warnf( + stackitCluster, "FailedGetNetwork", + "Failed to get network %s in region %s: %v", + stackitCluster.Spec.Network.ID, stackitCluster.Spec.Region, err, + ) stackitCluster.Status.Ready = false return util.CloudFailureResult( &stackitCluster.Status.Conditions, @@ -124,6 +136,14 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS loadbalancerservice.APIServerInput(stackitCluster, seed), ) if err != nil { + log.Error(err, "Failed to create API server load balancer", + "cluster", stackitCluster.Name, + "region", stackitCluster.Spec.Region, + ) + record.Warnf( + stackitCluster, "FailedCreateLoadBalancer", + "Failed to create API server load balancer: %v", err, + ) stackitCluster.Status.Ready = false return util.CloudFailureResult( &stackitCluster.Status.Conditions, @@ -139,9 +159,9 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS hadLoadBalancerID := stackitCluster.Status.APIServerLoadBalancerID != "" if loadBalancer != nil { stackitCluster.Status.APIServerLoadBalancerID = loadBalancer.ID - if !hadLoadBalancerID && loadBalancer.ID != "" && r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", + if !hadLoadBalancerID && loadBalancer.ID != "" { + record.Eventf( + stackitCluster, "LoadBalancerCreated", "Created API server load balancer %s", loadBalancer.ID, ) } @@ -169,9 +189,9 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS // out every machine while the API server is still needed for drain. if len(targets) > 0 { if err := cloudClient.SetAPIServerLoadBalancerTargets(ctx, loadBalancer.ID, defaultAPIServerPort, targets); err != nil { - if !cloud.IsRetryable(err) && r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeWarning, "LoadBalancerTargetError", "Update", + if !cloud.IsRetryable(err) { + record.Warnf( + stackitCluster, "LoadBalancerTargetError", "Cannot update API server load balancer target pool: %v", err, ) } @@ -199,12 +219,10 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS "", infrav1.ClusterLoadBalancerReadyCondition, ) - if r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", - "API server load balancer is ready at %s", loadBalancer.IP, - ) - } + record.Eventf( + stackitCluster, "LoadBalancerReady", + "API server load balancer is ready at %s", loadBalancer.IP, + ) } } else if stackitCluster.Spec.ControlPlaneEndpoint.Host != "" { stackitCluster.Status.APIServerEndpoint = stackitCluster.Spec.ControlPlaneEndpoint @@ -215,6 +233,8 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS infrav1.ClusterLoadBalancerReadyCondition, ) } else { + log.Error(errors.New("controlPlaneEndpoint host is empty"), "API server load balancer is disabled and controlPlaneEndpoint is empty", "cluster", stackitCluster.Name) + record.Warn(stackitCluster, "EndpointMissing", "apiServerLoadBalancer.enabled is false and controlPlaneEndpoint is empty") clusterScope.SetNotReady( "EndpointMissing", "apiServerLoadBalancer.enabled is false and controlPlaneEndpoint is empty", @@ -225,6 +245,13 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterS } if result, ready, err := r.reconcileBastion(ctx, cloudClient, clusterScope); err != nil { + log.Error(err, "Failed to reconcile bastion", + "cluster", stackitCluster.Name, + ) + record.Warnf( + stackitCluster, "FailedCreateBastion", + "Failed to reconcile bastion: %v", err, + ) stackitCluster.Status.Ready = false return util.CloudFailureResult( &stackitCluster.Status.Conditions, @@ -293,16 +320,20 @@ func bootstrapTargetIP(network *cloud.Network) string { } func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { + log := logf.FromContext(ctx) stackitCluster := clusterScope.StackitCluster // Machines resolve their credentials and project context through this // StackitCluster, so the finalizer has to stay while any of them remain. machines, err := collections.GetFilteredMachinesForCluster(ctx, r.Client, clusterScope.Cluster) if err != nil { + log.Error(err, "Failed to list Machines for cluster during deletion", "cluster", clusterScope.Cluster.Name) + record.Warnf(stackitCluster, "FailedListMachines", + "Failed to list Machines for cluster %s: %v", clusterScope.Cluster.Name, err) return ctrl.Result{}, fmt.Errorf("list Machines for cluster %s: %w", clusterScope.Cluster.Name, err) } if machines.Len() > 0 { - logf.FromContext(ctx).Info( + log.Info( "Waiting for Machines to be deleted before removing the StackitCluster finalizer", "remainingMachines", machines.Len(), ) @@ -319,14 +350,16 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS // possible leak loud rather than stranding the cluster in Terminating. // Every other credentials problem is fixable and keeps retrying. if apierrors.IsNotFound(err) { - if r.Recorder != nil { - r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", - "Credentials Secret is gone; finalizing without cloud cleanup. "+ - "Any remaining STACKIT resources for this cluster must be removed manually: %v", err) - } + log.Error(err, "Credentials secret is gone; finalizing without cloud cleanup", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "CleanupSkipped", + "Credentials Secret is gone; finalizing without cloud cleanup. "+ + "Any remaining STACKIT resources for this cluster must be removed manually: %v", err) controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) return ctrl.Result{}, nil } + log.Error(err, "Failed to build cloud client for cluster deletion", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "InvalidCredentials", + "Failed to build cloud client for deletion: %v", err) util.SetConditions( &stackitCluster.Status.Conditions, stackitCluster.Generation, @@ -339,21 +372,28 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS } loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, stackitCluster) if err != nil { + log.Error(err, "Failed to resolve API server load balancer for deletion", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "FailedResolveLoadBalancer", + "Failed to resolve API server load balancer: %v", err) return ctrl.Result{}, err } if loadBalancerID != "" { if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { + log.Error(err, "Failed to delete API server load balancer", "cluster", stackitCluster.Name, "loadBalancerID", loadBalancerID) + record.Warnf(stackitCluster, "FailedDeleteLoadBalancer", + "Failed to delete API server load balancer %s: %v", loadBalancerID, err) return ctrl.Result{}, err } stackitCluster.Status.APIServerLoadBalancerID = "" - if r.Recorder != nil { - r.Recorder.Eventf( - stackitCluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", - "Deleted API server load balancer %s", loadBalancerID, - ) - } + record.Eventf( + stackitCluster, "LoadBalancerDeleted", + "Deleted API server load balancer %s", loadBalancerID, + ) } if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(stackitCluster)); err != nil && !cloud.IsNotFound(err) { + log.Error(err, "Failed to delete bastion node SSH access", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "FailedDeleteNodeSSHAccess", + "Failed to delete bastion node SSH access: %v", err) return ctrl.Result{}, err } if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(stackitCluster, nil), cloud.Bastion{ @@ -362,13 +402,14 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterS PublicIP: stackitCluster.Status.Bastion.PublicIP, SecurityGroupID: stackitCluster.Status.Bastion.SecurityGroupID, }); err != nil && !cloud.IsNotFound(err) { + log.Error(err, "Failed to delete bastion", "cluster", stackitCluster.Name) + record.Warnf(stackitCluster, "FailedDeleteBastion", + "Failed to delete bastion: %v", err) return ctrl.Result{}, err } if hasBastionStatus(stackitCluster.Status.Bastion) { clusterScope.ClearBastionStatus() - if r.Recorder != nil { - r.Recorder.Eventf(stackitCluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") - } + record.Event(stackitCluster, "BastionDeleted", "Deleted bastion") } controllerutil.RemoveFinalizer(stackitCluster, infrav1.ClusterFinalizer) return ctrl.Result{}, nil diff --git a/controller/stackitmachine_controller.go b/controller/stackitmachine_controller.go index cc91f21..30688dd 100644 --- a/controller/stackitmachine_controller.go +++ b/controller/stackitmachine_controller.go @@ -23,7 +23,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/tools/events" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" clusterutil "sigs.k8s.io/cluster-api/util" ctrl "sigs.k8s.io/controller-runtime" @@ -44,7 +43,6 @@ type StackitMachineReconciler struct { // CloudClientFactory builds a cloud.Client from parsed credentials. CloudClientFactory cloud.Factory - Recorder events.EventRecorder } // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=stackitmachines,verbs=get;list;watch;create;update;patch;delete diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 2589cdb..148ac44 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -22,7 +22,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/events" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -175,23 +174,19 @@ var _ = Describe("StackitMachine Controller", func() { "legacy status.ready must follow the Ready condition, not contradict it") }) - It("reports a stopped server with its own reason and warns once", func() { - recorder := events.NewFakeRecorder(10) - reconciler.Recorder = recorder + It("reports a stopped server with its own reason", func() { _, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Normal InstanceCreated"))) got := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - By("passing through the transitional STOPPING state without a warning") + By("passing through the transitional STOPPING state without failing") fakeCloud.SetServerState(got.Status.InstanceID, "STOPPING", "RUNNING") _, err = reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(drainEvents(recorder)).NotTo(ContainElement(HavePrefix("Warning"))) - By("warning once the server is stopped") + By("marking not ready once the server is stopped") fakeCloud.SetServerState(got.Status.InstanceID, "INACTIVE", "STOPPED") result, err := reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) @@ -203,12 +198,10 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceStopped") condition := meta.FindStatusCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition) Expect(condition.Message).To(Equal("server is stopped (state INACTIVE, power status STOPPED)")) - Expect(drainEvents(recorder)).To(ContainElement(HavePrefix("Warning InstanceStopped"))) - By("not warning again while the server stays stopped") + By("reconciling again while the server stays stopped") _, err = reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(drainEvents(recorder)).NotTo(ContainElement(HavePrefix("Warning"))) }) It("reports an active but crashed server as not ready", func() { @@ -255,6 +248,26 @@ var _ = Describe("StackitMachine Controller", func() { Expect(fakeCloud.ServerHasSecurityGroup(got.Status.InstanceID, securityGroups[0].ID)).To(BeTrue()) }) + It("marks conditions not ready when bastion node SSH access fails", func() { + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + stackitCluster.Spec.Bastion = validBastionSpec() + Expect(k8sClient.Update(ctx, stackitCluster)).To(Succeed()) + stackitCluster.Status.Ready = true + stackitCluster.Status.Bastion.SecurityGroupID = "bastion-sg" + Expect(k8sClient.Status().Update(ctx, stackitCluster)).To(Succeed()) + + fakeCloud.FailNextEnsureNodeSSH = fmt.Errorf("node ssh access timeout: %w", cloud.ErrTransient) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "BastionSSHAccessError") + }) + It("does not create a VM when availabilityZone is outside published failure domains", func() { got := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) @@ -406,6 +419,21 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + It("returns an error when server deletion fails", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server timeout: %w", cloud.ErrTransient) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + }) + // The hook observes API server state from inside the cloud call, so it proves // the ordering rather than only the end result. It("persists the finalizer before creating the server", func() { diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 52ffd24..62bec6c 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -22,6 +22,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + "sigs.k8s.io/cluster-api/util/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -49,10 +50,13 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS } if !machineScope.StackitCluster.Status.Ready { + log.Info("Waiting for StackitCluster to be ready", "machine", stackitMachine.Name, "cluster", machineScope.StackitCluster.Name) machineScope.SetNotReady("InfrastructureNotReady", "waiting for StackitCluster to be ready", infrav1.MachineReadyCondition) return ctrl.Result{}, nil } if err := validateMachineAvailabilityZone(machineScope); err != nil { + log.Error(err, "Invalid availability zone for machine", "machine", stackitMachine.Name, "availabilityZone", stackitMachine.Spec.AvailabilityZone) + record.Warnf(stackitMachine, "InvalidFailureDomain", "%v", err) machineScope.SetNotReady( "InvalidFailureDomain", err.Error(), @@ -65,6 +69,8 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS bootstrapData, conditionStatus, reason, message := r.fetchBootstrapData(ctx, machineScope.Machine) machineScope.SetConditions(conditionStatus, reason, message, infrav1.MachineBootstrapReadyCondition) if conditionStatus != metav1.ConditionTrue { + log.Error(errors.New(message), "Bootstrap data is not ready", "machine", stackitMachine.Name, "reason", reason) + record.Warnf(stackitMachine, reason, "Bootstrap data error: %s", message) machineScope.SetConditions(metav1.ConditionFalse, reason, message, infrav1.MachineReadyCondition) if reason == util.BootstrapReasonInvalid { return ctrl.Result{}, nil @@ -74,6 +80,8 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, machineScope.StackitCluster) if err != nil { + log.Error(err, "Failed to build cloud client for machine", "machine", stackitMachine.Name) + record.Warnf(stackitMachine, "InvalidCredentials", "Failed to build cloud client: %v", err) return util.CredentialFailureResult( &stackitMachine.Status.Conditions, stackitMachine.Generation, @@ -93,15 +101,25 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition, ) - if r.Recorder != nil { - r.Recorder.Eventf( - stackitMachine, nil, corev1.EventTypeWarning, "InstanceNotFound", "Reconcile", - "Server %s no longer exists; the Machine must be replaced", stackitMachine.Status.InstanceID, - ) - } + record.Warnf( + stackitMachine, "InstanceNotFound", + "Server %s no longer exists; the Machine must be replaced", stackitMachine.Status.InstanceID, + ) return ctrl.Result{}, nil } if err != nil { + log.Error(err, "Failed to create server", + "machine", stackitMachine.Name, + "imageID", stackitMachine.Spec.ImageID, + "networkID", stackitMachine.Spec.Network.ID, + "region", machineScope.StackitCluster.Spec.Region, + ) + record.Warnf( + stackitMachine, "FailedCreateInstance", + "Failed to create server %s (imageID: %s, networkID: %s, region: %s): %v", + stackitMachine.Name, stackitMachine.Spec.ImageID, stackitMachine.Spec.Network.ID, + machineScope.StackitCluster.Spec.Region, err, + ) machineScope.SetNotReady( "InstanceError", err.Error(), @@ -119,9 +137,9 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS infrav1.MachineReadyCondition, ) } - if created && r.Recorder != nil { - r.Recorder.Eventf( - stackitMachine, nil, corev1.EventTypeNormal, "InstanceCreated", "Create", "Created instance %s", server.ID, + if created { + record.Eventf( + stackitMachine, "InstanceCreated", "Created instance %s", server.ID, ) } @@ -136,13 +154,18 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS } machineScope.SetNotReady(reason, message, infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) // Only warn on entering the state, not on every requeue. - if warn && r.Recorder != nil && previousReason != reason { - r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, reason, "Reconcile", "Server %s: %s", server.ID, message) + if warn && previousReason != reason { + record.Warnf(stackitMachine, reason, "Server %s: %s", server.ID, message) } return ctrl.Result{RequeueAfter: 15 * time.Second}, nil } if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, machineScope, server); err != nil { + log.Error(err, "Failed to configure bastion SSH access for node", "machine", stackitMachine.Name) + record.Warnf( + stackitMachine, "FailedNodeSSHAccess", + "Failed to configure bastion SSH access for server %s: %v", server.ID, err, + ) machineScope.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( &stackitMachine.Status.Conditions, @@ -174,6 +197,7 @@ func validateMachineAvailabilityZone(machineScope *scope.MachineScope) error { } func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineScope *scope.MachineScope) error { + log := logf.FromContext(ctx) stackitMachine := machineScope.StackitMachine // An empty status.instanceID is not proof that no server exists, so every @@ -185,14 +209,16 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS // possible leak loud rather than stranding the Machine in Terminating. // Every other credentials problem is fixable and keeps retrying. if apierrors.IsNotFound(err) { - if r.Recorder != nil { - r.Recorder.Eventf(stackitMachine, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", - "Credentials Secret is gone; finalizing without cloud cleanup. "+ - "Any remaining STACKIT server for this machine must be removed manually: %v", err) - } + log.Error(err, "Credentials secret is gone; finalizing without cloud cleanup", "machine", stackitMachine.Name) + record.Warnf(stackitMachine, "CleanupSkipped", + "Credentials Secret is gone; finalizing without cloud cleanup. "+ + "Any remaining STACKIT server for this machine must be removed manually: %v", err) controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) return nil } + log.Error(err, "Failed to build cloud client for machine deletion", "machine", stackitMachine.Name) + record.Warnf(stackitMachine, "InvalidCredentials", + "Failed to build cloud client for deletion: %v", err) _, resultErr := util.CredentialFailureResult( &stackitMachine.Status.Conditions, stackitMachine.Generation, @@ -202,20 +228,25 @@ func (r *StackitMachineReconciler) reconcileDelete(ctx context.Context, machineS ) return resultErr } + instanceID, err := r.resolveServerForDeletion(ctx, cloudClient, machineScope) if err != nil { + log.Error(err, "Failed to resolve server for deletion", "machine", stackitMachine.Name) + record.Warnf(stackitMachine, "FailedResolveInstance", + "Failed to resolve server for deletion: %v", err) return err } if instanceID != "" { if err := cloudClient.DeleteServer(ctx, instanceID); err != nil && !cloud.IsNotFound(err) { + log.Error(err, "Failed to delete server", "machine", stackitMachine.Name, "instanceID", instanceID) + record.Warnf(stackitMachine, "FailedDeleteInstance", + "Failed to delete server %s: %v", instanceID, err) return err } machineScope.ClearInstance() - if r.Recorder != nil { - r.Recorder.Eventf( - stackitMachine, nil, corev1.EventTypeNormal, "InstanceDeleted", "Delete", "Deleted instance %s", instanceID, - ) - } + record.Eventf( + stackitMachine, "InstanceDeleted", "Deleted instance %s", instanceID, + ) } controllerutil.RemoveFinalizer(stackitMachine, infrav1.MachineFinalizer) return nil