Skip to content

Commit 2cdf88f

Browse files
committed
cleaning up and starting to write better tests for the schedule controller
1 parent cbe860c commit 2cdf88f

File tree

4 files changed

+130
-80
lines changed

4 files changed

+130
-80
lines changed

internal/controller/questdbsnapshot_controller.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,8 @@ func (r *QuestDBSnapshotReconciler) Reconcile(ctx context.Context, req ctrl.Requ
6969
return ctrl.Result{}, client.IgnoreNotFound(err)
7070
}
7171

72-
// Get the secrets for the questdb
73-
74-
// Get status of the secrets
72+
// Get status of any related secrets. These will be used later to add pgwire credentials
73+
// to the pre- and post-snapshot jobs.
7574
s, err := secrets.GetSecrets(ctx, r.Client, client.ObjectKeyFromObject(snap))
7675
if err != nil {
7776
return ctrl.Result{}, err
@@ -236,6 +235,8 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
236235
var err error
237236

238237
if snap.DeletionTimestamp.IsZero() {
238+
// If we are not deleting the Snapshot, ensure that the finalizer exists
239+
239240
if !controllerutil.ContainsFinalizer(snap, crdv1beta1.QuestDBSnapshotFinalizer) {
240241
controllerutil.AddFinalizer(snap, crdv1beta1.QuestDBSnapshotFinalizer)
241242
if err := r.Update(ctx, snap); err != nil {
@@ -259,6 +260,8 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
259260
}
260261

261262
// If the job has succeeded, we need to finalize the snapshot
263+
// So we kick kick off the post-snapshot job by setting the phase
264+
// to finalizing, handling that state, and eventually requeueing the request
262265
if job.Status.Succeeded == 1 {
263266
snap.Status.Phase = crdv1beta1.SnapshotFinalizing
264267
err = r.Status().Update(ctx, snap)
@@ -275,6 +278,7 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
275278
}
276279

277280
// If the job is not active, but there are still more attempts, we need to requeue
281+
// until either the job has succeeded, or we hit the maximum number of attempts
278282
if job.Status.Active == 0 && job.Status.Failed < snap.Spec.JobBackoffLimit {
279283
return ctrl.Result{RequeueAfter: 4 * time.Second}, nil
280284
}
@@ -289,7 +293,8 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
289293
return ctrl.Result{}, err
290294

291295
case crdv1beta1.SnapshotRunning:
292-
// Wait for the snapshot to finish
296+
// Wait for the snapshot to finish. This will eventually progress the phase to
297+
// finalizing, at which point we will continue with the logic below
293298
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
294299
case crdv1beta1.SnapshotFinalizing:
295300
// Check the status of the post-snapshot job
@@ -328,6 +333,7 @@ func (r *QuestDBSnapshotReconciler) handleFinalizer(ctx context.Context, snap *c
328333

329334
return ctrl.Result{}, err
330335

336+
// If the snapshot succeeded, we can safely delete the snapshot
331337
case crdv1beta1.SnapshotSucceeded:
332338
controllerutil.RemoveFinalizer(snap, crdv1beta1.QuestDBSnapshotFinalizer)
333339
return ctrl.Result{}, r.Update(ctx, snap)

internal/controller/questdbsnapshotschedule_controller.go

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
apierrors "k8s.io/apimachinery/pkg/api/errors"
2828
"k8s.io/apimachinery/pkg/runtime"
2929
"k8s.io/client-go/tools/record"
30+
"k8s.io/client-go/util/retry"
3031
ctrl "sigs.k8s.io/controller-runtime"
3132
"sigs.k8s.io/controller-runtime/pkg/client"
3233
"sigs.k8s.io/controller-runtime/pkg/log"
@@ -55,8 +56,7 @@ type QuestDBSnapshotScheduleReconciler struct {
5556
// move the current state of the cluster closer to the desired state.
5657
func (r *QuestDBSnapshotScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
5758
var (
58-
err error
59-
dueForSnapshot bool
59+
err error
6060

6161
sched = &crdv1beta1.QuestDBSnapshotSchedule{}
6262
latestSnap = &crdv1beta1.QuestDBSnapshot{}
@@ -81,7 +81,7 @@ func (r *QuestDBSnapshotScheduleReconciler) Reconcile(ctx context.Context, req c
8181
return ctrl.Result{}, err
8282
}
8383

84-
// Update the snapshot phase status
84+
// Update the snapshot phase status based on the latest snapshot
8585
if latestSnap != nil {
8686
if latestSnap.Status.Phase != sched.Status.SnapshotPhase {
8787
sched.Status.SnapshotPhase = latestSnap.Status.Phase
@@ -92,23 +92,19 @@ func (r *QuestDBSnapshotScheduleReconciler) Reconcile(ctx context.Context, req c
9292
}
9393

9494
// Check if we are due for a snapshot
95-
if sched.Status.NextSnapshot.IsZero() {
96-
dueForSnapshot = true
97-
} else {
98-
dueForSnapshot = r.TimeSource.Now().After(sched.Status.NextSnapshot.Time)
99-
}
100-
10195
nextSnapshotTime, err := r.getNextSnapshotTime(sched)
10296
if err != nil {
10397
return ctrl.Result{}, err
10498
}
10599

100+
// Calculate the requeue time before any modifications
101+
// are made to the NextSnapshot time
106102
requeueTime := time.Until(nextSnapshotTime)
107103
if requeueTime < 0 {
108104
requeueTime = 0
109105
}
110106

111-
if dueForSnapshot {
107+
if nextSnapshotTime.Before(r.TimeSource.Now()) {
112108
// Update the next snapshot time
113109
sched.Status.NextSnapshot = metav1.NewTime(nextSnapshotTime)
114110
if err = r.Status().Update(ctx, sched); err != nil {
@@ -122,18 +118,18 @@ func (r *QuestDBSnapshotScheduleReconciler) Reconcile(ctx context.Context, req c
122118
}
123119

124120
// Build the snapshot
125-
snap, err := r.buildSnapshot(sched)
126-
if err != nil {
127-
return ctrl.Result{}, err
128-
}
129-
130-
// Create the snapshot
131-
if err = r.Create(ctx, &snap); err != nil {
132-
if !apierrors.IsAlreadyExists(err) {
133-
r.Recorder.Event(sched, "Warning", "SnapshotFailed", fmt.Sprintf("Failed to create snapshot: %s", err))
134-
return ctrl.Result{RequeueAfter: requeueTime}, err
121+
snap := r.buildSnapshot(sched)
122+
123+
err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
124+
// Create the snapshot
125+
if err = r.Create(ctx, &snap); err != nil {
126+
if !apierrors.IsAlreadyExists(err) {
127+
r.Recorder.Event(sched, "Warning", "SnapshotFailed", fmt.Sprintf("Failed to create snapshot: %s", err))
128+
}
135129
}
136-
}
130+
131+
return err
132+
})
137133

138134
if err == nil {
139135
r.Recorder.Event(sched, "Normal", "SnapshotCreated", fmt.Sprintf("Created snapshot: %s", snap.Name))
@@ -152,7 +148,7 @@ func (r *QuestDBSnapshotScheduleReconciler) SetupWithManager(mgr ctrl.Manager) e
152148
Complete(r)
153149
}
154150

155-
func (r *QuestDBSnapshotScheduleReconciler) buildSnapshot(sched *crdv1beta1.QuestDBSnapshotSchedule) (crdv1beta1.QuestDBSnapshot, error) {
151+
func (r *QuestDBSnapshotScheduleReconciler) buildSnapshot(sched *crdv1beta1.QuestDBSnapshotSchedule) crdv1beta1.QuestDBSnapshot {
156152
var (
157153
err error
158154
)
@@ -165,8 +161,11 @@ func (r *QuestDBSnapshotScheduleReconciler) buildSnapshot(sched *crdv1beta1.Ques
165161
Spec: sched.Spec.Snapshot,
166162
}
167163

168-
err = ctrl.SetControllerReference(sched, &snap, r.Scheme)
169-
return snap, err
164+
if err = ctrl.SetControllerReference(sched, &snap, r.Scheme); err != nil {
165+
panic(fmt.Sprintf("failed to set controller reference, even though we are building an object from scratch: %s", err.Error()))
166+
}
167+
168+
return snap
170169

171170
}
172171

@@ -212,7 +211,7 @@ func (r *QuestDBSnapshotScheduleReconciler) getNextSnapshotTime(sched *crdv1beta
212211

213212
lastTime := sched.Status.NextSnapshot.Time
214213
if lastTime.IsZero() {
215-
lastTime = r.TimeSource.Now()
214+
lastTime = sched.CreationTimestamp.Time
216215
}
217216
return crontab.Next(lastTime), nil
218217
}

internal/controller/questdbsnapshotschedule_controller_test.go

Lines changed: 89 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import (
77
. "github.com/onsi/gomega"
88
crdv1beta1 "github.com/questdb/questdb-operator/api/v1beta1"
99
testutils "github.com/questdb/questdb-operator/tests/utils"
10+
"github.com/thejerf/abtime"
1011
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
"k8s.io/client-go/kubernetes/scheme"
13+
"k8s.io/client-go/tools/record"
14+
ctrl "sigs.k8s.io/controller-runtime"
1115
"sigs.k8s.io/controller-runtime/pkg/client"
1216
)
1317

@@ -16,75 +20,121 @@ var _ = Describe("QuestDBSnapshotSchedule Controller", func() {
1620
q *crdv1beta1.QuestDB
1721
sched *crdv1beta1.QuestDBSnapshotSchedule
1822

23+
timeSource = abtime.NewManual()
24+
1925
timeout = time.Second * 2
2026
//consistencyTimeout = time.Millisecond * 600
2127
interval = time.Millisecond * 100
22-
)
23-
24-
BeforeEach(func() {
25-
26-
q = testutils.BuildAndCreateMockQuestDB(ctx, k8sClient)
2728

28-
sched = &crdv1beta1.QuestDBSnapshotSchedule{
29-
ObjectMeta: metav1.ObjectMeta{
30-
Name: q.Name,
31-
Namespace: q.Namespace,
32-
},
33-
Spec: crdv1beta1.QuestDBSnapshotScheduleSpec{
34-
Snapshot: crdv1beta1.QuestDBSnapshotSpec{
35-
QuestDBName: q.Name,
36-
VolumeSnapshotClassName: "csi-hostpath-snapclass",
37-
},
38-
Schedule: "*/1 * * * *",
39-
},
40-
}
41-
})
29+
r *QuestDBSnapshotScheduleReconciler
30+
)
4231

4332
Context("golden path case", Ordered, func() {
4433
var (
45-
origSched crdv1beta1.QuestDBSnapshotSchedule
46-
4734
snapList = &crdv1beta1.QuestDBSnapshotList{}
4835
)
4936

5037
BeforeAll(func() {
51-
By("Creating the QuestDBSnapshotSchedule")
38+
r = &QuestDBSnapshotScheduleReconciler{
39+
Client: k8sClient,
40+
Scheme: scheme.Scheme,
41+
Recorder: record.NewFakeRecorder(100),
42+
TimeSource: timeSource,
43+
}
44+
45+
By("Creating a QuestDB")
46+
q = testutils.BuildAndCreateMockQuestDB(ctx, k8sClient)
47+
48+
By("Creating a QuestDBSnapshotSchedule")
49+
sched = &crdv1beta1.QuestDBSnapshotSchedule{
50+
ObjectMeta: metav1.ObjectMeta{
51+
Name: q.Name,
52+
Namespace: q.Namespace,
53+
},
54+
Spec: crdv1beta1.QuestDBSnapshotScheduleSpec{
55+
Snapshot: crdv1beta1.QuestDBSnapshotSpec{
56+
QuestDBName: q.Name,
57+
VolumeSnapshotClassName: "csi-hostpath-snapclass",
58+
},
59+
Schedule: "*/1 * * * *",
60+
},
61+
}
5262
Expect(k8sClient.Create(ctx, sched)).To(Succeed())
53-
origSched = *sched
63+
64+
By("Reconciling the QuestDBSnapshotSchedule")
65+
_, err := r.Reconcile(ctx, ctrl.Request{
66+
NamespacedName: client.ObjectKeyFromObject(sched),
67+
})
68+
Expect(err).ToNot(HaveOccurred())
5469
})
5570

5671
It("should create a snapshot if the cron schedule has triggered", func() {
5772

58-
By("Bumping the clock 1 minute")
73+
By("Bumping the clock more than 1 minute")
5974
timeSource.Advance(time.Minute + time.Second)
6075

76+
By("Forcing a reconcile")
77+
_, err := r.Reconcile(ctx, ctrl.Request{
78+
NamespacedName: client.ObjectKeyFromObject(sched),
79+
})
80+
Expect(err).ToNot(HaveOccurred())
81+
6182
By("Checking that a snapshot has been created")
62-
Eventually(func(g Gomega) {
63-
g.Expect(k8sClient.List(ctx, snapList, client.InNamespace(origSched.Namespace))).Should(Succeed())
64-
g.Expect(snapList.Items).To(HaveLen(1))
65-
g.Expect(snapList.Items[0].OwnerReferences).To(HaveLen(1))
66-
g.Expect(snapList.Items[0].OwnerReferences[0].Name).To(Equal(origSched.Name))
67-
}, timeout, interval).Should(Succeed())
83+
Expect(k8sClient.List(ctx, snapList, client.InNamespace(sched.Namespace))).Should(Succeed())
84+
Expect(snapList.Items).To(HaveLen(1))
85+
Expect(snapList.Items[0].OwnerReferences).To(HaveLen(1))
86+
Expect(snapList.Items[0].OwnerReferences[0].Name).To(Equal(sched.Name))
6887
})
6988

7089
It("should report the phase of the latest snapshot", func() {
7190
By("Getting the latest snapshot")
7291
snapList := &crdv1beta1.QuestDBSnapshotList{}
73-
Eventually(func(g Gomega) {
74-
g.Expect(k8sClient.List(ctx, snapList, client.InNamespace(origSched.Namespace))).Should(Succeed())
75-
g.Expect(snapList.Items).To(HaveLen(1))
76-
}, timeout, interval).Should(Succeed())
92+
Expect(k8sClient.List(ctx, snapList, client.InNamespace(sched.Namespace))).Should(Succeed())
93+
Expect(snapList.Items).To(HaveLen(1))
7794

7895
latestSnap := &snapList.Items[0]
7996

8097
By("Setting the phase to Succeeded")
81-
latestSnap.Status.Phase = crdv1beta1.SnapshotSucceeded
82-
Expect(k8sClient.Status().Update(ctx, latestSnap)).To(Succeed())
83-
8498
Eventually(func(g Gomega) {
85-
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: origSched.Name, Namespace: origSched.Namespace}, &origSched)).To(Succeed())
86-
g.Expect(origSched.Status.SnapshotPhase).To(Equal(crdv1beta1.SnapshotSucceeded))
99+
k8sClient.Get(ctx, client.ObjectKeyFromObject(latestSnap), latestSnap)
100+
latestSnap.Status.Phase = crdv1beta1.SnapshotSucceeded
101+
g.Expect(k8sClient.Status().Update(ctx, latestSnap)).To(Succeed())
87102
}, timeout, interval).Should(Succeed())
103+
104+
By("Forcing a reconcile")
105+
_, err := r.Reconcile(ctx, ctrl.Request{
106+
NamespacedName: client.ObjectKeyFromObject(sched),
107+
})
108+
Expect(err).ToNot(HaveOccurred())
109+
110+
By("Checking that the status has been updated")
111+
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sched), sched)).To(Succeed())
112+
Expect(sched.Status.SnapshotPhase).To(Equal(crdv1beta1.SnapshotSucceeded))
113+
})
114+
115+
It("should take a second snapshot if the cron schedule has triggered", func() {
116+
By("Bumping the clock more than 1 minute")
117+
timeSource.Advance(time.Minute + time.Second)
118+
119+
By("Forcing a reconcile")
120+
_, err := r.Reconcile(ctx, ctrl.Request{
121+
NamespacedName: client.ObjectKeyFromObject(sched),
122+
})
123+
Expect(err).ToNot(HaveOccurred())
124+
125+
By("Checking that a snapshot has been created")
126+
snapList := &crdv1beta1.QuestDBSnapshotList{}
127+
Expect(k8sClient.List(ctx, snapList, client.InNamespace(sched.Namespace))).Should(Succeed())
128+
Expect(snapList.Items).To(HaveLen(2))
129+
})
130+
131+
It("should requeue the request to the time of the next trigger", func() {
132+
By("Forcing a reconcile")
133+
result, err := r.Reconcile(ctx, ctrl.Request{
134+
NamespacedName: client.ObjectKeyFromObject(sched),
135+
})
136+
Expect(err).ToNot(HaveOccurred())
137+
Expect(result.RequeueAfter).ToNot(BeZero())
88138
})
89139

90140
})

internal/controller/suite_test.go

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323

2424
. "github.com/onsi/ginkgo/v2"
2525
. "github.com/onsi/gomega"
26-
"github.com/thejerf/abtime"
2726

2827
"k8s.io/client-go/kubernetes/scheme"
2928
"k8s.io/client-go/rest"
@@ -43,10 +42,9 @@ import (
4342
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
4443

4544
var (
46-
cfg *rest.Config
47-
k8sClient client.Client
48-
testEnv *envtest.Environment
49-
timeSource = abtime.NewManual()
45+
cfg *rest.Config
46+
k8sClient client.Client
47+
testEnv *envtest.Environment
5048

5149
ctx context.Context
5250
cancel context.CancelFunc
@@ -101,19 +99,16 @@ var _ = BeforeSuite(func() {
10199
Recorder: mgr.GetEventRecorderFor("questdb-controller"),
102100
}).SetupWithManager(mgr)).Should(Succeed())
103101

104-
Expect((&QuestDBSnapshotScheduleReconciler{
105-
Client: mgr.GetClient(),
106-
Scheme: mgr.GetScheme(),
107-
Recorder: mgr.GetEventRecorderFor("questdbsnapshotschedule-controller"),
108-
TimeSource: timeSource,
109-
}).SetupWithManager(mgr)).Should(Succeed())
110-
111102
Expect((&QuestDBSnapshotReconciler{
112103
Client: mgr.GetClient(),
113104
Scheme: mgr.GetScheme(),
114105
Recorder: mgr.GetEventRecorderFor("questdbsnapshot-controller"),
115106
}).SetupWithManager(mgr)).Should(Succeed())
116107

108+
// Note: since we cannot mock the manager's time source, we will instantiate
109+
// a new QuestDBSnapshotScheduleReconciler with a mock time source and call
110+
// Reconcile directly.
111+
117112
// Start the manager
118113
go func() {
119114
defer GinkgoRecover()

0 commit comments

Comments
 (0)