Skip to content

Commit

Permalink
Improve error management
Browse files Browse the repository at this point in the history 
Signed-off-by: David Cassany <[email protected]>
  • Loading branch information
@davidcassany
davidcassany committed Oct 5, 2023
1 parent 664bcc2 commit 582bdde
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 23 deletions.
29 changes: 17 additions & 12 deletions controllers/managedosversionchannel_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (r *ManagedOSVersionChannelReconciler) reconcile(ctx context.Context, manag
meta.SetStatusCondition(&managedOSVersionChannel.Status.Conditions, metav1.Condition{
Type: elementalv1.ReadyCondition,
Reason: elementalv1.InvalidConfigurationReason,
Status: metav1.ConditionTrue,
Status: metav1.ConditionFalse,
Message: msg,
})
logger.Error(nil, msg)
Expand All @@ -153,7 +153,7 @@ func (r *ManagedOSVersionChannelReconciler) reconcile(ctx context.Context, manag
meta.SetStatusCondition(&managedOSVersionChannel.Status.Conditions, metav1.Condition{
Type: elementalv1.ReadyCondition,
Reason: elementalv1.InvalidConfigurationReason,
Status: metav1.ConditionTrue,
Status: metav1.ConditionFalse,
Message: msg,
})
logger.Error(nil, msg)
Expand All @@ -166,7 +166,7 @@ func (r *ManagedOSVersionChannelReconciler) reconcile(ctx context.Context, manag
meta.SetStatusCondition(&managedOSVersionChannel.Status.Conditions, metav1.Condition{
Type: elementalv1.ReadyCondition,
Reason: elementalv1.InvalidConfigurationReason,
Status: metav1.ConditionTrue,
Status: metav1.ConditionFalse,
Message: msg,
})
logger.Error(nil, msg)
Expand Down Expand Up @@ -213,9 +213,13 @@ func (r *ManagedOSVersionChannelReconciler) handleSyncPod(ctx context.Context, p
logger := ctrl.LoggerFrom(ctx)

defer func() {
r.deletePodOnError(ctx, pod, ch, err)
// swallow former error to prevent a reconcile loop retrigger
r.handleError(ctx, pod, ch, err)
err = nil
}()

// Once the Pod already started we do not return error in case of failure so synchronization is not retriggered, we just set
// to false readyness condition and schedule a new resynchronization for the next interval
switch pod.Status.Phase {
case corev1.PodPending, corev1.PodRunning:
logger.Info("Waiting until the pod is on succeeded state ", "pod", pod.Name)
Expand All @@ -229,11 +233,11 @@ func (r *ManagedOSVersionChannelReconciler) handleSyncPod(ctx context.Context, p
case corev1.PodSucceeded:
data, err = r.syncerProvider.ReadPodLogs(ctx, r.kcl, pod, displayContainer)
if err != nil {
return ctrl.Result{}, err
return ctrl.Result{RequeueAfter: interval}, err
}
err = r.createManagedOSVersions(ctx, ch, data)
if err != nil {
return ctrl.Result{}, err
return ctrl.Result{RequeueAfter: interval}, err
}
if err = r.Delete(ctx, pod); err != nil {
logger.Error(err, "could not delete the pod", "pod", pod.Name)
Expand All @@ -250,12 +254,12 @@ func (r *ManagedOSVersionChannelReconciler) handleSyncPod(ctx context.Context, p
return ctrl.Result{RequeueAfter: interval}, nil
default:
// Any other phase (failed or unknown) is considered an error
return ctrl.Result{}, fmt.Errorf("synchronization pod failed")
return ctrl.Result{RequeueAfter: interval}, fmt.Errorf("synchronization pod failed")
}
}

// deletePodOnError deletes the pod if err is not nil
func (r *ManagedOSVersionChannelReconciler) deletePodOnError(ctx context.Context, pod *corev1.Pod, ch *elementalv1.ManagedOSVersionChannel, err error) {
// handleErrors deletes the pod, produces error log traces and sets the failure state if error is not nil
func (r *ManagedOSVersionChannelReconciler) handleError(ctx context.Context, pod *corev1.Pod, ch *elementalv1.ManagedOSVersionChannel, err error) {
logger := ctrl.LoggerFrom(ctx)

if err != nil {
Expand All @@ -272,7 +276,7 @@ func (r *ManagedOSVersionChannelReconciler) deletePodOnError(ctx context.Context
}
}

// createManagedOSVersions unmarshals managedOSVersions from a byte array and creates them
// createManagedOSVersions unmarshals managedOSVersions from a byte array and creates them.
func (r *ManagedOSVersionChannelReconciler) createManagedOSVersions(ctx context.Context, ch *elementalv1.ManagedOSVersionChannel, data []byte) error {
logger := ctrl.LoggerFrom(ctx)

Expand All @@ -289,6 +293,7 @@ func (r *ManagedOSVersionChannelReconciler) createManagedOSVersions(ctx context.
})

var errs []error

for _, v := range vers {
vcpy := v.DeepCopy()
vcpy.ObjectMeta.Namespace = ch.Namespace
Expand Down Expand Up @@ -321,11 +326,11 @@ func (r *ManagedOSVersionChannelReconciler) createManagedOSVersions(ctx context.
}
} else if err = r.Create(ctx, vcpy); err != nil {
if apierrors.IsAlreadyExists(err) {
logger.Info("already existing managedOSVersion", "name", vcpy.Name)
logger.Error(err, "already existing managedOSVersion", "name", vcpy.Name)
} else {
logger.Error(err, "failed to create a managedosversion", "name", vcpy.Name)
errs = append(errs, err)
}
errs = append(errs, err)
} else {
logger.Info("managedOSVersion created", "name", vcpy.Name)
}
Expand Down
24 changes: 13 additions & 11 deletions controllers/managedosversionchannel_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ var _ = Describe("reconcile managed os version channel", func() {
}, managedOSVersionChannel)).To(Succeed())

Expect(managedOSVersionChannel.Status.Conditions[0].Reason).To(Equal(elementalv1.InvalidConfigurationReason))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionTrue))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse))
})

It("should reconcile managed os version channel object with a bad a sync interval", func() {
Expand All @@ -247,7 +247,7 @@ var _ = Describe("reconcile managed os version channel", func() {
}, managedOSVersionChannel)).To(Succeed())

Expect(managedOSVersionChannel.Status.Conditions[0].Reason).To(Equal(elementalv1.InvalidConfigurationReason))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionTrue))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse))
})

