Skip to content
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

Merged
merged 1 commit into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions controllers/managedosversionchannel_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

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.

Copy link
Member

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 👍🏼

}

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was an actual bug or leftover

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, nice fix!

}

pod := &corev1.Pod{}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
},
}
}
63 changes: 56 additions & 7 deletions controllers/managedosversionchannel_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Expand All @@ -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
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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() {
Expand Down
Loading