From 9326b5d2ccdd30e396b34b4008c951707b5031ef Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 1 Oct 2024 11:48:19 +0200 Subject: [PATCH 1/4] Machine: ignore attached Volumes referred by pods ignored during drain --- internal/controllers/machine/drain/drain.go | 25 +- internal/controllers/machine/drain/filters.go | 12 + .../controllers/machine/machine_controller.go | 290 ++++++++++++++++-- .../machine/machine_controller_test.go | 246 ++++++++++++++- main.go | 5 + util/log/log.go | 35 ++- 6 files changed, 552 insertions(+), 61 deletions(-) diff --git a/internal/controllers/machine/drain/drain.go b/internal/controllers/machine/drain/drain.go index 5b9d7d3949e9..7ff519e000c7 100644 --- a/internal/controllers/machine/drain/drain.go +++ b/internal/controllers/machine/drain/drain.go @@ -34,6 +34,8 @@ import ( "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + + clog "sigs.k8s.io/cluster-api/util/log" ) // Helper contains the parameters to control the behaviour of the drain helper. @@ -351,33 +353,14 @@ func (r EvictionResult) ConditionMessage(nodeDrainStartTime *metav1.Time) string // podDeleteListToString returns a comma-separated list of the first n entries of the PodDelete list. func podDeleteListToString(podList []PodDelete, n int) string { - return listToString(podList, func(pd PodDelete) string { + return clog.ListToString(podList, func(pd PodDelete) string { return klog.KObj(pd.Pod).String() }, n) } // PodListToString returns a comma-separated list of the first n entries of the Pod list. func PodListToString(podList []*corev1.Pod, n int) string { - return listToString(podList, func(p *corev1.Pod) string { + return clog.ListToString(podList, func(p *corev1.Pod) string { return klog.KObj(p).String() }, n) } - -// listToString returns a comma-separated list of the first n entries of the list (strings are calculated via stringFunc). -func listToString[T any](list []T, stringFunc func(T) string, n int) string { - shortenedBy := 0 - if len(list) > n { - shortenedBy = len(list) - n - list = list[:n] - } - stringList := []string{} - for _, p := range list { - stringList = append(stringList, stringFunc(p)) - } - - if shortenedBy > 0 { - stringList = append(stringList, fmt.Sprintf("... (%d more)", shortenedBy)) - } - - return strings.Join(stringList, ", ") -} diff --git a/internal/controllers/machine/drain/filters.go b/internal/controllers/machine/drain/filters.go index 1851bd453104..bab2ff5161f5 100644 --- a/internal/controllers/machine/drain/filters.go +++ b/internal/controllers/machine/drain/filters.go @@ -62,6 +62,18 @@ func (l *PodDeleteList) Pods() []*corev1.Pod { return pods } +// IgnoredPods returns a list of Pods that have to be ignored before the Node can be considered completely drained. +// Note: As of today only Pods from DaemonSet, static Pods or if `SkipWaitForDeleteTimeoutSeconds` is set Pods in deletion get ignored. +func (l *PodDeleteList) IgnoredPods() []*corev1.Pod { + pods := []*corev1.Pod{} + for _, i := range l.items { + if !i.Status.Delete { + pods = append(pods, i.Pod) + } + } + return pods +} + func (l *PodDeleteList) errors() []error { failedPods := make(map[string][]string) for _, i := range l.items { diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 9babad1c13a3..b4f3518f2693 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -19,16 +19,19 @@ package machine import ( "context" "fmt" + "slices" "strings" "time" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -61,7 +64,8 @@ import ( ) const ( - drainRetryInterval = time.Duration(20) * time.Second + drainRetryInterval = time.Duration(20) * time.Second + waitForVolumeDetachRetryInterval = time.Duration(20) * time.Second ) var ( @@ -518,14 +522,15 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) (ctrl.Result s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason s.deletingMessage = fmt.Sprintf("Waiting for Node volumes to be detached (started at %s)", m.Status.Deletion.WaitForNodeVolumeDetachStartTime.Format(time.RFC3339)) - if ok, err := r.shouldWaitForNodeVolumes(ctx, s, m.Status.NodeRef.Name); ok || err != nil { - if err != nil { - s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason - s.deletingMessage = "Error waiting for volumes to be detached from Node, please check controller logs for errors" - r.recorder.Eventf(m, corev1.EventTypeWarning, "FailedWaitForVolumeDetach", "error waiting for node volumes detaching, Machine's node %q: %v", m.Status.NodeRef.Name, err) - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + result, err := r.shouldWaitForNodeVolumes(ctx, s) + if err != nil { + s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason + s.deletingMessage = "Error waiting for volumes to be detached from Node, please check controller logs for errors" + r.recorder.Eventf(m, corev1.EventTypeWarning, "FailedWaitForVolumeDetach", "error waiting for node volumes detaching, Machine's node %q: %v", m.Status.NodeRef.Name, err) + return ctrl.Result{}, err + } + if !result.IsZero() { + return result, nil } conditions.MarkTrue(m, clusterv1.VolumeDetachSucceededCondition) r.recorder.Eventf(m, corev1.EventTypeNormal, "NodeVolumesDetached", "success waiting for node volumes detaching Machine's node %q", m.Status.NodeRef.Name) @@ -783,7 +788,7 @@ func (r *Reconciler) drainNode(ctx context.Context, s *scope) (ctrl.Result, erro if err := remoteClient.Get(ctx, client.ObjectKey{Name: nodeName}, node); err != nil { if apierrors.IsNotFound(err) { // If an admin deletes the node directly, we'll end up here. - log.Error(err, "Could not find node from noderef, it may have already been deleted") + log.Info("Could not find Node from Machine.status.nodeRef, skip waiting for volume detachment.") return ctrl.Result{}, nil } return ctrl.Result{}, errors.Wrapf(err, "unable to get Node %s", nodeName) @@ -866,7 +871,8 @@ func (r *Reconciler) drainNode(ctx context.Context, s *scope) (ctrl.Result, erro // this could cause issue for some storage provisioner, for example, vsphere-volume this is problematic // because if the node is deleted before detach success, then the underline VMDK will be deleted together with the Machine // so after node draining we need to check if all volumes are detached before deleting the node. -func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope, nodeName string) (bool, error) { +func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope) (ctrl.Result, error) { + nodeName := s.machine.Status.NodeRef.Name log := ctrl.LoggerFrom(ctx, "Node", klog.KRef("", nodeName)) ctx = ctrl.LoggerInto(ctx, log) @@ -875,16 +881,16 @@ func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope, nod remoteClient, err := r.ClusterCache.GetClient(ctx, util.ObjectKey(cluster)) if err != nil { - return true, err + return ctrl.Result{}, err } node := &corev1.Node{} if err := remoteClient.Get(ctx, types.NamespacedName{Name: nodeName}, node); err != nil { if apierrors.IsNotFound(err) { log.Error(err, "Could not find node from noderef, it may have already been deleted") - return false, nil + return ctrl.Result{}, nil } - return true, err + return ctrl.Result{}, err } if noderefutil.IsNodeUnreachable(node) { @@ -892,17 +898,49 @@ func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope, nod // We need to skip the detachment as we otherwise block deletions // of unreachable nodes when a volume is attached. log.Info("Node is unreachable, skip waiting for volume detachment.") - return false, nil + return ctrl.Result{}, nil } - if len(node.Status.VolumesAttached) != 0 { - log.Info("Waiting for Node volumes to be detached") - s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason - s.deletingMessage = fmt.Sprintf("Waiting for Node volumes to be detached (started at %s)", machine.Status.Deletion.WaitForNodeVolumeDetachStartTime.Format(time.RFC3339)) - return true, nil + // Get two sets of information about volumes currently attached to the node: + // * VolumesAttached names from node.Status.VolumesAttached + // * PersistentVolume names from VolumeAttachments with status.Attached set to true + attachedNodeVolumeNames, attachedPVNames, err := getAttachedVolumeInformation(ctx, remoteClient, node) + if err != nil { + return ctrl.Result{}, err } - return false, nil + // Return early if there are no volumes to wait for getting detached. + if len(attachedNodeVolumeNames) == 0 && len(attachedPVNames) == 0 { + return ctrl.Result{}, nil + } + + // Get all PVCs we want to ignore because they belong to Pods for which we skipped drain. + pvcsToIgnoreFromPods, err := getPersistentVolumeClaimsToIgnore(ctx, remoteClient, nodeName) + if err != nil { + return ctrl.Result{}, err + } + + // List all PersistentVolumes and return the ones we want to wait for. + attachedVolumeInformation, err := getPersistentVolumesWaitingForDetach(ctx, remoteClient, attachedNodeVolumeNames, attachedPVNames, pvcsToIgnoreFromPods) + if err != nil { + return ctrl.Result{}, err + } + + // If no pvcs were found we have to wait for and there is no unmatched information, then we are finished with waiting for volumes. + if attachedVolumeInformation.isEmpty() { + return ctrl.Result{}, nil + } + + // Add entry to the reconcileDeleteCache so we won't retry shouldWaitForNodeVolumes again before waitForVolumeDetachRetryInterval. + r.reconcileDeleteCache.Add(cache.NewReconcileEntry(machine, time.Now().Add(waitForVolumeDetachRetryInterval))) + + s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason + s.deletingMessage = fmt.Sprintf("Waiting for Node volumes to be detached (started at %s)", machine.Status.Deletion.WaitForNodeVolumeDetachStartTime.Format(time.RFC3339)) + + log.Info("Waiting for Node volumes to be detached", + attachedVolumeInformation.logKeys()..., + ) + return ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, nil } func (r *Reconciler) deleteNode(ctx context.Context, cluster *clusterv1.Cluster, name string) error { @@ -1064,3 +1102,213 @@ func (r *Reconciler) nodeToMachine(ctx context.Context, o client.Object) []recon return nil } + +// getAttachedVolumeInformation returns information about volumes attached to the node: +// * VolumesAttached names from node.Status.VolumesAttached. +// * PersistentVolume names from VolumeAttachments with status.Attached set to true. +func getAttachedVolumeInformation(ctx context.Context, remoteClient client.Client, node *corev1.Node) (sets.Set[string], sets.Set[string], error) { + attachedVolumeName := sets.Set[string]{} + attachedPVNames := sets.Set[string]{} + + for _, attachedVolume := range node.Status.VolumesAttached { + attachedVolumeName.Insert(string(attachedVolume.Name)) + } + + volumeAttachments, err := getVolumeAttachmentForNode(ctx, remoteClient, node.GetName()) + if err != nil { + return nil, nil, errors.Wrap(err, "failed to list VolumeAttachments") + } + + for _, va := range volumeAttachments { + // Return an error if a VolumeAttachments does not refer a PersistentVolume. + if va.Spec.Source.PersistentVolumeName == nil { + return nil, nil, errors.Errorf("spec.source.persistentVolumeName for VolumeAttachment %s is not set", va.GetName()) + } + attachedPVNames.Insert(*va.Spec.Source.PersistentVolumeName) + } + + return attachedVolumeName, attachedPVNames, nil +} + +// getPersistentVolumeClaimsToIgnore gets all pods which have been ignored by drain and returns a list of +// NamespacedNames for all PersistentVolumeClaims referred by the pods. +// Note: this does not require us to list PVC's directly. +func getPersistentVolumeClaimsToIgnore(ctx context.Context, remoteClient client.Client, nodeName string) (sets.Set[string], error) { + drainHelper := drain.Helper{ + Client: remoteClient, + } + + pods, err := drainHelper.GetPodsForEviction(ctx, nodeName) + if err != nil { + return nil, errors.Wrap(err, "failed to find PersistentVolumeClaims from Pods ignored during drain") + } + + ignoredPods := pods.IgnoredPods() + + pvcsToIgnore := sets.Set[string]{} + + for _, pod := range ignoredPods { + for _, volume := range pod.Spec.Volumes { + if volume.PersistentVolumeClaim == nil { + continue + } + + key := types.NamespacedName{Namespace: pod.GetNamespace(), Name: volume.PersistentVolumeClaim.ClaimName}.String() + pvcsToIgnore.Insert(key) + } + } + + return pvcsToIgnore, nil +} + +// getVolumeAttachmentForNode does a paged list of VolumeAttachments and returns a list +// of VolumeAttachments attached to the given node. +func getVolumeAttachmentForNode(ctx context.Context, c client.Client, nodeName string) ([]*storagev1.VolumeAttachment, error) { + volumeAttachments := []*storagev1.VolumeAttachment{} + volumeAttachmentList := &storagev1.VolumeAttachmentList{} + for { + listOpts := []client.ListOption{ + client.Continue(volumeAttachmentList.GetContinue()), + client.Limit(100), + } + if err := c.List(ctx, volumeAttachmentList, listOpts...); err != nil { + return nil, errors.Wrap(err, "failed to list VolumeAttachments") + } + + for _, volumeAttachment := range volumeAttachmentList.Items { + // Skip VolumeAttachments which are not in attached state. + if !volumeAttachment.Status.Attached { + continue + } + // Skip VolumeAttachments which are not for the given node. + if volumeAttachment.Spec.NodeName != nodeName { + continue + } + volumeAttachments = append(volumeAttachments, &volumeAttachment) + } + + if volumeAttachmentList.GetContinue() == "" { + break + } + } + + return volumeAttachments, nil +} + +type attachedVolumeInformation struct { + // Attached PersistentVolumeClaims. + persistentVolumeClaims []string + + // Attached PersistentVolumes without a corresponding PersistentVolumeClaim. + persistentVolumesWithoutPVCClaimRef []string + + // Entries in Node.Status.AttachedVolumes[].Name without a corresponding PersistentVolume. + nodeStatusVolumeNamesWithoutPV []string + // Names of PersistentVolumes from VolumeAttachments which don't have a corresponding PersistentVolume. + persistentVolumeNamesWithoutPV []string +} + +func (a *attachedVolumeInformation) isEmpty() bool { + return len(a.persistentVolumeClaims) == 0 && + len(a.persistentVolumesWithoutPVCClaimRef) == 0 && + len(a.nodeStatusVolumeNamesWithoutPV) == 0 && + len(a.persistentVolumeNamesWithoutPV) == 0 +} + +func (a *attachedVolumeInformation) logKeys() []any { + logKeys := []any{} + if len(a.persistentVolumeClaims) > 0 { + slices.Sort(a.persistentVolumeClaims) + logKeys = append(logKeys, "PersistentVolumeClaims", clog.StringListToString(a.persistentVolumeClaims)) + } + + if len(a.persistentVolumesWithoutPVCClaimRef) > 0 { + slices.Sort(a.persistentVolumesWithoutPVCClaimRef) + logKeys = append(logKeys, "PersistentVolumesWithoutPVCClaimRef", clog.StringListToString(a.persistentVolumesWithoutPVCClaimRef)) + } + + if len(a.nodeStatusVolumeNamesWithoutPV) > 0 { + slices.Sort(a.nodeStatusVolumeNamesWithoutPV) + logKeys = append(logKeys, "NodeStatusVolumeNamesWithoutPV", clog.StringListToString(a.nodeStatusVolumeNamesWithoutPV)) + } + + if len(a.persistentVolumeNamesWithoutPV) > 0 { + slices.Sort(a.persistentVolumeNamesWithoutPV) + logKeys = append(logKeys, "PersistentVolumeNamesWithoutPV", clog.StringListToString(a.persistentVolumeNamesWithoutPV)) + } + + return logKeys +} + +// getPersistentVolumesWaitingForDetach returns a list of all persistentVolumes which either have +// the calculated AttachedVolume name in attachedVolumeNames or their name in attachedPVNames. +// PersistentVolumes which refer a PersistentVolumeClaim contained in pvcsToIgnore are filtered out. +// If there are names in attachedVolumeNames or attachedPVNames without a corresponding PV, this returns an error. +func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, attachedNodeVolumeNames, attachedPVNames, pvcsToIgnore sets.Set[string]) (*attachedVolumeInformation, error) { + pvcsWaitingForDetach := sets.Set[string]{} + persistentVolumesWithoutPVCClaimRef := []string{} + foundAttachedVolumeNames := sets.Set[string]{} + foundAttachedPVNames := sets.Set[string]{} + + // List all PersistentVolumes and preserve the ones we have to wait for. + // Also store the found VolumeHandles and names of PersistentVolumes to check + // that all expected PersistentVolumes have been found. + // Note: pvsWaitingFor will not include PersistentVolumes we want to ignore. + // These are usually volumes which have a Pod which is expected to be kept. + persistentVolumeList := &corev1.PersistentVolumeList{} + for { + listOpts := []client.ListOption{ + client.Continue(persistentVolumeList.GetContinue()), + client.Limit(100), + } + if err := c.List(ctx, persistentVolumeList, listOpts...); err != nil { + return nil, errors.Wrap(err, "failed to list PersistentVolumes") + } + + for _, persistentVolume := range persistentVolumeList.Items { + found := false + // Lookup if the PersistentVolume matches an entry in attachedVolumeNames. + if persistentVolume.Spec.CSI != nil { + attachedVolumeName := fmt.Sprintf("kubernetes.io/csi/%s^%s", persistentVolume.Spec.CSI.Driver, persistentVolume.Spec.CSI.VolumeHandle) + if attachedNodeVolumeNames.Has(attachedVolumeName) { + foundAttachedVolumeNames.Insert(attachedVolumeName) + found = true + } + } + + // Lookup if the PersistentVolume matches an entry in attachedPVNames. + if attachedPVNames.Has(persistentVolume.Name) { + foundAttachedPVNames.Insert(persistentVolume.Name) + found = true + } + + // PersistentVolume which do not match an entry in attachedVolumeNames or + // attachedPVNames can be ignored. + if !found { + continue + } + + // The ClaimRef should only be nil for unbound volumes and these should not be able to be attached. + // Also we're unable to map references which are not of Kind PersistentVolumeClaim so we record the PersistentVolume instead. + if persistentVolume.Spec.ClaimRef == nil || persistentVolume.Spec.ClaimRef.Kind != "PersistentVolumeClaim" { + persistentVolumesWithoutPVCClaimRef = append(persistentVolumesWithoutPVCClaimRef, persistentVolume.Name) + continue + } + + key := types.NamespacedName{Namespace: persistentVolume.Spec.ClaimRef.Namespace, Name: persistentVolume.Spec.ClaimRef.Name}.String() + // Add the PersistentVolumeClaim namespaced name to the list we are waiting for being detached. + pvcsWaitingForDetach.Insert(key) + } + + if persistentVolumeList.GetContinue() == "" { + break + } + } + + return &attachedVolumeInformation{ + persistentVolumeClaims: pvcsWaitingForDetach.Difference(pvcsToIgnore).UnsortedList(), + persistentVolumesWithoutPVCClaimRef: persistentVolumesWithoutPVCClaimRef, + nodeStatusVolumeNamesWithoutPV: attachedNodeVolumeNames.Difference(foundAttachedVolumeNames).UnsortedList(), + persistentVolumeNamesWithoutPV: attachedPVNames.Difference(foundAttachedPVNames).UnsortedList(), + }, nil +} diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index ac7f447e8d9d..675a866540ba 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -25,6 +25,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -1921,6 +1922,42 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { TypeMeta: metav1.TypeMeta{Kind: "Cluster", APIVersion: clusterv1.GroupVersion.String()}, ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "test-cluster"}, } + + nodeName := "test-node" + + persistentVolume := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pv", + }, + Spec: corev1.PersistentVolumeSpec{ + ClaimRef: &corev1.ObjectReference{ + Kind: "PersistentVolumeClaim", + Namespace: "default", + Name: "test-pvc", + }, + PersistentVolumeSource: corev1.PersistentVolumeSource{ + CSI: &corev1.CSIPersistentVolumeSource{ + VolumeHandle: "foo", + Driver: "dummy", + }, + }, + }, + } + + volumeAttachment := &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-va", + }, + Spec: storagev1.VolumeAttachmentSpec{ + NodeName: nodeName, + Source: storagev1.VolumeAttachmentSource{ + PersistentVolumeName: &persistentVolume.Name, + }, + }, + Status: storagev1.VolumeAttachmentStatus{ + Attached: true, + }, + } testMachine := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "test", @@ -1935,7 +1972,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { attachedVolumes := []corev1.AttachedVolume{ { - Name: "test-volume", + Name: corev1.UniqueVolumeName(fmt.Sprintf("kubernetes.io/csi/%s^%s", persistentVolume.Spec.PersistentVolumeSource.CSI.Driver, persistentVolume.Spec.PersistentVolumeSource.CSI.VolumeHandle)), DevicePath: "test-path", }, } @@ -1943,15 +1980,168 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { tests := []struct { name string node *corev1.Node + objs []client.Object expected bool expectedDeletingReason string expectedDeletingMessage string }{ { - name: "Node has volumes attached", + name: "Node has volumes attached according to node status", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + VolumesAttached: attachedVolumes, + }, + }, + objs: []client.Object{ + persistentVolume, + }, + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", + }, + { + name: "Node has volumes attached according to node status but its from a daemonset pod which gets ignored", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + VolumesAttached: attachedVolumes, + }, + }, + objs: []client.Object{ + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: appsv1.SchemeGroupVersion.WithKind("DaemonSet").Kind, + Name: "test-ds", + Controller: ptr.To(true), + }, + }, + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Volumes: []corev1.Volume{ + { + Name: "test-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "test-pvc", + }, + }, + }, + }, + }, + }, + &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ds", + Namespace: "default", + }, + }, + persistentVolume, + }, + expected: false, + }, + { + name: "Node has volumes attached according to volumeattachments", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + objs: []client.Object{ + volumeAttachment, + persistentVolume, + }, + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", + }, + { + name: "Node has volumes attached according to volumeattachments but its from a daemonset pod which gets ignored", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + objs: []client.Object{ + volumeAttachment, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: appsv1.SchemeGroupVersion.WithKind("DaemonSet").Kind, + Name: "test-ds", + Controller: ptr.To(true), + }, + }, + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Volumes: []corev1.Volume{ + { + Name: "test-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "test-pvc", + }, + }, + }, + }, + }, + }, + &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ds", + Namespace: "default", + }, + }, + persistentVolume, + }, + expected: false, + }, + { + name: "Node has volumes attached from a Pod which is in deletion", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-node", + Name: nodeName, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ @@ -1963,6 +2153,33 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { VolumesAttached: attachedVolumes, }, }, + objs: []client.Object{ + volumeAttachment, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "default", + DeletionTimestamp: ptr.To(metav1.NewTime(time.Now().Add(time.Hour * 24 * -1))), + Finalizers: []string{ + "prevent-removal", + }, + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Volumes: []corev1.Volume{ + { + Name: "test-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "test-pvc", + }, + }, + }, + }, + }, + }, + persistentVolume, + }, expected: true, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", @@ -1971,7 +2188,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { name: "Node has no volumes attached", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-node", + Name: nodeName, }, Status: corev1.NodeStatus{ Conditions: []corev1.NodeCondition{ @@ -2026,11 +2243,18 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { var objs []client.Object objs = append(objs, testCluster, tt.node) + objs = append(objs, tt.objs...) - c := fake.NewClientBuilder().WithObjects(objs...).Build() + c := fake.NewClientBuilder().WithIndex(&corev1.Pod{}, "spec.nodeName", nodeNameIndex). + WithObjects(objs...).Build() r := &Reconciler{ - Client: c, - ClusterCache: clustercache.NewFakeClusterCache(c, client.ObjectKeyFromObject(testCluster)), + Client: c, + ClusterCache: clustercache.NewFakeClusterCache(c, client.ObjectKeyFromObject(testCluster)), + reconcileDeleteCache: cache.New[cache.ReconcileEntry](), + } + + testMachine.Status.NodeRef = &corev1.ObjectReference{ + Name: tt.node.GetName(), } s := &scope{ @@ -2038,15 +2262,19 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { machine: testMachine, } - got, err := r.shouldWaitForNodeVolumes(ctx, s, tt.node.Name) + got, err := r.shouldWaitForNodeVolumes(ctx, s) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.expected)) + g.Expect(!got.IsZero()).To(Equal(tt.expected)) g.Expect(s.deletingReason).To(Equal(tt.expectedDeletingReason)) g.Expect(s.deletingMessage).To(Equal(tt.expectedDeletingMessage)) }) } } +func nodeNameIndex(o client.Object) []string { + return []string{o.(*corev1.Pod).Spec.NodeName} +} + func TestIsDeleteNodeAllowed(t *testing.T) { deletionts := metav1.Now() diff --git a/main.go b/main.go index 95706478afeb..9cc34c5f103e 100644 --- a/main.go +++ b/main.go @@ -29,6 +29,7 @@ import ( "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -127,6 +128,7 @@ var ( func init() { _ = clientgoscheme.AddToScheme(scheme) _ = apiextensionsv1.AddToScheme(scheme) + _ = storagev1.AddToScheme(scheme) _ = clusterv1alpha3.AddToScheme(scheme) _ = clusterv1alpha4.AddToScheme(scheme) @@ -433,6 +435,9 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager, watchNamespaces map // Don't cache Pods & DaemonSets (we get/list them e.g. during drain). &corev1.Pod{}, &appsv1.DaemonSet{}, + // Don't cache PersistentVolumes and VolumeAttachments (we get/list them e.g. during wait for volumes to detach) + &storagev1.VolumeAttachment{}, + &corev1.PersistentVolume{}, }, }, }, diff --git a/util/log/log.go b/util/log/log.go index 11a41c460361..5f7432e51f0a 100644 --- a/util/log/log.go +++ b/util/log/log.go @@ -107,19 +107,16 @@ func getOwners(ctx context.Context, c client.Client, obj metav1.Object) ([]owner return owners, nil } -// ObjNamesString returns a comma separated list of the object names, limited to -// five objects. On more than five objects it outputs the first five objects and -// adds information about how much more are in the given list. -func ObjNamesString[T client.Object](objs []T) string { +// ListToString returns a comma-separated list of the first n entries of the list (strings are calculated via stringFunc). +func ListToString[T any](list []T, stringFunc func(T) string, n int) string { shortenedBy := 0 - if len(objs) > 5 { - shortenedBy = len(objs) - 5 - objs = objs[:5] + if len(list) > n { + shortenedBy = len(list) - n + list = list[:n] } - stringList := []string{} - for _, obj := range objs { - stringList = append(stringList, obj.GetName()) + for _, p := range list { + stringList = append(stringList, stringFunc(p)) } if shortenedBy > 0 { @@ -128,3 +125,21 @@ func ObjNamesString[T client.Object](objs []T) string { return strings.Join(stringList, ", ") } + +// StringListToString returns a comma separated list of the strings, limited to +// five objects. On more than five objects it outputs the first five objects and +// adds information about how much more are in the given list. +func StringListToString(objs []string) string { + return ListToString(objs, func(s string) string { + return s + }, 5) +} + +// ObjNamesString returns a comma separated list of the object names, limited to +// five objects. On more than five objects it outputs the first five objects and +// adds information about how much more are in the given list. +func ObjNamesString[T client.Object](objs []T) string { + return ListToString(objs, func(obj T) string { + return obj.GetName() + }, 5) +} From 3a0dfa79e3b3c2d87c17349d91f364f61af2aa5c Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 7 Oct 2024 11:42:46 +0200 Subject: [PATCH 2/4] inmemory: add support for storage.k8s.io/v1 volumeattachments --- .../inmemory/pkg/server/api/const.go | 35 +++++++++++++++++-- .../inmemory/pkg/server/api/handler.go | 10 ++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/test/infrastructure/inmemory/pkg/server/api/const.go b/test/infrastructure/inmemory/pkg/server/api/const.go index 391471041763..b40f62ada73f 100644 --- a/test/infrastructure/inmemory/pkg/server/api/const.go +++ b/test/infrastructure/inmemory/pkg/server/api/const.go @@ -25,7 +25,7 @@ var ( Versions: []string{"v1"}, } - // apiVersions is the value returned by /api/v1 discovery call. + // corev1APIResourceList is the value returned by /api/v1 discovery call. // Note: This must contain all APIs required by CAPI. corev1APIResourceList = &metav1.APIResourceList{ GroupVersion: "v1", @@ -164,7 +164,7 @@ var ( }, } - // apiVersions is the value returned by /apis discovery call. + // apiGroupList is the value returned by /apis discovery call. // Note: This must contain all APIs required by CAPI. apiGroupList = &metav1.APIGroupList{ Groups: []metav1.APIGroup{ @@ -197,7 +197,7 @@ var ( }, } - // apiVersions is the value returned by /apis/rbac.authorization.k8s.io/v1 discovery call. + // rbacv1APIResourceList is the value returned by /apis/rbac.authorization.k8s.io/v1 discovery call. // Note: This must contain all APIs required by CAPI. rbacv1APIResourceList = &metav1.APIResourceList{ GroupVersion: "rbac.authorization.k8s.io/v1", @@ -272,6 +272,9 @@ var ( }, }, } + + // appsV1ResourceList is the value returned by /apis/apps/v1 discovery call. + // Note: This must contain all APIs required by CAPI. appsV1ResourceList = &metav1.APIResourceList{ GroupVersion: "apps/v1", APIResources: []metav1.APIResource{ @@ -317,4 +320,30 @@ var ( }, }, } + + // storageV1ResourceList is the value returned by /apis/storage.k8s.io/v1 discovery call. + // Note: This must contain all APIs required by CAPI. + storageV1ResourceList = &metav1.APIResourceList{ + GroupVersion: "storage.k8s.io/v1", + APIResources: []metav1.APIResource{ + { + Name: "volumeattachments", + SingularName: "volumeattachment", + Namespaced: false, + Kind: "VolumeAttachment", + Verbs: []string{ + "create", + "delete", + "deletecollection", + "get", + "list", + "patch", + "update", + "watch", + }, + ShortNames: []string{}, + StorageVersionHash: "", + }, + }, + } ) diff --git a/test/infrastructure/inmemory/pkg/server/api/handler.go b/test/infrastructure/inmemory/pkg/server/api/handler.go index e0a2db0e671e..b6378c9215d7 100644 --- a/test/infrastructure/inmemory/pkg/server/api/handler.go +++ b/test/infrastructure/inmemory/pkg/server/api/handler.go @@ -222,6 +222,13 @@ func (h *apiServerHandler) apisDiscovery(req *restful.Request, resp *restful.Res } return } + if req.PathParameter("group") == "storage.k8s.io" && req.PathParameter("version") == "v1" { + if err := resp.WriteEntity(storageV1ResourceList); err != nil { + _ = resp.WriteErrorString(http.StatusInternalServerError, err.Error()) + return + } + return + } _ = resp.WriteErrorString(http.StatusInternalServerError, fmt.Sprintf("discovery info not defined for %s/%s", req.PathParameter("group"), req.PathParameter("version"))) return @@ -667,6 +674,9 @@ func getAPIResourceList(req *restful.Request) *metav1.APIResourceList { if req.PathParameter("group") == "apps" && req.PathParameter("version") == "v1" { return appsV1ResourceList } + if req.PathParameter("group") == "storage.k8s.io" && req.PathParameter("version") == "v1" { + return storageV1ResourceList + } return nil } return corev1APIResourceList From 521918bc198c2df3b61e9ca3b3f0cf215fab99b3 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 18 Oct 2024 12:31:45 +0200 Subject: [PATCH 3/4] add more details to the v1beta2 deleting condition --- .../controllers/machine/machine_controller.go | 32 +++++- .../machine/machine_controller_test.go | 97 ++++++++++++++++--- 2 files changed, 117 insertions(+), 12 deletions(-) diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index b4f3518f2693..29046a008fec 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -935,7 +935,7 @@ func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope) (ct r.reconcileDeleteCache.Add(cache.NewReconcileEntry(machine, time.Now().Add(waitForVolumeDetachRetryInterval))) s.deletingReason = clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason - s.deletingMessage = fmt.Sprintf("Waiting for Node volumes to be detached (started at %s)", machine.Status.Deletion.WaitForNodeVolumeDetachStartTime.Format(time.RFC3339)) + s.deletingMessage = attachedVolumeInformation.conditionMessage(machine) log.Info("Waiting for Node volumes to be detached", attachedVolumeInformation.logKeys()..., @@ -1240,6 +1240,36 @@ func (a *attachedVolumeInformation) logKeys() []any { return logKeys } +func (a *attachedVolumeInformation) conditionMessage(machine *clusterv1.Machine) string { + if a.isEmpty() { + return "" + } + + conditionMessage := fmt.Sprintf("Waiting for Node volumes to be detached (started at %s)", machine.Status.Deletion.WaitForNodeVolumeDetachStartTime.Format(time.RFC3339)) + + if len(a.persistentVolumeClaims) > 0 { + slices.Sort(a.persistentVolumeClaims) + conditionMessage = fmt.Sprintf("%s\n* PersistentVolumeClaims: %s", conditionMessage, clog.StringListToString(a.persistentVolumeClaims)) + } + + if len(a.persistentVolumesWithoutPVCClaimRef) > 0 { + slices.Sort(a.persistentVolumesWithoutPVCClaimRef) + conditionMessage = fmt.Sprintf("%s\n* PersistentVolumes without a .spec.claimRef to a PersistentVolumeClaim: %s", conditionMessage, clog.StringListToString(a.persistentVolumesWithoutPVCClaimRef)) + } + + if len(a.nodeStatusVolumeNamesWithoutPV) > 0 { + slices.Sort(a.nodeStatusVolumeNamesWithoutPV) + conditionMessage = fmt.Sprintf("%s\n* Node.status.volumesAttached entries not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.nodeStatusVolumeNamesWithoutPV)) + } + + if len(a.persistentVolumeNamesWithoutPV) > 0 { + slices.Sort(a.persistentVolumeNamesWithoutPV) + conditionMessage = fmt.Sprintf("%s\n* VolumeAttachment.spec.source.persistentVolumeName not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.persistentVolumeNamesWithoutPV)) + } + + return conditionMessage +} + // getPersistentVolumesWaitingForDetach returns a list of all persistentVolumes which either have // the calculated AttachedVolume name in attachedVolumeNames or their name in attachedPVNames. // PersistentVolumes which refer a PersistentVolumeClaim contained in pvcsToIgnore are filtered out. diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 675a866540ba..56c70bad3062 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -1944,6 +1944,9 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, } + persistentVolumeWithoutClaim := persistentVolume.DeepCopy() + persistentVolumeWithoutClaim.Spec.ClaimRef.Kind = "NotAPVC" + volumeAttachment := &storagev1.VolumeAttachment{ ObjectMeta: metav1.ObjectMeta{ Name: "test-va", @@ -2004,9 +2007,56 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { objs: []client.Object{ persistentVolume, }, - expected: true, - expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, - expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* PersistentVolumeClaims: default/test-pvc`, + }, + { + name: "Node has volumes attached according to node status but the pv does not reference a PersistentVolumeClaim", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + VolumesAttached: attachedVolumes, + }, + }, + objs: []client.Object{ + persistentVolumeWithoutClaim, + }, + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* PersistentVolumes without a .spec.claimRef to a PersistentVolumeClaim: test-pv`, + }, + { + name: "Node has volumes attached according to node status but without a pv", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + VolumesAttached: attachedVolumes, + }, + }, + objs: []client.Object{}, + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* Node.status.volumesAttached entries not matching a PersistentVolume: kubernetes.io/csi/dummy^foo`, }, { name: "Node has volumes attached according to node status but its from a daemonset pod which gets ignored", @@ -2080,9 +2130,33 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { volumeAttachment, persistentVolume, }, - expected: true, - expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, - expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* PersistentVolumeClaims: default/test-pvc`, + }, + { + name: "Node has volumes attached according to volumeattachments but without a pv", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + objs: []client.Object{ + volumeAttachment, + }, + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* VolumeAttachment.spec.source.persistentVolumeName not matching a PersistentVolume: test-pv`, }, { name: "Node has volumes attached according to volumeattachments but its from a daemonset pod which gets ignored", @@ -2180,9 +2254,10 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, persistentVolume, }, - expected: true, - expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, - expectedDeletingMessage: "Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z)", + expected: true, + expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, + expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) +* PersistentVolumeClaims: default/test-pvc`, }, { name: "Node has no volumes attached", @@ -2265,8 +2340,8 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { got, err := r.shouldWaitForNodeVolumes(ctx, s) g.Expect(err).ToNot(HaveOccurred()) g.Expect(!got.IsZero()).To(Equal(tt.expected)) - g.Expect(s.deletingReason).To(Equal(tt.expectedDeletingReason)) - g.Expect(s.deletingMessage).To(Equal(tt.expectedDeletingMessage)) + g.Expect(s.deletingReason).To(BeEquivalentTo(tt.expectedDeletingReason)) + g.Expect(s.deletingMessage).To(BeEquivalentTo(tt.expectedDeletingMessage)) }) } } From b3ecaad11af127a2be5fae0d39ef66fd8b940d36 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 22 Oct 2024 09:02:02 +0200 Subject: [PATCH 4/4] review fixes --- .../controllers/machine/machine_controller.go | 39 +++++++++---------- .../machine/machine_controller_test.go | 30 +++++++------- 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 29046a008fec..51d42434b0f2 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -788,7 +788,7 @@ func (r *Reconciler) drainNode(ctx context.Context, s *scope) (ctrl.Result, erro if err := remoteClient.Get(ctx, client.ObjectKey{Name: nodeName}, node); err != nil { if apierrors.IsNotFound(err) { // If an admin deletes the node directly, we'll end up here. - log.Info("Could not find Node from Machine.status.nodeRef, skip waiting for volume detachment.") + log.Info("Could not find Node from Machine.status.nodeRef, skipping Node drain.") return ctrl.Result{}, nil } return ctrl.Result{}, errors.Wrapf(err, "unable to get Node %s", nodeName) @@ -887,7 +887,7 @@ func (r *Reconciler) shouldWaitForNodeVolumes(ctx context.Context, s *scope) (ct node := &corev1.Node{} if err := remoteClient.Get(ctx, types.NamespacedName{Name: nodeName}, node); err != nil { if apierrors.IsNotFound(err) { - log.Error(err, "Could not find node from noderef, it may have already been deleted") + log.Info("Could not find Node from Machine.status.nodeRef, skip waiting for volume detachment.") return ctrl.Result{}, nil } return ctrl.Result{}, err @@ -1196,7 +1196,7 @@ func getVolumeAttachmentForNode(ctx context.Context, c client.Client, nodeName s } type attachedVolumeInformation struct { - // Attached PersistentVolumeClaims. + // Attached PersistentVolumeClaims filtered by references from pods that don't get drained. persistentVolumeClaims []string // Attached PersistentVolumes without a corresponding PersistentVolumeClaim. @@ -1259,32 +1259,29 @@ func (a *attachedVolumeInformation) conditionMessage(machine *clusterv1.Machine) if len(a.nodeStatusVolumeNamesWithoutPV) > 0 { slices.Sort(a.nodeStatusVolumeNamesWithoutPV) - conditionMessage = fmt.Sprintf("%s\n* Node.status.volumesAttached entries not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.nodeStatusVolumeNamesWithoutPV)) + conditionMessage = fmt.Sprintf("%s\n* Node with .status.volumesAttached entries not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.nodeStatusVolumeNamesWithoutPV)) } if len(a.persistentVolumeNamesWithoutPV) > 0 { slices.Sort(a.persistentVolumeNamesWithoutPV) - conditionMessage = fmt.Sprintf("%s\n* VolumeAttachment.spec.source.persistentVolumeName not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.persistentVolumeNamesWithoutPV)) + conditionMessage = fmt.Sprintf("%s\n* VolumeAttachment with .spec.source.persistentVolumeName not matching a PersistentVolume: %s", conditionMessage, clog.StringListToString(a.persistentVolumeNamesWithoutPV)) } return conditionMessage } -// getPersistentVolumesWaitingForDetach returns a list of all persistentVolumes which either have -// the calculated AttachedVolume name in attachedVolumeNames or their name in attachedPVNames. -// PersistentVolumes which refer a PersistentVolumeClaim contained in pvcsToIgnore are filtered out. -// If there are names in attachedVolumeNames or attachedPVNames without a corresponding PV, this returns an error. +// getPersistentVolumesWaitingForDetach returns information about attached volumes +// that correspond either to attachedVolumeNames or attachedPVNames. +// Volumes which refer a PersistentVolumeClaim contained in pvcsToIgnore are filtered out. func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, attachedNodeVolumeNames, attachedPVNames, pvcsToIgnore sets.Set[string]) (*attachedVolumeInformation, error) { - pvcsWaitingForDetach := sets.Set[string]{} - persistentVolumesWithoutPVCClaimRef := []string{} - foundAttachedVolumeNames := sets.Set[string]{} + attachedPVCs := sets.Set[string]{} + attachedPVsWithoutPVCClaimRef := []string{} + foundAttachedNodeVolumeNames := sets.Set[string]{} foundAttachedPVNames := sets.Set[string]{} // List all PersistentVolumes and preserve the ones we have to wait for. // Also store the found VolumeHandles and names of PersistentVolumes to check // that all expected PersistentVolumes have been found. - // Note: pvsWaitingFor will not include PersistentVolumes we want to ignore. - // These are usually volumes which have a Pod which is expected to be kept. persistentVolumeList := &corev1.PersistentVolumeList{} for { listOpts := []client.ListOption{ @@ -1301,7 +1298,7 @@ func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, if persistentVolume.Spec.CSI != nil { attachedVolumeName := fmt.Sprintf("kubernetes.io/csi/%s^%s", persistentVolume.Spec.CSI.Driver, persistentVolume.Spec.CSI.VolumeHandle) if attachedNodeVolumeNames.Has(attachedVolumeName) { - foundAttachedVolumeNames.Insert(attachedVolumeName) + foundAttachedNodeVolumeNames.Insert(attachedVolumeName) found = true } } @@ -1312,7 +1309,7 @@ func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, found = true } - // PersistentVolume which do not match an entry in attachedVolumeNames or + // PersistentVolume which do not match an entry in attachedNodeVolumeNames or // attachedPVNames can be ignored. if !found { continue @@ -1321,13 +1318,13 @@ func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, // The ClaimRef should only be nil for unbound volumes and these should not be able to be attached. // Also we're unable to map references which are not of Kind PersistentVolumeClaim so we record the PersistentVolume instead. if persistentVolume.Spec.ClaimRef == nil || persistentVolume.Spec.ClaimRef.Kind != "PersistentVolumeClaim" { - persistentVolumesWithoutPVCClaimRef = append(persistentVolumesWithoutPVCClaimRef, persistentVolume.Name) + attachedPVsWithoutPVCClaimRef = append(attachedPVsWithoutPVCClaimRef, persistentVolume.Name) continue } key := types.NamespacedName{Namespace: persistentVolume.Spec.ClaimRef.Namespace, Name: persistentVolume.Spec.ClaimRef.Name}.String() // Add the PersistentVolumeClaim namespaced name to the list we are waiting for being detached. - pvcsWaitingForDetach.Insert(key) + attachedPVCs.Insert(key) } if persistentVolumeList.GetContinue() == "" { @@ -1336,9 +1333,9 @@ func getPersistentVolumesWaitingForDetach(ctx context.Context, c client.Client, } return &attachedVolumeInformation{ - persistentVolumeClaims: pvcsWaitingForDetach.Difference(pvcsToIgnore).UnsortedList(), - persistentVolumesWithoutPVCClaimRef: persistentVolumesWithoutPVCClaimRef, - nodeStatusVolumeNamesWithoutPV: attachedNodeVolumeNames.Difference(foundAttachedVolumeNames).UnsortedList(), + persistentVolumeClaims: attachedPVCs.Difference(pvcsToIgnore).UnsortedList(), + persistentVolumesWithoutPVCClaimRef: attachedPVsWithoutPVCClaimRef, + nodeStatusVolumeNamesWithoutPV: attachedNodeVolumeNames.Difference(foundAttachedNodeVolumeNames).UnsortedList(), persistentVolumeNamesWithoutPV: attachedPVNames.Difference(foundAttachedPVNames).UnsortedList(), }, nil } diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 56c70bad3062..3ceed73f8bdc 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -1984,7 +1984,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { name string node *corev1.Node objs []client.Object - expected bool + expected ctrl.Result expectedDeletingReason string expectedDeletingMessage string }{ @@ -2007,7 +2007,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { objs: []client.Object{ persistentVolume, }, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) * PersistentVolumeClaims: default/test-pvc`, @@ -2031,7 +2031,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { objs: []client.Object{ persistentVolumeWithoutClaim, }, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) * PersistentVolumes without a .spec.claimRef to a PersistentVolumeClaim: test-pv`, @@ -2053,10 +2053,10 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, }, objs: []client.Object{}, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) -* Node.status.volumesAttached entries not matching a PersistentVolume: kubernetes.io/csi/dummy^foo`, +* Node with .status.volumesAttached entries not matching a PersistentVolume: kubernetes.io/csi/dummy^foo`, }, { name: "Node has volumes attached according to node status but its from a daemonset pod which gets ignored", @@ -2109,7 +2109,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, persistentVolume, }, - expected: false, + expected: ctrl.Result{}, }, { name: "Node has volumes attached according to volumeattachments", @@ -2130,7 +2130,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { volumeAttachment, persistentVolume, }, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) * PersistentVolumeClaims: default/test-pvc`, @@ -2153,10 +2153,10 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { objs: []client.Object{ volumeAttachment, }, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) -* VolumeAttachment.spec.source.persistentVolumeName not matching a PersistentVolume: test-pv`, +* VolumeAttachment with .spec.source.persistentVolumeName not matching a PersistentVolume: test-pv`, }, { name: "Node has volumes attached according to volumeattachments but its from a daemonset pod which gets ignored", @@ -2209,7 +2209,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, persistentVolume, }, - expected: false, + expected: ctrl.Result{}, }, { name: "Node has volumes attached from a Pod which is in deletion", @@ -2254,7 +2254,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, persistentVolume, }, - expected: true, + expected: ctrl.Result{RequeueAfter: waitForVolumeDetachRetryInterval}, expectedDeletingReason: clusterv1.MachineDeletingWaitingForVolumeDetachV1Beta2Reason, expectedDeletingMessage: `Waiting for Node volumes to be detached (started at 2024-10-09T16:13:59Z) * PersistentVolumeClaims: default/test-pvc`, @@ -2274,7 +2274,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, }, }, - expected: false, + expected: ctrl.Result{}, }, { name: "Node is unreachable and has volumes attached", @@ -2292,7 +2292,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { VolumesAttached: attachedVolumes, }, }, - expected: false, + expected: ctrl.Result{}, }, { name: "Node is unreachable and has no volumes attached", @@ -2309,7 +2309,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { }, }, }, - expected: false, + expected: ctrl.Result{}, }, } for _, tt := range tests { @@ -2339,7 +2339,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) { got, err := r.shouldWaitForNodeVolumes(ctx, s) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(!got.IsZero()).To(Equal(tt.expected)) + g.Expect(got).To(BeEquivalentTo(tt.expected)) g.Expect(s.deletingReason).To(BeEquivalentTo(tt.expectedDeletingReason)) g.Expect(s.deletingMessage).To(BeEquivalentTo(tt.expectedDeletingMessage)) })