It("should reconcile managed os version channel object with a not valid type", func() {
Expand All @@ -264,15 +264,14 @@ var _ = Describe("reconcile managed os version channel", func() {
})
Expect(err).ToNot(HaveOccurred())
Expect(res.RequeueAfter).To(Equal(0 * time.Second))
Expect(res.Requeue).To(BeFalse())

Expect(cl.Get(ctx, client.ObjectKey{
Name: managedOSVersionChannel.Name,
Namespace: managedOSVersionChannel.Namespace,
}, managedOSVersionChannel)).To(Succeed())

Expect(managedOSVersionChannel.Status.Conditions[0].Reason).To(Equal(elementalv1.InvalidConfigurationReason))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionTrue))
Expect(managedOSVersionChannel.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse))
})

It("it fails to reconcile a managed os version channel when channel provides invalid JSON", func() {
Expand All @@ -285,8 +284,9 @@ var _ = Describe("reconcile managed os version channel", func() {
Expect(cl.Create(ctx, managedOSVersionChannel)).To(Succeed())

// No error and status updated (no requeue)
_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
res, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).ToNot(HaveOccurred())
Expect(res.RequeueAfter).To(Equal(0 * time.Second))

Expect(cl.Get(ctx, client.ObjectKey{
Name: managedOSVersionChannel.Name,
Expand All @@ -303,7 +303,7 @@ var _ = Describe("reconcile managed os version channel", func() {

setPodPhase(pod, corev1.PodSucceeded)
_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).To(HaveOccurred())
Expect(err).NotTo(HaveOccurred())

Expect(cl.Get(ctx, client.ObjectKey{
Name: managedOSVersionChannel.Name,
Expand All @@ -330,7 +330,7 @@ var _ = Describe("reconcile managed os version channel", func() {
Expect(cl.Create(ctx, managedOSVersionChannel)).To(Succeed())

// No error and status updated (no requeue)
_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
res, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).ToNot(HaveOccurred())

Expect(cl.Get(ctx, client.ObjectKey{
Expand All @@ -349,8 +349,9 @@ var _ = Describe("reconcile managed os version channel", func() {
Expect(pod.Status.Phase).To(Equal(corev1.PodPending))

setPodPhase(pod, corev1.PodSucceeded)
_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).To(HaveOccurred())
res, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).NotTo(HaveOccurred())
Expect(res.RequeueAfter).To(Equal(1 * time.Hour))

Expect(cl.Get(ctx, client.ObjectKey{
Name: managedOSVersionChannel.Name,
Expand Down Expand Up @@ -395,8 +396,9 @@ var _ = Describe("reconcile managed os version channel", func() {
Expect(pod.Status.Phase).To(Equal(corev1.PodPending))

setPodPhase(pod, corev1.PodFailed)
_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).To(HaveOccurred())
res, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: name})
Expect(err).NotTo(HaveOccurred())
Expect(res.RequeueAfter).To(Equal(1 * time.Hour))

Expect(cl.Get(ctx, client.ObjectKey{
Name: managedOSVersionChannel.Name,
Expand Down

0 comments on commit 582bdde

Please sign in to comment.