-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure re-sync is triggered #773
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,12 +187,12 @@ func (r *ManagedOSVersionChannelReconciler) reconcile(ctx context.Context, manag | |
|
||
if readyCondition.Status == metav1.ConditionTrue { | ||
logger.Info("synchronization already done", "lastSync", lastSync) | ||
return ctrl.Result{}, nil | ||
return ctrl.Result{RequeueAfter: time.Until(lastSync.Add(interval))}, nil | ||
} | ||
|
||
if managedOSVersionChannel.Status.FailedSynchronizationAttempts > maxConscutiveFailures { | ||
logger.Error(fmt.Errorf("stop retrying"), "sychronization failed consecutively too many times", "failed attempts", managedOSVersionChannel.Status.FailedSynchronizationAttempts) | ||
return ctrl.Result{}, nil | ||
return ctrl.Result{RequeueAfter: time.Until(lastSync.Add(interval))}, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was an actual bug or leftover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, nice fix! |
||
} | ||
|
||
pod := &corev1.Pod{} | ||
|
@@ -478,6 +478,11 @@ func (r *ManagedOSVersionChannelReconciler) createSyncerPod(ctx context.Context, | |
return nil | ||
} | ||
|
||
// filterChannelEvents is a method that filters reconcile requests events for the channels reconciler. | ||
// ManagedOSVersionChannelReconciler watches channels and owned pods. This filter ignores pod | ||
// create/delete/generic events and only reacts on pod phase updates. Channel update events are | ||
// only reconciled if the update includes a new generation of the resource, all other events are not | ||
// filtered. | ||
func filterChannelEvents() predicate.Funcs { | ||
return predicate.Funcs{ | ||
// Process only new generation updates for channels and new phase for pods updates | ||
|
@@ -525,5 +530,16 @@ func filterChannelEvents() predicate.Funcs { | |
logger.V(log.DebugDepth).Info("Processing generic event", "Obj", e.Object.GetName()) | ||
return true | ||
}, | ||
// Ignore pods creation | ||
CreateFunc: func(e event.CreateEvent) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to prevent reconciling again immediately after creating the pod resource. we should only re-reconcile on pod status changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well done, this was the extra reconcile loop we saw |
||
logger := ctrl.LoggerFrom(context.Background()) | ||
|
||
if _, ok := e.Object.(*corev1.Pod); ok { | ||
return false | ||
} | ||
// Return true in case it watches other types | ||
logger.V(log.DebugDepth).Info("Processing create event", "Obj", e.Object.GetName()) | ||
return true | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,10 +209,11 @@ var _ = Describe("reconcile managed os version channel", func() { | |
Namespace: pod.Namespace, | ||
}, pod)).NotTo(Succeed()) | ||
|
||
// No re-sync can be triggered before the minium time between syncs | ||
// Re-sync is triggered to interval | ||
res, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: name}) | ||
Expect(err).ToNot(HaveOccurred()) | ||
Expect(res.RequeueAfter).To(Equal(0 * time.Second)) | ||
Expect(res.RequeueAfter).To(BeNumerically("<", 1*time.Minute)) | ||
Expect(res.RequeueAfter).To(BeNumerically(">", 59*time.Second)) | ||
}) | ||
|
||
It("should reconcile managed os version channel object without a type", func() { | ||
|
@@ -555,14 +556,14 @@ var _ = Describe("managed os version channel controller integration tests", func | |
Namespace: ch.Namespace, | ||
}, pod) | ||
return err != nil && apierrors.IsNotFound(err) | ||
}, 12*time.Second, 2*time.Second).Should(BeTrue()) | ||
}, 6*time.Second, 1*time.Second).Should(BeTrue()) | ||
|
||
// Simulate a channel content change | ||
syncerProvider.SetJSON(updatedJSON) | ||
|
||
// Updating the channel after the minimum time between syncs causes an automatic update | ||
// Updating the channel causes an automatic update | ||
patchBase := client.MergeFrom(ch.DeepCopy()) | ||
ch.Spec.SyncInterval = "10m" | ||
ch.Spec.SyncInterval = "10s" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sets sync interval to 10 seconds for this test |
||
Expect(cl.Patch(ctx, ch, patchBase)).To(Succeed()) | ||
|
||
// Pod is created | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A pod is created immediately after patching the channel as a channel resource update triggers a new sync |
||
|
@@ -572,7 +573,7 @@ var _ = Describe("managed os version channel controller integration tests", func | |
Namespace: ch.Namespace, | ||
}, pod) | ||
return err == nil | ||
}, 12*time.Second, 2*time.Second).Should(BeTrue()) | ||
}, 6*time.Second, 1*time.Second).Should(BeTrue()) | ||
setPodPhase(pod, corev1.PodSucceeded) | ||
|
||
// New added versions are synced | ||
|
@@ -582,14 +583,62 @@ var _ = Describe("managed os version channel controller integration tests", func | |
Namespace: ch.Namespace, | ||
}, managedOSVersion) | ||
return err == nil | ||
}, 12*time.Second, 2*time.Second).Should(BeTrue()) | ||
}, 6*time.Second, 1*time.Second).Should(BeTrue()) | ||
|
||
// After channel update already existing versions were patched | ||
Expect(cl.Get(ctx, client.ObjectKey{ | ||
Name: "v0.1.0", | ||
Namespace: ch.Namespace, | ||
}, managedOSVersion)).To(Succeed()) | ||
Expect(managedOSVersion.Spec.Version).To(Equal("v0.1.0-patched")) | ||
|
||
// Pod is deleted | ||
Eventually(func() bool { | ||
err := cl.Get(ctx, client.ObjectKey{ | ||
Name: ch.Name, | ||
Namespace: ch.Namespace, | ||
}, pod) | ||
return err != nil && apierrors.IsNotFound(err) | ||
}, 2*time.Second, 1*time.Second).Should(BeTrue()) | ||
|
||
Expect(cl.Get(ctx, client.ObjectKey{ | ||
Name: ch.Name, | ||
Namespace: ch.Namespace, | ||
}, ch)).To(Succeed()) | ||
|
||
// Simulate another channel content change | ||
syncerProvider.SetJSON(deprecatingJSON) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing channel content but not channel resource, hence this does not trigger a new resync, we have to wait for the interval (10s). |
||
|
||
timeout := time.Until(ch.Status.LastSyncedTime.Add(10*time.Second)) - 1*time.Second | ||
|
||
// No pod is created during the interval | ||
Consistently(func() bool { | ||
err := cl.Get(ctx, client.ObjectKey{ | ||
Name: ch.Name, | ||
Namespace: ch.Namespace, | ||
}, pod) | ||
return apierrors.IsNotFound(err) | ||
}, timeout, 1*time.Second).Should(BeTrue()) | ||
|
||
// Pod is created once the resync is triggered automatically | ||
Eventually(func() bool { | ||
err := cl.Get(ctx, client.ObjectKey{ | ||
Name: ch.Name, | ||
Namespace: ch.Namespace, | ||
}, pod) | ||
return err == nil | ||
}, 4*time.Second, 1*time.Second).Should(BeTrue()) | ||
setPodPhase(pod, corev1.PodSucceeded) | ||
|
||
// v0.1.0 becomes a managedOSVersion out of sync | ||
Eventually(func() bool { | ||
Expect(cl.Get(ctx, client.ObjectKey{ | ||
Name: "v0.1.0", | ||
Namespace: ch.Namespace, | ||
}, managedOSVersion)).To(Succeed()) | ||
|
||
return managedOSVersion.Annotations[elementalv1.ElementalManagedOSVersionNoLongerSyncedAnnotation] == elementalv1.ElementalManagedOSVersionNoLongerSyncedValue | ||
}, 6*time.Second, 1*time.Second).Should(BeTrue()) | ||
}) | ||
|
||
It("should deprecate a version after it's removed from channel", func() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this shouldn't be needed, but it certainly does not hurt and helps on making the logic more robust. The unit test verifying the automatic resync after the interval passes without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was wondering if this could let us to queue extra, unneeded reconcile loops, but seems in practice never happens. Moreover, if we ever would reconcile once more, nothing bad could happen 👍🏼