diff --git a/cmd/machine-controller-manager/app/options/options.go b/cmd/machine-controller-manager/app/options/options.go index 9532557805..8378575e52 100644 --- a/cmd/machine-controller-manager/app/options/options.go +++ b/cmd/machine-controller-manager/app/options/options.go @@ -70,6 +70,7 @@ func NewMCMServer() *MCMServer { SafetyUp: 2, SafetyDown: 1, MachineSafetyOvershootingPeriod: metav1.Duration{Duration: 1 * time.Minute}, + MachinePreserveTimeout: metav1.Duration{Duration: 96 * time.Hour}, }, }, } @@ -98,6 +99,7 @@ func (s *MCMServer) AddFlags(fs *pflag.FlagSet) { fs.Int32Var(&s.SafetyOptions.SafetyDown, "safety-down", s.SafetyOptions.SafetyDown, "Upper-limit minus safety-down value gives the lower-limit. This is the limits below which any temporarily frozen machineSet/machineDeployment object is unfrozen. lower-limit = desired + maxSurge (if applicable) + safetyUp - safetyDown.") fs.DurationVar(&s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "machine-safety-overshooting-period", s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "Time period (in duration) used to poll for overshooting of machine objects backing a machineSet by safety controller.") + fs.DurationVar(&s.SafetyOptions.MachinePreserveTimeout.Duration, "machine-preserve-timeout", s.SafetyOptions.MachinePreserveTimeout.Duration, "Duration for which a failed machine should be preserved if it has the appropriate preserve annotation set.") fs.BoolVar(&s.AutoscalerScaleDownAnnotationDuringRollout, "autoscaler-scaledown-annotation-during-rollout", true, "Add cluster autoscaler scale-down disabled annotation during roll-out.") diff --git a/pkg/controller/deployment.go b/pkg/controller/deployment.go index 14a1cbb87c..9efa96f501 100644 --- a/pkg/controller/deployment.go +++ b/pkg/controller/deployment.go @@ -558,7 +558,7 @@ func (dc *controller) reconcileClusterMachineDeployment(key string) error { return err } - err = dc.updateMachineAndMachineDeploymentDeletionAnnotations(ctx, d) + d, err = dc.updateMachineAndMachineDeploymentDeletionAnnotations(ctx, d, machineMap) if err != nil { return err } @@ -659,10 +659,10 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac } } -func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd *v1alpha1.MachineDeployment) (err error) { +func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd *v1alpha1.MachineDeployment, machineMap map[types.UID]*v1alpha1.MachineList) (*v1alpha1.MachineDeployment, error) { tgd := dc.computeMachineTriggerDeletionData(mcd) if tgd == nil { - return nil + return mcd, nil } if tgd.triggerDeletionAnnotationValueChanged { @@ -674,12 +674,13 @@ func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx c if mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM] == "" { delete(mcdDeepCopy.Annotations, machineutils.TriggerDeletionByMCM) } - _, err = dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdDeepCopy, metav1.UpdateOptions{}) + updatedMCD, err := dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdDeepCopy, metav1.UpdateOptions{}) if err != nil { klog.Errorf("failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(tgd.markedMachines), mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM]) - return + return mcd, err } klog.V(3).Infof("Updated MachineDeployment %q with #%d machines still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(tgd.markedMachines), mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM]) + mcd = updatedMCD } for i, machine := range tgd.markedMachines { @@ -687,23 +688,33 @@ func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx c klog.V(4).Infof("Machine %q of MachineDeployment %q already has MachinePriority=1 and MarkedForDeletionTime=%q annotation", machine.Name, mcd.Name, machine.Annotations[machineutils.MarkedForDeletionTime]) continue } - machineDeepCopy := machine.DeepCopy() - if machineDeepCopy.Annotations == nil { - machineDeepCopy.Annotations = make(map[string]string) - } - machineDeepCopy.Annotations[machineutils.MachinePriority] = "1" - if machineDeepCopy.Annotations[machineutils.MarkedForDeletionTime] == "" { - machineDeepCopy.Annotations[machineutils.MarkedForDeletionTime] = tgd.markedMachineDeletionTimes[i] - } - _, err = dc.controlMachineClient.Machines(machine.Namespace).Update(ctx, machineDeepCopy, metav1.UpdateOptions{}) + updatedMachine, err := machineutils.PatchMachine(ctx, dc.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + if m.Annotations == nil { + m.Annotations = make(map[string]string) + } + m.Annotations[machineutils.MachinePriority] = "1" + if m.Annotations[machineutils.MarkedForDeletionTime] == "" { + m.Annotations[machineutils.MarkedForDeletionTime] = tgd.markedMachineDeletionTimes[i] + } + return nil + }, true) if err != nil { klog.Errorf("failed to set MachinePriority=1 annotation on Machine %q of MachineDeployment %q: %v", machine.Name, mcd.Name, err) - return + return mcd, err + } + if controllerRef := metav1.GetControllerOf(updatedMachine); controllerRef != nil { + if machineList, ok := machineMap[controllerRef.UID]; ok { + for i, machine := range machineList.Items { + if machine.Name == updatedMachine.Name && machine.Namespace == updatedMachine.Namespace { + machineList.Items[i] = *updatedMachine + } + } + } } klog.V(3).Infof("Machine %q of MachineDeployment %q marked with MachinePriority=1 annotation successfully", machine.Name, mcd.Name) } - return + return mcd, nil } // computeMachineTriggerDeletionData computes the data related to machines that are triggered for deletion based on the annotation on the MachineDeployment. diff --git a/pkg/controller/deployment_sync.go b/pkg/controller/deployment_sync.go index 5b237d5b5d..e666b0b4cd 100644 --- a/pkg/controller/deployment_sync.go +++ b/pkg/controller/deployment_sync.go @@ -191,7 +191,7 @@ func (dc *controller) addHashKeyToISAndMachines(ctx context.Context, is *v1alpha } // 2. Update all machines managed by the rs to have the new hash label, so they will be correctly adopted. - if err := LabelMachinesWithHash(ctx, machineList, dc.controlMachineClient, dc.machineLister, is.Namespace, is.Name, hash); err != nil { + if err := LabelMachinesWithHash(ctx, machineList, dc.controlMachineClient, is.Namespace, is.Name, hash); err != nil { return nil, fmt.Errorf("error in adding template hash label %s to machines %+v: %s", hash, machineList, err) } diff --git a/pkg/controller/deployment_test.go b/pkg/controller/deployment_test.go index 9bc8e05b69..789979efaa 100644 --- a/pkg/controller/deployment_test.go +++ b/pkg/controller/deployment_test.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" ) @@ -1982,7 +1983,11 @@ var _ = Describe("machineDeployment", func() { testMachineDeployment.Annotations[machineutils.TriggerDeletionByMCM] = fmt.Sprintf("%s~%s", testMachine.Name, time.Now().Format(time.RFC3339)) }, func(_ *machinev1.MachineDeployment, mcs []machinev1.MachineSet, _ []machinev1.Machine, _ *corev1.Node) error { - Expect(mcs[0].Annotations[machineutils.LastDeploymentReplicaChangeByScalerTime]).To(Equal(ts)) + actualTS, err := time.Parse(time.RFC3339, mcs[0].Annotations[machineutils.LastDeploymentReplicaChangeByScalerTime]) + Expect(err).NotTo(HaveOccurred()) + expectedTS, err := time.Parse(time.RFC3339, ts) + Expect(err).NotTo(HaveOccurred()) + Expect(actualTS).To(BeTemporally(">=", expectedTS)) _, exists := mcs[0].Annotations[machineutils.TriggerDeletionByMCM] Expect(exists).To(BeFalse()) return nil @@ -2356,7 +2361,7 @@ var _ = Describe("machineDeployment", func() { defer trackers.Stop() waitForCacheSync(stop, c) - err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment) + _, err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment, map[types.UID]*machinev1.MachineList{}) Expect(err).To(BeNil()) waitForCacheSync(stop, c) diff --git a/pkg/controller/deployment_util.go b/pkg/controller/deployment_util.go index fdf958d8cc..90e5e6e4ed 100644 --- a/pkg/controller/deployment_util.go +++ b/pkg/controller/deployment_util.go @@ -41,7 +41,6 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/errors" intstrutil "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" @@ -963,26 +962,22 @@ func WaitForMachinesHashPopulated(ctx context.Context, c v1alpha1listers.Machine } // LabelMachinesWithHash labels all machines in the given machineList with the new hash label. -func LabelMachinesWithHash(ctx context.Context, machineList *v1alpha1.MachineList, c v1alpha1client.MachineV1alpha1Interface, machineLister v1alpha1listers.MachineLister, namespace, name, hash string) error { - for _, machine := range machineList.Items { +func LabelMachinesWithHash(ctx context.Context, machineList *v1alpha1.MachineList, c v1alpha1client.MachineV1alpha1Interface, namespace, name, hash string) error { + for i, machine := range machineList.Items { // Ignore inactive Machines. if !machineutils.IsMachineActive(&machine) { continue } // Only label the machine that doesn't already have the new hash if machine.Labels[v1alpha1.DefaultMachineDeploymentUniqueLabelKey] != hash { - _, err := machineutils.UpdateMachineWithRetries(ctx, c.Machines(machine.Namespace), machineLister, machine.Namespace, machine.Name, - func(machineToUpdate *v1alpha1.Machine) error { - // Precondition: the machine doesn't contain the new hash in its label. - if machineToUpdate.Labels[v1alpha1.DefaultMachineDeploymentUniqueLabelKey] == hash { - return errors.ErrPreconditionViolated - } - machineToUpdate.Labels = labelsutil.AddLabel(machineToUpdate.Labels, v1alpha1.DefaultMachineDeploymentUniqueLabelKey, hash) - return nil - }) + updatedMachine, err := machineutils.PatchMachine(ctx, c.Machines(machine.Namespace), &machine, func(m *v1alpha1.Machine) error { + m.Labels = labelsutil.AddLabel(m.Labels, v1alpha1.DefaultMachineDeploymentUniqueLabelKey, hash) + return nil + }, true) if err != nil { return fmt.Errorf("error in adding template hash label %s to machine %q: %v", hash, machine.Name, err) } + machineList.Items[i] = *updatedMachine klog.V(4).Infof("Labeled machine %s/%s of MachineSet %s/%s with hash %s.", machine.Namespace, machine.Name, namespace, name, hash) } } diff --git a/pkg/controller/machineset.go b/pkg/controller/machineset.go index 23d07dcea1..fab76bd83e 100644 --- a/pkg/controller/machineset.go +++ b/pkg/controller/machineset.go @@ -41,7 +41,7 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/integer" - "github.com/gardener/machine-controller-manager/pkg/apis/machine" + machineapi "github.com/gardener/machine-controller-manager/pkg/apis/machine" "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" "github.com/gardener/machine-controller-manager/pkg/apis/machine/validation" "github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils" @@ -506,7 +506,7 @@ func (c *controller) reconcileClusterMachineSet(key string) error { } // Validate MachineSet - internalMachineSet := &machine.MachineSet{} + internalMachineSet := &machineapi.MachineSet{} err = c.internalExternalScheme.Convert(machineSet, internalMachineSet, nil) if err != nil { return err @@ -534,19 +534,25 @@ func (c *controller) reconcileClusterMachineSet(key string) error { // list all machines to include the machines that don't match the rs`s selector // anymore but has the stale controller ref. // TODO: Do the List and Filter in a single pass, or use an index. - filteredMachines, err := c.machineLister.List(labels.Everything()) + machineList, err := c.machineLister.List(labels.Everything()) if err != nil { return err } // NOTE: filteredMachines are pointing to objects from cache - if you need to // modify them, you need to copy it first. - filteredMachines, err = c.claimMachines(ctx, machineSet, selector, filteredMachines) + filteredMachines, err := c.claimMachines(ctx, machineSet, selector, machineList) if err != nil { return err } + // Deep-copy to avoid mutating cache objects downstream. + for i, m := range filteredMachines { + filteredMachines[i] = m.DeepCopy() + } + // syncMachinesNodeTemplates syncs the nodeTemplate with claimedMachines if any of the machine's nodeTemplate has changed. + // TODO: too many update calls for the same machine object. Try to reduce it. err = c.syncMachinesNodeTemplates(ctx, filteredMachines, machineSet) if err != nil { return err @@ -563,7 +569,10 @@ func (c *controller) reconcileClusterMachineSet(key string) error { return err } - filteredMachines = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet) + filteredMachines, err = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet) + if err != nil { + return err + } // TODO: Fix working of expectations to reflect correct behaviour // machineSetNeedsSync := c.expectations.SatisfiedExpectations(key) @@ -904,47 +913,35 @@ func isMachineStatusEqual(s1, s2 v1alpha1.MachineStatus) bool { // or if it is a candidate for auto-preservation. If none of these conditions are met, it returns true indicating // that the failed machine should be terminated. func (c *controller) shouldFailedMachineBeTerminated(machine *v1alpha1.Machine) bool { - // if preserve expiry time is set and is in the future, machine is already preserved - if machine.Status.CurrentStatus.PreserveExpiryTime != nil { - if machine.Status.CurrentStatus.PreserveExpiryTime.After(time.Now()) { - klog.V(3).Infof("Failed machine %q is preserved until %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) - return false - } - klog.V(3).Infof("Preservation of failed machine %q has timed out at %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) + if machine.Status.CurrentStatus.PreserveExpiryTime == nil { return true } - preserveValue, err := c.findEffectivePreserveValue(machine) - if err != nil { - // in case of error fetching node or annotations, we don't want to block deletion of failed machines, so we return true - klog.Errorf("error finding effective preserve value for machine %q: %v. Proceeding with termination of the machine.", machine.Name, err) - return true - } - switch preserveValue { - case machineutils.PreserveMachineAnnotationValueWhenFailed, machineutils.PreserveMachineAnnotationValueNow, machineutils.PreserveMachineAnnotationValueAutoPreserved: // this is in case preservation process is not complete yet + // if preserve expiry time is set and is in the future, machine is already preserved + if machine.Status.CurrentStatus.PreserveExpiryTime.After(time.Now()) { + klog.V(3).Infof("Failed machine %q is preserved until %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) return false - case machineutils.PreserveMachineAnnotationValueFalse: - return true - default: - return true } + klog.V(3).Infof("Preservation of failed machine %q has timed out at %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime) + return true } // manageAutoPreservationOfFailedMachines annotates failed machines with preserve=auto-preserved annotation // to trigger preservation of the machines, by the machine controller, up to the limit defined in the // MachineSet's AutoPreserveFailedMachineMax field. If the AutoPreserveFailedMachineMax limit is breached, it removes the preserve=auto-preserved annotation from the oldest annotated machines. -func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context, machines []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) []*v1alpha1.Machine { +func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context, machines []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) ([]*v1alpha1.Machine, error) { // TODO@thiyyakat: if preservation is to be honoured across updates, capacity remaining should consider machines in all machinesets autoPreservationCapacityRemaining := machineSet.Spec.AutoPreserveFailedMachineMax - machineSet.Status.AutoPreserveFailedMachineCount if autoPreservationCapacityRemaining == 0 { // no capacity remaining, nothing to do - return machines + return machines, nil } else if autoPreservationCapacityRemaining < 0 { // when autoPreserveFailedMachineMax is decreased, it can be negative. numStillExceeding := c.stopAutoPreservationForMachines(ctx, machines, int(-autoPreservationCapacityRemaining)) if numStillExceeding > 0 { klog.V(2).Infof("Attempted to decrease count of auto-preserved machines, but there are still %d violations of AutoPreserveFailedMachineMax.", numStillExceeding) } - return machines + return machines, nil } + var autoPreservationCandidates []*v1alpha1.Machine var others []*v1alpha1.Machine for _, m := range machines { @@ -955,24 +952,50 @@ func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context, others = append(others, m) } } + sort.Slice(autoPreservationCandidates, func(i, j int) bool { return autoPreservationCandidates[i].CreationTimestamp.After(autoPreservationCandidates[j].CreationTimestamp.Time) }) - for index, m := range autoPreservationCandidates { + + var errs []error + for index, machine := range autoPreservationCandidates { if autoPreservationCapacityRemaining == 0 { break } - klog.V(2).Infof("Annotating failed machine %q for auto-preservation as part of machine set %q", m.Name, machineSet.Name) - updatedMachine, err := machineutils.UpdateMachineWithRetries(ctx, c.controlMachineClient.Machines(m.Namespace), c.machineLister, m.Namespace, m.Name, addAutoPreserveAnnotationOnMachine) + + klog.V(2).Infof("Annotating failed machine %q for auto-preservation and setting PreserveExpiryTime as part of machine set %q", machine.Name, machineSet.Name) + annotatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + if m.Annotations == nil { + m.Annotations = make(map[string]string) + } + m.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValueAutoPreserved + return nil + }, true) + if err != nil { + klog.Errorf("could not annotate machine %q for auto-preservation: %v", machine.Name, err) + errs = append(errs, err) + continue + } + + preservedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(annotatedMachine.Namespace), annotatedMachine, func(m *v1alpha1.Machine) error { + if annotatedMachine.Spec.MachineConfiguration != nil && annotatedMachine.Spec.MachineConfiguration.MachinePreserveTimeout != nil { + m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(annotatedMachine.Spec.MachineConfiguration.MachinePreserveTimeout.Duration)} + } else { + m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.safetyOptions.MachinePreserveTimeout.Duration)} + } + return nil + }, true, "status") if err != nil { - klog.V(2).Infof("Error annotating machine %q for auto-preservation: %v", m.Name, err) - // since addAutoPreserveAnnotation uses retries internally, on error we can continue with other machines + klog.Errorf("could not set PreserveExpiryTime on machine %q for auto-preservation: %v", annotatedMachine.Name, err) + errs = append(errs, err) continue } - autoPreservationCandidates[index] = updatedMachine + + autoPreservationCandidates[index] = preservedMachine autoPreservationCapacityRemaining-- } - return append(autoPreservationCandidates, others...) + + return append(autoPreservationCandidates, others...), errors.Join(errs...) } func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machines []*v1alpha1.Machine, numToStop int) int { @@ -991,14 +1014,18 @@ func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machin return autoPreservedMachines[i].CreationTimestamp.Before(&autoPreservedMachines[j].CreationTimestamp) }) } - for index, m := range autoPreservedMachines { + + for index, machine := range autoPreservedMachines { if numToStop == 0 { break } - klog.V(2).Infof("Removing auto-preservation annotation from machine %q as AutoPreserveFailedMachineMax is breached", m.Name) - updatedMachine, err := machineutils.UpdateMachineWithRetries(ctx, c.controlMachineClient.Machines(m.Namespace), c.machineLister, m.Namespace, m.Name, removeAutoPreserveAnnotationFromMachine) + klog.V(2).Infof("Removing auto-preservation annotation from machine %q as AutoPreserveFailedMachineMax is breached", machine.Name) + updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + delete(m.Annotations, machineutils.PreserveMachineAnnotationKey) + return nil + }, true) if err != nil { - klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValueAutoPreserved, m.Name, err) + klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValueAutoPreserved, machine.Name, err) continue } autoPreservedMachines[index] = updatedMachine @@ -1006,36 +1033,3 @@ func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machin } return numToStop } - -func addAutoPreserveAnnotationOnMachine(machineToUpdate *v1alpha1.Machine) error { - if machineToUpdate.Annotations == nil { - machineToUpdate.Annotations = make(map[string]string) - } - machineToUpdate.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValueAutoPreserved - return nil -} - -func removeAutoPreserveAnnotationFromMachine(machineToUpdate *v1alpha1.Machine) error { - delete(machineToUpdate.Annotations, machineutils.PreserveMachineAnnotationKey) - return nil -} - -func (c *controller) findEffectivePreserveValue(machine *v1alpha1.Machine) (string, error) { - var nodeAnnotationValue, machineAnnotationValue, lANodeAnnotationValue string - machineAnnotationValue = machine.Annotations[machineutils.PreserveMachineAnnotationKey] - lANodeAnnotationValue = machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] - nodeName := machine.Labels[v1alpha1.NodeLabelKey] - if nodeName != "" { - node, err := c.nodeLister.Get(nodeName) - if err != nil { - klog.Errorf("error fetching node %q for machine %q: %v", nodeName, machine.Name, err) - return "", err - } - nodeAnnotationValue = node.Annotations[machineutils.PreserveMachineAnnotationKey] - } - if nodeAnnotationValue == "" && lANodeAnnotationValue == "" { - return machineAnnotationValue, nil - } else { - return nodeAnnotationValue, nil - } -} diff --git a/pkg/controller/machineset_test.go b/pkg/controller/machineset_test.go index 640606878e..061f5cedd3 100644 --- a/pkg/controller/machineset_test.go +++ b/pkg/controller/machineset_test.go @@ -2156,7 +2156,8 @@ var _ = Describe("machineset", func() { waitForCacheSync(stop, c) machinesList := []*machinev1.Machine{testMachine1, testMachine2, testMachine3, testMachine4} machinesList = append(machinesList, tc.setup.additionalMachines...) - c.manageAutoPreservationOfFailedMachines(context.TODO(), machinesList, testMachineSet) + _, err := c.manageAutoPreservationOfFailedMachines(context.TODO(), machinesList, testMachineSet) + Expect(err).To(BeNil()) waitForCacheSync(stop, c) updatedMachine1, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine1.Name, metav1.GetOptions{}) updatedMachine2, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine2.Name, metav1.GetOptions{}) @@ -2330,60 +2331,6 @@ var _ = Describe("machineset", func() { result: false, }, }), - Entry("should return true if machine is annotated with preserve=false", testCase{ - setup: setup{ - machineAnnotationValue: machineutils.PreserveMachineAnnotationValueFalse, - nodeName: "test-node", - }, - expect: expect{ - result: true, - }, - }), - Entry("should return true if node is annotated with preserve=false", testCase{ - setup: setup{ - nodeAnnotationValue: machineutils.PreserveMachineAnnotationValueFalse, - nodeName: "test-node", - }, - expect: expect{ - result: true, - }, - }), - Entry("should return false if machine is annotated with preserve=now, and node has not been annotated, and preserveExpiryTime is not yet set", testCase{ - setup: setup{ - machineAnnotationValue: machineutils.PreserveMachineAnnotationValueNow, - nodeName: "test-node", - }, - expect: expect{ - result: false, - }, - }), - Entry("should return false if node is annotated with preserve=now, and preserveExpiryTime is not yet set", testCase{ - setup: setup{ - nodeAnnotationValue: machineutils.PreserveMachineAnnotationValueNow, - nodeName: "test-node", - }, - expect: expect{ - result: false, - }, - }), - Entry("should return false if machine is annotated with preserve=when-failed, and node has not been annotated", testCase{ - setup: setup{ - machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - nodeName: "test-node", - }, - expect: expect{ - result: false, - }, - }), - Entry("should return false if node is annotated with preserve=when-failed", testCase{ - setup: setup{ - nodeAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - nodeName: "test-node", - }, - expect: expect{ - result: false, - }, - }), Entry("should return true if preservation has timed out", testCase{ setup: setup{ preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(-1 * time.Second)}, @@ -2394,17 +2341,6 @@ var _ = Describe("machineset", func() { result: true, }, }), - Entry("should return true if laNodePreserveValue is not empty, machineAnnotationValue is not empty and nodeAnnotationValue is empty, indicating that node Annotation Value was deleted", testCase{ - setup: setup{ - laNodeAnnotationValue: machineutils.PreserveMachineAnnotationValueNow, - machineAnnotationValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - nodeName: "test-node", - nodeAnnotationValue: "", - }, - expect: expect{ - result: true, - }, - }), ) }) }) diff --git a/pkg/controller/machineset_util.go b/pkg/controller/machineset_util.go index a940c982f3..8c212d33bc 100644 --- a/pkg/controller/machineset_util.go +++ b/pkg/controller/machineset_util.go @@ -83,26 +83,22 @@ func GetMachineSetHash(is *v1alpha1.MachineSet, uniquifier *int32) (string, erro // syncMachinesNodeTemplates updates all machines in the given machineList with the new nodeTemplate if required. func (c *controller) syncMachinesNodeTemplates(ctx context.Context, machineList []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) error { - - controlClient := c.controlMachineClient - machineLister := c.machineLister - - for _, machine := range machineList { + for i, machine := range machineList { // Ignore inactive Machines. if !machineutils.IsMachineActive(machine) { continue } - nodeTemplateChanged := copyMachineSetNodeTemplatesToMachines(machineSet, machine) // Only sync the machine that doesn't already have the latest nodeTemplate. - if nodeTemplateChanged { - _, err := machineutils.UpdateMachineWithRetries(ctx, controlClient.Machines(machine.Namespace), machineLister, machine.Namespace, machine.Name, - func(_ *v1alpha1.Machine) error { - return nil - }) + if nodeTemplateOutOfSync(machineSet, machine) { + updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + m.Spec.NodeTemplateSpec = machineSet.Spec.Template.Spec.NodeTemplateSpec + return nil + }, true) if err != nil { return fmt.Errorf("error in updating nodeTemplateSpec to machine %q: %v", machine.Name, err) } + machineList[i] = updatedMachine klog.V(2).Infof("Updated machine %s/%s of MachineSet %s/%s with latest nodeTemplate.", machine.Namespace, machine.Name, machineSet.Namespace, machineSet.Name) } } @@ -111,96 +107,66 @@ func (c *controller) syncMachinesNodeTemplates(ctx context.Context, machineList // syncMachinesClassKind updates all machines in the given machineList with the new classKind if required. func (c *controller) syncMachinesClassKind(ctx context.Context, machineList []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) error { - - controlClient := c.controlMachineClient - machineLister := c.machineLister - - for _, machine := range machineList { - classKindChanged := copyMachineSetClassKindToMachines(machineSet, machine) + for i, machine := range machineList { // Only sync the machine that doesn't already have the matching classKind. - if classKindChanged { - _, err := machineutils.UpdateMachineWithRetries(ctx, controlClient.Machines(machine.Namespace), machineLister, machine.Namespace, machine.Name, - func(_ *v1alpha1.Machine) error { - return nil - }) + if classKindOutOfSync(machineSet, machine) { + updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + m.Spec.Class.Kind = machineSet.Spec.Template.Spec.Class.Kind + return nil + }, true) if err != nil { return fmt.Errorf("error in updating classKind to machine %q: %v", machine.Name, err) } + machineList[i] = updatedMachine klog.V(2).Infof("Updated Machine %s/%s of MachineSet %s/%s with latest classKind.", machine.Namespace, machine.Name, machineSet.Namespace, machineSet.Name) } } return nil } -// copyMachineSetNodeTemplatesToMachines copies machineset's nodeTemplate to machine's nodeTemplate, -// and returns true if machine's nodeTemplate is changed. -// Note that apply and revision nodeTemplates are not copied. -func copyMachineSetNodeTemplatesToMachines(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { - machineSetNodeTemplateCopy := machineset.Spec.Template.Spec.NodeTemplateSpec.DeepCopy() - machineNodeTemplateCopy := machine.Spec.NodeTemplateSpec.DeepCopy() +// nodeTemplateOutOfSync returns true if machine's nodeTemplate is changed. +func nodeTemplateOutOfSync(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { + machineSetNodeTemplate := machineset.Spec.Template.Spec.NodeTemplateSpec + machineNodeTemplate := machine.Spec.NodeTemplateSpec - isNodeTemplateChanged := !(apiequality.Semantic.DeepEqual(machineSetNodeTemplateCopy, machineNodeTemplateCopy)) - - if isNodeTemplateChanged { - machine.Spec.NodeTemplateSpec = machineset.Spec.Template.Spec.NodeTemplateSpec - } - return isNodeTemplateChanged + return !(apiequality.Semantic.DeepEqual(machineSetNodeTemplate, machineNodeTemplate)) } // syncMachinesConfig updates all machines in the given machineList with the new config if required. func (c *controller) syncMachinesConfig(ctx context.Context, machineList []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) error { - - controlClient := c.controlMachineClient - machineLister := c.machineLister - - for _, machine := range machineList { + for i, machine := range machineList { // Ignore inactive Machines. if !machineutils.IsMachineActive(machine) { continue } - configChanged := copyMachineSetConfigToMachines(machineSet, machine) // Only sync the machine that doesn't already have the latest config. - if configChanged { - _, err := machineutils.UpdateMachineWithRetries(ctx, controlClient.Machines(machine.Namespace), machineLister, machine.Namespace, machine.Name, - func(_ *v1alpha1.Machine) error { - return nil - }) + if configOutOfSync(machineSet, machine) { + updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + m.Spec.MachineConfiguration = machineSet.Spec.Template.Spec.MachineConfiguration + return nil + }, true) if err != nil { return fmt.Errorf("error in updating MachineConfig to machine %q: %v", machine.Name, err) } + machineList[i] = updatedMachine klog.V(2).Infof("Updated machine %s/%s of MachineSet %s/%s with latest config.", machine.Namespace, machine.Name, machineSet.Namespace, machineSet.Name) } } return nil } -// copyMachineSetConfigToMachines copies machineset's config to machine's config, -// and returns true if machine's config is changed. -// Note that apply and revision config are not copied. -func copyMachineSetConfigToMachines(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { - isConfigChanged := false - - machineSetConfigCopy := machineset.Spec.Template.Spec.MachineConfiguration.DeepCopy() - machineConfigCopy := machine.Spec.MachineConfiguration.DeepCopy() - - isConfigChanged = !(apiequality.Semantic.DeepEqual(machineSetConfigCopy, machineConfigCopy)) +// configOutOfSync returns true if machine's config is changed. +func configOutOfSync(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { + machineSetConfig := machineset.Spec.Template.Spec.MachineConfiguration + machineConfig := machine.Spec.MachineConfiguration - if isConfigChanged { - machine.Spec.MachineConfiguration = machineset.Spec.Template.Spec.MachineConfiguration - } - return isConfigChanged + return !(apiequality.Semantic.DeepEqual(machineSetConfig, machineConfig)) } -// copyMachineSetClassKindToMachines copies machineset's class.Kind to machine's class.Kind, -// and returns true if machine's class.Kind is changed. -func copyMachineSetClassKindToMachines(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { - if machineset.Spec.Template.Spec.Class.Kind != machine.Spec.Class.Kind { - machine.Spec.Class.Kind = machineset.Spec.Template.Spec.Class.Kind - return true - } - - return false +// classKindOutOfSync returns true if machine's class.Kind is changed. +func classKindOutOfSync(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool { + return machineset.Spec.Template.Spec.Class.Kind != machine.Spec.Class.Kind } func logMachinesToDelete(machines []*v1alpha1.Machine) { diff --git a/pkg/options/types.go b/pkg/options/types.go index 40e3b3d411..5cf12b180a 100644 --- a/pkg/options/types.go +++ b/pkg/options/types.go @@ -78,6 +78,9 @@ type SafetyOptions struct { // Period (in durartion) used to poll for overshooting // of machine objects backing a machineSet by safety controller MachineSafetyOvershootingPeriod metav1.Duration + // Timeout (in duration) used while preserving a machine, + // beyond which preservation is stopped + MachinePreserveTimeout metav1.Duration } // LeaderElectionConfiguration defines the configuration of leader election diff --git a/pkg/util/provider/machinecontroller/machine.go b/pkg/util/provider/machinecontroller/machine.go index 96bea9b72e..3cfce3fce5 100644 --- a/pkg/util/provider/machinecontroller/machine.go +++ b/pkg/util/provider/machinecontroller/machine.go @@ -292,7 +292,7 @@ func (c *controller) reconcileClusterMachine(ctx context.Context, machine *v1alp } } - retry, err = c.manageMachinePreservation(ctx, machine) + machine, retry, err = c.manageMachinePreservation(ctx, machine) if err != nil { return retry, err } @@ -442,7 +442,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque // To avoid this scenario, check if the name of the node is equal to the machine name before marking them as stale. // Ideally, the check should compare that the providerID of the machine and the node are matching, but since this is // not enforced for MCM extensions the current best option is to compare the names. - if _, err := c.nodeLister.Get(nodeName); err == nil && nodeName != machineName { + if node, err := c.nodeLister.Get(nodeName); err == nil && nodeName != machineName { // mark the machine obj as `Failed` klog.Errorf("Stale node obj with name %q for machine %q has been found. Hence marking the created VM for deletion to trigger a new machine creation.", nodeName, machine.Name) @@ -458,13 +458,21 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque } _, err := c.driver.DeleteMachine(ctx, deleteMachineRequest) - if err != nil { klog.V(2).Infof("VM deletion in context of stale node obj failed for machine %q, will be retried. err=%q", machine.Name, err.Error()) } else { klog.V(2).Infof("VM successfully deleted in context of stale node obj for machine %q", machine.Name) } + machineCurrentStatus := v1alpha1.CurrentStatus{ + Phase: v1alpha1.MachineFailed, + LastUpdateTime: metav1.Now(), + } + + if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed { + machineCurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)} + } + // machine obj marked Failed for double security updateRetryPeriod, updateErr := c.machineStatusUpdate( ctx, @@ -475,10 +483,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque Type: v1alpha1.MachineOperationCreate, LastUpdateTime: metav1.Now(), }, - v1alpha1.CurrentStatus{ - Phase: v1alpha1.MachineFailed, - LastUpdateTime: metav1.Now(), - }, + machineCurrentStatus, machine.Status.LastKnownState, ) @@ -792,20 +797,9 @@ func (c *controller) isCreationProcessing(machine *v1alpha1.Machine) bool { Machine Preservation operations */ -// preserveStateInfo encapsulates the preservation annotation values found -// on the machine and node objects, along with the effective preservation value for the machine -// and the last applied node preserve value by MCM. -type preserveStateInfo struct { - nodeAnnotated bool - machineAnnotated bool - nodeValue string - machineValue string - lastAppliedNodeValue string - preserveExpiryTimeSet bool -} - // manageMachinePreservation manages machine preservation based on the preserve annotation values on the node and machine objects. -func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1alpha1.Machine) (retry machineutils.RetryPeriod, err error) { +func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1alpha1.Machine) (updatedMachine *v1alpha1.Machine, retry machineutils.RetryPeriod, err error) { + updatedMachine = machine defer func() { if err != nil { if apierrors.IsNotFound(err) { @@ -822,69 +816,46 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a } }() nodeName := machine.Labels[v1alpha1.NodeLabelKey] - // We buffer the error returned here until we can tell if the machine is preservation-bound. - preserveInfo, getErr := c.getPreserveStateInfo(machine) - if preserveInfo.machineAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.machineValue) { - // If machine is annotated incorrectly, log and proceed as though machine is not annotated. - klog.Warningf("Preserve annotation %q=%q on machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.machineValue, machine.Name) - preserveInfo.machineAnnotated = false - preserveInfo.machineValue = "" - } - if preserveInfo.nodeAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.nodeValue) { - klog.Warningf("Preserve annotation %q=%q on node %q backing machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.nodeValue, nodeName, machine.Name) - return + node, err := c.nodeLister.Get(nodeName) + if err != nil { + klog.V(3).Infof("Error fetching node %q . Will check the machine %q for annotation:%q", nodeName, machine.Name, machineutils.PreserveMachineAnnotationKey) } - preservationBound := isMachinePreservationBound(&preserveInfo) - if !preservationBound { - // We clear the error here to prevent preservation logic from interfering with non-preservation-bound machines. - err = nil + + preserveAnnotationValue, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine) + if !shouldHandlePreservation { return - } else if getErr != nil { - if !apierrors.IsNotFound(getErr) { - err = getErr - return - } - klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name) - err = nil } - // Note: when the backing node cannot be found, we assume the machine's annotation value needs to be enforced to enable - // preservation of the machine object. - effectivePreserveValue := getEffectivePreservationAnnotations(&preserveInfo, getErr) - var removeAnnotations bool clone := machine.DeepCopy() - switch effectivePreserveValue { - // effectivePreserveValue == "" implies the preservation annotation was deleted to indicate that + switch preserveAnnotationValue { + // preserveAnnotationValue == "" implies the preservation annotation was deleted to indicate that // preservation must be stopped case "", machineutils.PreserveMachineAnnotationValueFalse: - clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations) + clone, err = c.stopPreservationIfActive(ctx, clone, false) case machineutils.PreserveMachineAnnotationValueWhenFailed: // on timing out, remove preserve annotation to prevent incorrect re-preservation if machineutils.IsMachinePreservationExpired(clone) { - removeAnnotations = true - clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations) + clone, err = c.stopPreservationIfActive(ctx, clone, true) } else if !machineutils.IsMachineFailed(clone) { - clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations) + clone, err = c.stopPreservationIfActive(ctx, clone, false) } else { - clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue) + clone, err = c.preserveMachine(ctx, clone, preserveAnnotationValue) } case machineutils.PreserveMachineAnnotationValueNow: if machineutils.IsMachinePreservationExpired(clone) { // on timing out, remove preserve annotation to prevent incorrect re-preservation - removeAnnotations = true - clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations) + clone, err = c.stopPreservationIfActive(ctx, clone, true) } else { - clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue) + clone, err = c.preserveMachine(ctx, clone, preserveAnnotationValue) } case machineutils.PreserveMachineAnnotationValueAutoPreserved: if !machineutils.IsMachineFailed(clone) || machineutils.IsMachinePreservationExpired(clone) { // To prevent incorrect re-preservation of a recovered, previously auto-preserved machine on future failures // (since the autoPreserveFailedMachineCount maintained by the machineSetController, may have changed), // in addition to stopping preservation, we also remove the preservation annotation on the machine. - removeAnnotations = true - clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations) + clone, err = c.stopPreservationIfActive(ctx, clone, true) } else { - clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue) + clone, err = c.preserveMachine(ctx, clone, preserveAnnotationValue) } } if err != nil { @@ -907,97 +878,61 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a } } - if shouldAnnotationsBeUpdatedOnMachine(removeAnnotations, &preserveInfo) { - err = c.updatePreserveAnnotationOnMachine(ctx, preserveInfo.nodeValue, clone) - } - return -} - -// getEffectivePreservationAnnotations returns the effective preservation value. -// -// If there is no active node annotation AND no previously-applied node annotation, -// enforce machine's preserve annotation. -// Otherwise, the node annotation takes precedence (even if now empty/removed). -// -// lastAppliedNodeValue is required to handle the following scenario: -// -// T1: Node and Machine both have the same annotation with the same value. (MCM is up and running). -// T2 (T2 > T1): MCM went down. -// T3 (T3 > T2): Node annotation was removed. -// T4 (T4 > T3): MCM came back up. -// At T4 it sees a Node with no preserve annotation but a Machine with a preserve annotation. -// It continues to preserve the machine. -func getEffectivePreservationAnnotations(info *preserveStateInfo, getPreserveStateErr error) string { - // If the node cannot be found, nodeValue is "". - // In this case, we want the machine's annotation value to be enforced. - if apierrors.IsNotFound(getPreserveStateErr) { - return info.machineValue - } - // If there is no active node annotation AND no previously-applied node annotation, - // enforce machine's preserve annotation. - // Otherwise, the node annotation takes precedence (even if now empty/removed). - if info.nodeValue == "" && info.lastAppliedNodeValue == "" { - return info.machineValue - } - return info.nodeValue -} - -func isMachinePreservationBound(info *preserveStateInfo) bool { - // if machine has no preservation state, the machine is not preservation-bound - if !info.preserveExpiryTimeSet && !info.nodeAnnotated && !info.machineAnnotated && info.lastAppliedNodeValue == "" { - return false - } - return true -} - -func (c *controller) getPreserveStateInfo(machine *v1alpha1.Machine) (preserveStateInfo, error) { - var info preserveStateInfo - if machine.Annotations != nil { - info.machineValue, info.machineAnnotated = machine.Annotations[machineutils.PreserveMachineAnnotationKey] - info.lastAppliedNodeValue = machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] - } - if machine.Status.CurrentStatus.PreserveExpiryTime != nil { - info.preserveExpiryTimeSet = true - } - nodeName := machine.Labels[v1alpha1.NodeLabelKey] - if nodeName != "" { - node, err := c.nodeLister.Get(nodeName) + if node != nil { + updatedMachine, err = c.updatePreserveAnnotationOnMachine(ctx, node.Annotations[machineutils.PreserveMachineAnnotationKey], clone) if err != nil { - return info, err + updatedMachine = clone } - info.nodeValue, info.nodeAnnotated = node.Annotations[machineutils.PreserveMachineAnnotationKey] - } - return info, nil -} - -// shouldAnnotationsBeUpdatedOnMachine returns true when the machine's annotation tracking needs -// to be synced after a preservation action. -func shouldAnnotationsBeUpdatedOnMachine(removeAnnotations bool, preserveInfo *preserveStateInfo) bool { - // annotations were already removed by stopPreservationIfActive — nothing left to sync - if removeAnnotations { - return false - } - // node annotation is not in control — machine annotation prevails, no sync needed - if !preserveInfo.nodeAnnotated && preserveInfo.lastAppliedNodeValue == "" { - return false - } - // node value is unchanged and machine has no annotation to clear — nothing has changed - if preserveInfo.nodeValue == preserveInfo.lastAppliedNodeValue && !preserveInfo.machineAnnotated { - return false + } else { + updatedMachine = clone } - return true + return } // updatePreserveAnnotationOnMachine clears the machine's PreserveMachineAnnotationKey and sets // [machineutils.LastAppliedNodePreserveValueAnnotationKey] to nodeValue. -func (c *controller) updatePreserveAnnotationOnMachine(ctx context.Context, nodeValue string, machine *v1alpha1.Machine) error { - clone := machine.DeepCopy() - if clone.Annotations == nil { - clone.Annotations = make(map[string]string) +func (c *controller) updatePreserveAnnotationOnMachine(ctx context.Context, nodeValue string, machine *v1alpha1.Machine) (*v1alpha1.Machine, error) { + if nodeValue == "" { + if _, exists := machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey]; !exists { + return machine, nil + } + klog.V(3).Infof( + "Since node %q 's annotation:%q was removed, removing machine %q 's annotation:%q", + machine.Labels[v1alpha1.NodeLabelKey], + machineutils.PreserveMachineAnnotationKey, + machine.Name, + machineutils.LastAppliedNodePreserveValueAnnotationKey, + ) } else { - delete(clone.Annotations, machineutils.PreserveMachineAnnotationKey) + klog.V(3).Infof( + "Syncing node %q 's annotation:%q=%q to its machine %q 's annotation:%q=%q", + machine.Labels[v1alpha1.NodeLabelKey], + machineutils.PreserveMachineAnnotationKey, + nodeValue, + machine.Name, + machineutils.LastAppliedNodePreserveValueAnnotationKey, + nodeValue, + ) } - clone.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] = nodeValue - _, err := c.controlMachineClient.Machines(clone.Namespace).Update(ctx, clone, metav1.UpdateOptions{}) - return err + klog.V(3).Infof( + "Removing machine %q 's annotation:%q=%q as node %q 's has annotation:%q=%q", + machine.Name, + machineutils.PreserveMachineAnnotationKey, + machine.Annotations[machineutils.PreserveMachineAnnotationKey], + machine.Labels[v1alpha1.NodeLabelKey], + machineutils.PreserveMachineAnnotationKey, + nodeValue, + ) + return machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error { + if m.Annotations == nil { + m.Annotations = make(map[string]string) + } + if nodeValue == "" { + delete(m.Annotations, machineutils.LastAppliedNodePreserveValueAnnotationKey) + } else { + m.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] = nodeValue + } + delete(m.Annotations, machineutils.PreserveMachineAnnotationKey) + return nil + }, true) } diff --git a/pkg/util/provider/machinecontroller/machine_test.go b/pkg/util/provider/machinecontroller/machine_test.go index 1723e9a0a6..9949ca2a4c 100644 --- a/pkg/util/provider/machinecontroller/machine_test.go +++ b/pkg/util/provider/machinecontroller/machine_test.go @@ -1464,6 +1464,85 @@ var _ = Describe("machine", func() { }, }), ) + + DescribeTable("##PreserveExpiryTime on stale-node transition to Failed", + func(preserveAnnotation string, expectExpirySet bool) { + stop := make(chan struct{}) + defer close(stop) + + annotations := map[string]string{} + if preserveAnnotation != "" { + annotations[machineutils.PreserveMachineAnnotationKey] = preserveAnnotation + } + + machines := newMachines(1, &v1alpha1.MachineTemplateSpec{ + ObjectMeta: *newObjectMeta(objMeta, 0), + Spec: v1alpha1.MachineSpec{ + Class: v1alpha1.ClassSpec{Kind: "MachineClass", Name: "machine-0"}, + }, + }, nil, nil, annotations, nil, true, metav1.Now()) + + secrets := []*corev1.Secret{{ + ObjectMeta: *newObjectMeta(objMeta, 0), + Data: map[string][]byte{"userData": []byte("test")}, + }} + machineClasses := []*v1alpha1.MachineClass{{ + ObjectMeta: *newObjectMeta(objMeta, 0), + SecretRef: newSecretReference(objMeta, 0), + }} + nodes := []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "fakeNode-0"}}} + + machineObjects := []runtime.Object{} + for _, o := range machineClasses { + machineObjects = append(machineObjects, o) + } + for _, o := range machines { + machineObjects = append(machineObjects, o) + } + controlCoreObjects := []runtime.Object{} + for _, o := range secrets { + controlCoreObjects = append(controlCoreObjects, o) + } + targetCoreObjects := []runtime.Object{} + for _, o := range nodes { + targetCoreObjects = append(targetCoreObjects, o) + } + + fakedriver := driver.NewFakeDriver(false, "fakeID-0", "fakeNode-0", "", nil, nil, nil) + controller, trackers := createController(stop, objMeta.Namespace, machineObjects, controlCoreObjects, targetCoreObjects, fakedriver, false) + defer trackers.Stop() + controller.safetyOptions.MachinePreserveTimeout = metav1.Duration{Duration: 24 * time.Hour} + waitForCacheSync(stop, controller) + + machine, err := controller.controlMachineClient.Machines(objMeta.Namespace).Get(context.TODO(), "machine-0", metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + machineClass, err := controller.controlMachineClient.MachineClasses(objMeta.Namespace).Get(context.TODO(), machine.Spec.Class.Name, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + secret, err := controller.controlCoreClient.CoreV1().Secrets(objMeta.Namespace).Get(context.TODO(), machineClass.SecretRef.Name, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + + _, _ = controller.triggerCreationFlow(context.TODO(), &driver.CreateMachineRequest{ + Machine: machine, + MachineClass: machineClass, + Secret: secret, + }) + + updated, getErr := controller.controlMachineClient.Machines(objMeta.Namespace).Get(context.TODO(), machine.Name, metav1.GetOptions{}) + Expect(getErr).To(BeNil()) + Expect(updated.Status.CurrentStatus.Phase).To(Equal(v1alpha1.MachineFailed)) + if expectExpirySet { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).NotTo(BeNil()) + Expect(updated.Status.CurrentStatus.PreserveExpiryTime.After(time.Now())).To(BeTrue()) + } else { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).To(BeNil()) + } + }, + Entry("preserve=when-failed: PreserveExpiryTime must be set", machineutils.PreserveMachineAnnotationValueWhenFailed, true), + Entry("preserve=now: PreserveExpiryTime must NOT be set by stale-node path", machineutils.PreserveMachineAnnotationValueNow, false), + Entry("preserve=false: PreserveExpiryTime must NOT be set", machineutils.PreserveMachineAnnotationValueFalse, false), + Entry("preserve=auto-preserved: PreserveExpiryTime must NOT be set (set later after auto-preserve selection)", machineutils.PreserveMachineAnnotationValueAutoPreserved, false), + Entry("no preserve annotation: PreserveExpiryTime must NOT be set", "", false), + ) }) Describe("#triggerDeletionFlow", func() { @@ -3956,146 +4035,7 @@ var _ = Describe("machine", func() { }), ) }) - Describe("#getEffectivePreservationAnnotations", func() { - type setup struct { - nodeAnnotationValue string - machineAnnotations map[string]string - } - type expect struct { - effectivePreserveValue string - machineAnnotations map[string]string - } - - type testCase struct { - setup setup - expect expect - } - - DescribeTable("getEffectivePreservationAnnotations scenarios", - func(tc testCase) { - info := &preserveStateInfo{ - nodeValue: tc.setup.nodeAnnotationValue, - machineValue: tc.setup.machineAnnotations[machineutils.PreserveMachineAnnotationKey], - lastAppliedNodeValue: tc.setup.machineAnnotations[machineutils.LastAppliedNodePreserveValueAnnotationKey], - } - preserveValue := getEffectivePreservationAnnotations(info, nil) - Expect(preserveValue).To(Equal(tc.expect.effectivePreserveValue)) - }, - Entry("when node is not annotated and laNodeAnnotationValue is empty, should return machine's annotation value and empty string", testCase{ - setup: setup{ - nodeAnnotationValue: "", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "A", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - expect: expect{ - effectivePreserveValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "A", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - }), - Entry("when neither node nor machine is not annotated and laNodeAnnotationValue is empty, should return two empty strings", testCase{ - setup: setup{ - nodeAnnotationValue: "", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - expect: expect{ - effectivePreserveValue: "", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - }), - Entry("when neither node nor machine is annotated and laNodeAnnotationValue is \"A\", should return two empty strings", testCase{ - setup: setup{ - nodeAnnotationValue: "", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - expect: expect{ - effectivePreserveValue: "", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - }), - Entry("when node is annotated, laNodeAnnotationValue is empty, and machine is not annotated, should return node's annotation value as effective value and last applied value", testCase{ - setup: setup{ - nodeAnnotationValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - expect: expect{ - effectivePreserveValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - }), - Entry("when node is annotated, laNodeAnnotationValue is empty, and machine is annotated differently, should return node's annotation value as effective value and last applied value", testCase{ - setup: setup{ - nodeAnnotationValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "B", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "", - }, - }, - expect: expect{ - effectivePreserveValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - }), - Entry("when node, machine annotation values and laNodeAnnotationValue are the same, should return node's annotation value as effective value and last applied value", testCase{ - setup: setup{ - nodeAnnotationValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "A", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - expect: expect{ - effectivePreserveValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - }), - Entry("when node, machine annotation values are the same and laNodeAnnotationValue differs, should return node's annotation value as effective value and last applied value", testCase{ - setup: setup{ - nodeAnnotationValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "A", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "B", - }, - }, - expect: expect{ - effectivePreserveValue: "A", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: "", - machineutils.LastAppliedNodePreserveValueAnnotationKey: "A", - }, - }, - }), - ) - }) Describe("#manageMachinePreservation", func() { type setup struct { machineAnnotationValue string @@ -4185,7 +4125,7 @@ var _ = Describe("machine", func() { c, trackers := createController(stop, testNamespace, controlMachineObjects, nil, targetCoreObjects, nil, false) defer trackers.Stop() waitForCacheSync(stop, c) - retry, err := c.manageMachinePreservation(context.TODO(), machine) + _, retry, err := c.manageMachinePreservation(context.TODO(), machine) Expect(retry).To(Equal(tc.expect.retry)) if tc.expect.err != nil { @@ -4578,7 +4518,7 @@ var _ = Describe("machine", func() { defer trackers.Stop() waitForCacheSync(stop, c) - retry, err := c.manageMachinePreservation(context.TODO(), machine) + _, retry, err := c.manageMachinePreservation(context.TODO(), machine) Expect(err).ToNot(HaveOccurred()) Expect(retry).To(Equal(machineutils.LongRetry)) @@ -4602,362 +4542,6 @@ var _ = Describe("machine", func() { ) }) - Describe("#isMachinePreservationBound", func() { - type setup struct { - machineAnnotations map[string]string - preserveExpiryTime *metav1.Time - nodeName string - nodeAnnotations map[string]string - includeNode bool - } - type expect struct { - bound bool - err error - } - type testCase struct { - setup setup - expect expect - } - - DescribeTable("isMachinePreservationBound scenarios", - func(tc testCase) { - stop := make(chan struct{}) - defer close(stop) - - machine := &v1alpha1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNamespace, - Name: "m1", - Labels: map[string]string{v1alpha1.NodeLabelKey: tc.setup.nodeName}, - Annotations: tc.setup.machineAnnotations, - }, - Status: v1alpha1.MachineStatus{ - CurrentStatus: v1alpha1.CurrentStatus{ - PreserveExpiryTime: tc.setup.preserveExpiryTime, - }, - }, - } - - var targetCoreObjects []runtime.Object - if tc.setup.includeNode { - targetCoreObjects = append(targetCoreObjects, &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: tc.setup.nodeName, - Annotations: tc.setup.nodeAnnotations, - }, - }) - } - - c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, targetCoreObjects, nil, false) - defer trackers.Stop() - waitForCacheSync(stop, c) - - preserveInfo, err := c.getPreserveStateInfo(machine) - if tc.expect.err != nil { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(Equal(tc.expect.err.Error())) - } else { - Expect(err).ToNot(HaveOccurred()) - } - bound := isMachinePreservationBound(&preserveInfo) - Expect(bound).To(Equal(tc.expect.bound)) - }, - Entry("machine has no annotations, no preserveExpiryTime, and node has no preservation annotation", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - }, - expect: expect{bound: false}, - }), - Entry("machine has no node label and no other preservation markers", testCase{ - setup: setup{ - nodeName: "", - }, - expect: expect{bound: false}, - }), - Entry("machine has preserve annotation set", testCase{ - setup: setup{ - nodeName: "node-1", - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - includeNode: true, - }, - expect: expect{bound: true}, - }), - Entry("machine has a non-nil preserveExpiryTime", testCase{ - setup: setup{ - nodeName: "node-1", - preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(1 * time.Hour)}, - includeNode: true, - }, - expect: expect{bound: true}, - }), - Entry("machine's node has a preservation annotation", testCase{ - setup: setup{ - nodeName: "node-1", - nodeAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - includeNode: true, - }, - expect: expect{bound: true}, - }), - Entry("machine has last-applied node preserve value annotation", testCase{ - setup: setup{ - nodeName: "node-1", - machineAnnotations: map[string]string{ - machineutils.LastAppliedNodePreserveValueAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - includeNode: true, - }, - expect: expect{bound: true}, - }), - Entry("node is not found and machine has no preservation markers", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: false, - }, - expect: expect{ - bound: false, - err: fmt.Errorf("node %q not found", "node-1"), - }, - }), - ) - }) - - Describe("#getPreserveStateInfo", func() { - type setup struct { - machineAnnotations map[string]string - preserveExpiryTime *metav1.Time - nodeName string - nodeAnnotations map[string]string - includeNode bool - } - type expect struct { - machineAnnotated bool - machineValue string - nodeAnnotated bool - nodeValue string - lastAppliedNodeValue string - preserveExpiryTimeSet bool - err error - } - type testCase struct { - setup setup - expect expect - } - - DescribeTable("getPreserveStateInfo scenarios", - func(tc testCase) { - stop := make(chan struct{}) - defer close(stop) - - machine := &v1alpha1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNamespace, - Name: "m1", - Labels: map[string]string{v1alpha1.NodeLabelKey: tc.setup.nodeName}, - Annotations: tc.setup.machineAnnotations, - }, - Status: v1alpha1.MachineStatus{ - CurrentStatus: v1alpha1.CurrentStatus{ - PreserveExpiryTime: tc.setup.preserveExpiryTime, - }, - }, - } - - var targetCoreObjects []runtime.Object - if tc.setup.includeNode { - targetCoreObjects = append(targetCoreObjects, &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: tc.setup.nodeName, - Annotations: tc.setup.nodeAnnotations, - }, - }) - } - - c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, targetCoreObjects, nil, false) - defer trackers.Stop() - waitForCacheSync(stop, c) - - info, err := c.getPreserveStateInfo(machine) - if tc.expect.err != nil { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(tc.expect.err.Error())) - } else { - Expect(err).ToNot(HaveOccurred()) - } - Expect(info.machineAnnotated).To(Equal(tc.expect.machineAnnotated)) - Expect(info.machineValue).To(Equal(tc.expect.machineValue)) - Expect(info.nodeAnnotated).To(Equal(tc.expect.nodeAnnotated)) - Expect(info.nodeValue).To(Equal(tc.expect.nodeValue)) - Expect(info.lastAppliedNodeValue).To(Equal(tc.expect.lastAppliedNodeValue)) - Expect(info.preserveExpiryTimeSet).To(Equal(tc.expect.preserveExpiryTimeSet)) - }, - Entry("machine has no annotations and no node label", testCase{ - setup: setup{}, - expect: expect{}, - }), - Entry("machine has preserve annotation and node is not annotated", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - }, - expect: expect{ - machineAnnotated: true, - machineValue: machineutils.PreserveMachineAnnotationValueNow, - }, - }), - Entry("machine has last-applied node preserve value annotation but no preserve annotations on node or machine", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - machineAnnotations: map[string]string{ - machineutils.LastAppliedNodePreserveValueAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - }, - expect: expect{ - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueNow, - }, - }), - Entry("machine has a non-nil preserveExpiryTime", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - preserveExpiryTime: &metav1.Time{Time: metav1.Now().Add(1 * time.Hour)}, - }, - expect: expect{ - preserveExpiryTimeSet: true, - }, - }), - Entry("node has preserve annotation", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - nodeAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueWhenFailed, - }, - }, - expect: expect{ - nodeAnnotated: true, - nodeValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - }, - }), - Entry("node not found returns error and partial info", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: false, - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - }, - }, - expect: expect{ - machineAnnotated: true, - machineValue: machineutils.PreserveMachineAnnotationValueNow, - err: fmt.Errorf("node-1"), - }, - }), - Entry("both machine and node have preserve annotations", testCase{ - setup: setup{ - nodeName: "node-1", - includeNode: true, - machineAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueNow, - machineutils.LastAppliedNodePreserveValueAnnotationKey: machineutils.PreserveMachineAnnotationValueWhenFailed, - }, - nodeAnnotations: map[string]string{ - machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueAutoPreserved, - }, - }, - expect: expect{ - machineAnnotated: true, - machineValue: machineutils.PreserveMachineAnnotationValueNow, - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - nodeAnnotated: true, - nodeValue: machineutils.PreserveMachineAnnotationValueAutoPreserved, - }, - }), - ) - }) - - Describe("#shouldAnnotationsBeUpdatedOnMachine", func() { - type testCase struct { - removeAnnotations bool - preserveInfo *preserveStateInfo - expect bool - } - - DescribeTable("shouldAnnotationsBeUpdatedOnMachine scenarios", - func(tc testCase) { - Expect(shouldAnnotationsBeUpdatedOnMachine(tc.removeAnnotations, tc.preserveInfo)).To(Equal(tc.expect)) - }, - Entry("removeAnnotations=true: always returns false", testCase{ - removeAnnotations: true, - preserveInfo: &preserveStateInfo{nodeAnnotated: true, nodeValue: machineutils.PreserveMachineAnnotationValueNow}, - expect: false, - }), - Entry("node not annotated and no lastAppliedNodeValue: returns false", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{nodeAnnotated: false, lastAppliedNodeValue: ""}, - expect: false, - }), - Entry("node value unchanged and machine not annotated: returns false", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{ - nodeAnnotated: true, - nodeValue: machineutils.PreserveMachineAnnotationValueNow, - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueNow, - machineAnnotated: false, - }, - expect: false, - }), - Entry("node value changed: returns true", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{ - nodeAnnotated: true, - nodeValue: machineutils.PreserveMachineAnnotationValueNow, - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueWhenFailed, - machineAnnotated: false, - }, - expect: true, - }), - Entry("node value unchanged but machine is annotated: returns true", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{ - nodeAnnotated: true, - nodeValue: machineutils.PreserveMachineAnnotationValueNow, - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueNow, - machineAnnotated: true, - }, - expect: true, - }), - Entry("lastAppliedNodeValue set without node annotation: returns true (machine annotation present)", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{ - nodeAnnotated: false, - nodeValue: "", - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueNow, - machineAnnotated: true, - }, - expect: true, - }), - Entry("lastAppliedNodeValue set without node annotation and machine not annotated: returns true (node value drifted from last-applied)", testCase{ - removeAnnotations: false, - preserveInfo: &preserveStateInfo{ - nodeAnnotated: false, - nodeValue: "", - lastAppliedNodeValue: machineutils.PreserveMachineAnnotationValueNow, - machineAnnotated: false, - }, - expect: true, - }), - ) - }) - Describe("#updatePreserveAnnotationOnMachine", func() { type setup struct { machineAnnotations map[string]string @@ -4990,7 +4574,7 @@ var _ = Describe("machine", func() { defer trackers.Stop() waitForCacheSync(stop, c) - err := c.updatePreserveAnnotationOnMachine(context.TODO(), tc.nodeValue, machine) + _, err := c.updatePreserveAnnotationOnMachine(context.TODO(), tc.nodeValue, machine) if tc.expect.err != nil { Expect(err).To(HaveOccurred()) } else { diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index ed8ba3bdee..0c18ec86c1 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1142,6 +1142,10 @@ func (c *controller) reconcileMachineHealth(ctx context.Context, machine *v1alph LastUpdateTime: metav1.Now(), PreserveExpiryTime: machine.Status.CurrentStatus.PreserveExpiryTime, } + // check if preservation is needed for the failed machine + if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed { + clone.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)} + } cloneDirty = true if machineClass != nil { metrics.IncrementNumFailedToJoin(machine, machineClass) @@ -2158,7 +2162,20 @@ func (c *controller) updateMachineToFailedState(ctx context.Context, description PreserveExpiryTime: machine.Status.CurrentStatus.PreserveExpiryTime, } - _, err := c.controlMachineClient.Machines(clone.Namespace).UpdateStatus(ctx, clone, metav1.UpdateOptions{}) + node, err := c.nodeLister.Get(machine.Labels[v1alpha1.NodeLabelKey]) + if err != nil { + klog.Infof("Error fetching the node %q: %v", machine.Labels[v1alpha1.NodeLabelKey], err) + } + + // check if preservation is needed for the failed machine + if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed { + // we set the PreserveExpiryTime if not already set. + if clone.Status.CurrentStatus.PreserveExpiryTime == nil { + clone.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)} + } + } + + _, err = c.controlMachineClient.Machines(clone.Namespace).UpdateStatus(ctx, clone, metav1.UpdateOptions{}) updated := false if err != nil { // Keep retrying until update goes through diff --git a/pkg/util/provider/machinecontroller/machine_util_test.go b/pkg/util/provider/machinecontroller/machine_util_test.go index 92e86f1408..b1ef2a4395 100644 --- a/pkg/util/provider/machinecontroller/machine_util_test.go +++ b/pkg/util/provider/machinecontroller/machine_util_test.go @@ -3547,6 +3547,47 @@ var _ = Describe("machine_util", func() { }, }), ) + + DescribeTable("##PreserveExpiryTime on creation-timeout transition to Failed", + func(preserveAnnotation string, expectExpirySet bool) { + stop := make(chan struct{}) + defer close(stop) + + annotations := map[string]string{} + if preserveAnnotation != "" { + annotations[machineutils.PreserveMachineAnnotationKey] = preserveAnnotation + } + machine := newMachine( + &machinev1.MachineTemplateSpec{ObjectMeta: *newObjectMeta(&metav1.ObjectMeta{GenerateName: machineSet1Deploy1}, 0)}, + &machinev1.MachineStatus{CurrentStatus: machinev1.CurrentStatus{Phase: machinev1.MachinePending}}, + nil, annotations, map[string]string{machinev1.NodeLabelKey: "node-0-0"}, true, metav1.NewTime(time.Now().Add(-25*time.Minute)), + ) + + c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, nil, nil, false) + defer trackers.Stop() + c.safetyOptions.MachinePreserveTimeout = metav1.Duration{Duration: 24 * time.Hour} + c.permitGiver = permits.NewPermitGiver(5*time.Second, 1*time.Second) + defer c.permitGiver.Close() + waitForCacheSync(stop, c) + + _, _ = c.reconcileMachineHealth(context.TODO(), machine) + + updated, err := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), machine.Name, metav1.GetOptions{}) + Expect(err).To(BeNil()) + Expect(updated.Status.CurrentStatus.Phase).To(Equal(machinev1.MachineFailed)) + if expectExpirySet { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).NotTo(BeNil()) + Expect(updated.Status.CurrentStatus.PreserveExpiryTime.After(time.Now())).To(BeTrue()) + } else { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).To(BeNil()) + } + }, + Entry("preserve=when-failed: PreserveExpiryTime must be set", machineutils.PreserveMachineAnnotationValueWhenFailed, true), + Entry("preserve=now: PreserveExpiryTime must NOT be set by creation-timeout path", machineutils.PreserveMachineAnnotationValueNow, false), + Entry("preserve=false: PreserveExpiryTime must NOT be set", machineutils.PreserveMachineAnnotationValueFalse, false), + Entry("preserve=auto-preserved: PreserveExpiryTime must NOT be set (set later after auto-preserve selection)", machineutils.PreserveMachineAnnotationValueAutoPreserved, false), + Entry("no preserve annotation: PreserveExpiryTime must NOT be set", "", false), + ) }) Describe("#inPlaceUpdate", func() { @@ -4929,4 +4970,77 @@ var _ = Describe("machine_util", func() { }), ) }) + + Describe("#updateMachineToFailedState", func() { + type setup struct { + preserveAnnotation string + } + type expect struct { + preserveExpiryTimeSet bool + } + type testCase struct { + setup setup + expect expect + } + + DescribeTable("##PreserveExpiryTime on transition to Failed", + func(tc *testCase) { + stop := make(chan struct{}) + defer close(stop) + + annotations := map[string]string{} + if tc.setup.preserveAnnotation != "" { + annotations[machineutils.PreserveMachineAnnotationKey] = tc.setup.preserveAnnotation + } + + machine := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: testNamespace, + Annotations: annotations, + }, + Status: machinev1.MachineStatus{ + CurrentStatus: machinev1.CurrentStatus{ + Phase: machinev1.MachinePending, + }, + }, + } + clone := machine.DeepCopy() + + c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, nil, nil, false) + defer trackers.Stop() + c.safetyOptions.MachinePreserveTimeout = metav1.Duration{Duration: 24 * time.Hour} + waitForCacheSync(stop, c) + + _, err := c.updateMachineToFailedState(context.TODO(), "test failure", machine, clone) + Expect(err).To(Equal(errSuccessfulPhaseUpdate)) + + updated, getErr := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), machine.Name, metav1.GetOptions{}) + Expect(getErr).To(BeNil()) + Expect(updated.Status.CurrentStatus.Phase).To(Equal(machinev1.MachineFailed)) + if tc.expect.preserveExpiryTimeSet { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).NotTo(BeNil()) + Expect(updated.Status.CurrentStatus.PreserveExpiryTime.After(time.Now())).To(BeTrue()) + } else { + Expect(updated.Status.CurrentStatus.PreserveExpiryTime).To(BeNil()) + } + }, + Entry("preserve=when-failed: PreserveExpiryTime must be set on transition to Failed", &testCase{ + setup: setup{preserveAnnotation: machineutils.PreserveMachineAnnotationValueWhenFailed}, + expect: expect{preserveExpiryTimeSet: true}, + }), + Entry("preserve=now: PreserveExpiryTime must NOT be set by updateMachineToFailedState (already set by preserveMachine)", &testCase{ + setup: setup{preserveAnnotation: machineutils.PreserveMachineAnnotationValueNow}, + expect: expect{preserveExpiryTimeSet: false}, + }), + Entry("preserve=false: PreserveExpiryTime must NOT be set", &testCase{ + setup: setup{preserveAnnotation: machineutils.PreserveMachineAnnotationValueFalse}, + expect: expect{preserveExpiryTimeSet: false}, + }), + Entry("no preserve annotation: PreserveExpiryTime must NOT be set", &testCase{ + setup: setup{}, + expect: expect{preserveExpiryTimeSet: false}, + }), + ) + }) }) diff --git a/pkg/util/provider/machineutils/utils.go b/pkg/util/provider/machineutils/utils.go index 7c9a510a96..2ce594da1a 100644 --- a/pkg/util/provider/machineutils/utils.go +++ b/pkg/util/provider/machineutils/utils.go @@ -7,17 +7,18 @@ package machineutils import ( "context" + "encoding/json" "time" "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" v1alpha1client "github.com/gardener/machine-controller-manager/pkg/client/clientset/versioned/typed/machine/v1alpha1" - v1alpha1listers "github.com/gardener/machine-controller-manager/pkg/client/listers/machine/v1alpha1" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - errorsutil "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/client-go/util/retry" "k8s.io/klog/v2" + + jsonpatch "gopkg.in/evanphx/json-patch.v4" ) const ( @@ -82,7 +83,7 @@ const ( NodeScaledDown = "ScaleDown" // NodeTerminationCondition describes nodes that are terminating - NodeTerminationCondition v1.NodeConditionType = "Terminating" + NodeTerminationCondition corev1.NodeConditionType = "Terminating" // TaintNodeCriticalComponentsNotReady is the name of a gardener taint // indicating that a node is not yet ready to have user workload scheduled @@ -180,34 +181,90 @@ func GetMachineDeploymentName(machine *v1alpha1.Machine) string { return machine.Labels["name"] } -// see https://github.com/kubernetes/kubernetes/issues/21479 -type updateMachineFunc func(machine *v1alpha1.Machine) error - -// UpdateMachineWithRetries updates a machine with given applyUpdate function. Note that machine not found error is ignored. -func UpdateMachineWithRetries(ctx context.Context, machineClient v1alpha1client.MachineInterface, machineLister v1alpha1listers.MachineLister, namespace, name string, applyUpdate updateMachineFunc) (*v1alpha1.Machine, error) { - var machine *v1alpha1.Machine - - retryErr := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - var err error - machine, err = machineLister.Machines(namespace).Get(name) - if err != nil { - return err +// PatchMachine patches a machine using a merge patch derived from mutateFn applied to the given machine object. +// If optimisticLock is true, the patch includes the current resourceVersion to detect concurrent updates. +// subresources optionally targets a subresource (e.g. "status"); omit it to patch the main resource. +func PatchMachine( + ctx context.Context, + machineClient v1alpha1client.MachineInterface, + machine *v1alpha1.Machine, + mutateFn func(*v1alpha1.Machine) error, + optimisticLock bool, + subresources ...string, +) (*v1alpha1.Machine, error) { + base, err := json.Marshal(machine) + if err != nil { + return nil, err + } + modified := machine.DeepCopy() + if err := mutateFn(modified); err != nil { + return nil, err + } + modifiedJSON, err := json.Marshal(modified) + if err != nil { + return nil, err + } + patch, err := jsonpatch.CreateMergePatch(base, modifiedJSON) + if err != nil { + return nil, err + } + if string(patch) == "{}" { + return machine, nil + } + if optimisticLock { + var patchMap map[string]any + if err := json.Unmarshal(patch, &patchMap); err != nil { + return nil, err } - machine = machine.DeepCopy() - // Apply the update, then attempt to push it to the apiserver. - if applyErr := applyUpdate(machine); applyErr != nil { - return applyErr + meta, ok := patchMap["metadata"].(map[string]any) + if !ok { + meta = map[string]any{} + } + meta["resourceVersion"] = machine.ResourceVersion + patchMap["metadata"] = meta + patch, err = json.Marshal(patchMap) + if err != nil { + return nil, err } - machine, err = machineClient.Update(ctx, machine, metav1.UpdateOptions{}) - return err - }) - - // Ignore the precondition violated error, this machine is already updated - // with the desired label. - if retryErr == errorsutil.ErrPreconditionViolated { - klog.V(4).Infof("Machine %s precondition doesn't hold, skip updating it.", name) - retryErr = nil } + return machineClient.Patch(ctx, machine.Name, types.MergePatchType, patch, metav1.PatchOptions{}, subresources...) +} - return machine, retryErr +// GetPreserveAnnotationValue returns the preserve annotation value for the given node and machine +// and a boolean informing whether we need to do any work or skip. +// Invalid annotation values are treated as absent. +func GetPreserveAnnotationValue( + node *corev1.Node, + machine *v1alpha1.Machine, +) (annotationValue string, shouldHandlePreservation bool) { + if node != nil { + if val, ok := + node.Annotations[PreserveMachineAnnotationKey]; ok && + AllowedPreserveAnnotationValues.Has(val) { + return val, true + } + klog.Warningf( + "Node %q doesn't have the annotation:%q or the annotation is not valid", + machine.Labels[v1alpha1.NodeLabelKey], + PreserveMachineAnnotationKey, + ) + if _, ok := + machine.Annotations[LastAppliedNodePreserveValueAnnotationKey]; ok { + return "", true + } + } + if val, ok := + machine.Annotations[PreserveMachineAnnotationKey]; ok && + AllowedPreserveAnnotationValues.Has(val) { + return val, true + } + klog.Warningf( + "Machine %q doesn't have the annotation:%q or the annotation is not valid", + machine.Name, + PreserveMachineAnnotationKey, + ) + if machine.Status.CurrentStatus.PreserveExpiryTime != nil { + return "", true + } + return "", false } diff --git a/pkg/util/provider/machineutils/utils_test.go b/pkg/util/provider/machineutils/utils_test.go new file mode 100644 index 0000000000..025f5f155d --- /dev/null +++ b/pkg/util/provider/machineutils/utils_test.go @@ -0,0 +1,214 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Gardener contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package machineutils + +import ( + "testing" + + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetPreserveAnnotationValue(t *testing.T) { + tests := []struct { + name string + node *corev1.Node + machine *v1alpha1.Machine + expectedValue string + expectedExists bool + }{ + { + name: "node nil, machine has valid preserve annotation", + node: nil, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueWhenFailed, + }, + }, + }, + expectedValue: PreserveMachineAnnotationValueWhenFailed, + expectedExists: true, + }, + { + name: "node nil, machine has invalid preserve annotation", + node: nil, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: "invalid-value", + }, + }, + }, + expectedValue: "", + expectedExists: false, + }, + { + name: "node nil, machine has no preserve annotation", + node: nil, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + expectedValue: "", + expectedExists: false, + }, + { + name: "node nil, machine annotations nil", + node: nil, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{}, + }, + expectedValue: "", + expectedExists: false, + }, + { + name: "node has valid preserve annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueNow, + }, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{}, + }, + expectedValue: PreserveMachineAnnotationValueNow, + expectedExists: true, + }, + { + name: "node has invalid preserve annotation, machine has valid preserve annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: "invalid-value", + }, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueWhenFailed, + }, + }, + }, + expectedValue: PreserveMachineAnnotationValueWhenFailed, + expectedExists: true, + }, + { + name: "node has valid preserve annotation, machine also has valid preserve annotation - node takes priority", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueNow, + }, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueWhenFailed, + }, + }, + }, + expectedValue: PreserveMachineAnnotationValueNow, + expectedExists: true, + }, + { + name: "node has no preserve annotation, machine has LastAppliedNodePreserveValue annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + LastAppliedNodePreserveValueAnnotationKey: PreserveMachineAnnotationValueNow, + }, + }, + }, + expectedValue: "", + expectedExists: true, + }, + { + name: "node has no preserve annotation, machine has no annotations", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{}, + }, + expectedValue: "", + expectedExists: false, + }, + { + name: "node annotations nil, machine has valid preserve annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{}, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueAutoPreserved, + }, + }, + }, + expectedValue: PreserveMachineAnnotationValueAutoPreserved, + expectedExists: true, + }, + { + name: "node has false preserve annotation value", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: PreserveMachineAnnotationValueFalse, + }, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{}, + }, + expectedValue: PreserveMachineAnnotationValueFalse, + expectedExists: true, + }, + { + name: "node has invalid preserve annotation, machine has LastAppliedNodePreserveValue annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + PreserveMachineAnnotationKey: "invalid-value", + }, + }, + }, + machine: &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + LastAppliedNodePreserveValueAnnotationKey: PreserveMachineAnnotationValueNow, + }, + }, + }, + expectedValue: "", + expectedExists: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + val, shouldHandlePreservation := GetPreserveAnnotationValue(tt.node, tt.machine) + if val != tt.expectedValue { + t.Errorf("expected value %q, got %q", tt.expectedValue, val) + } + if shouldHandlePreservation != tt.expectedExists { + t.Errorf("expected exists %v, got %v", tt.expectedExists, shouldHandlePreservation) + } + }) + } +}