Skip to content

Commit

Permalink
Avoid race between waitForStack* and reprocessing
Browse files Browse the repository at this point in the history
Many of the tests wait for a particular value of
`.status.lastUpdate.State`, then check the values of conditions are as
expected. However, between these two things, they fetch the object from
the API server again (`refetch`), since the waitForStack* uses a
temporary variable rather than mutating the stack given to it. That that
admits the possibility that the status might have been updated again, by
the controller reconciling it again.

Instead, let waitForStack* write to the object it's given, and don't
`refetch` -- then, the status observed will be the status at the point
the wait succeeded.

Signed-off-by: Michael Bridgen <[email protected]>
  • Loading branch information
squaremo committed Oct 10, 2022
1 parent 2f701a5 commit 531fe1c
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 11 deletions.
6 changes: 0 additions & 6 deletions test/flux_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ var _ = Describe("Flux source integration", func() {

It("is marked as failed and to be retried", func() {
waitForStackFailure(stack)
refetch(stack)
reconcilingCondition := apimeta.FindStatusCondition(stack.Status.Conditions, pulumiv1.ReconcilingCondition)
Expect(reconcilingCondition).ToNot(BeNil())
Expect(reconcilingCondition.Reason).To(Equal(pulumiv1.ReconcilingRetryReason))
Expand Down Expand Up @@ -258,7 +257,6 @@ var _ = Describe("Flux source integration", func() {
})

It("records the revision from the source", func() {
refetch(stack)
Expect(stack.Status.LastUpdate).NotTo(BeNil())
Expect(stack.Status.LastUpdate.LastSuccessfulCommit).To(Equal(artifactRevision))
})
Expand All @@ -280,7 +278,6 @@ var _ = Describe("Flux source integration", func() {

It("marks the stack as failed and to be retried", func() {
waitForStackFailure(stack)
refetch(stack)
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReconcilingCondition)).To(BeTrue())
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReadyCondition)).To(BeFalse())

Expand All @@ -297,7 +294,6 @@ var _ = Describe("Flux source integration", func() {
Expect(k8sClient.Status().Update(context.TODO(), source)).To(Succeed())

waitForStackSuccess(stack)
refetch(stack)
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReadyCondition)).To(BeTrue())
})
})
Expand All @@ -312,7 +308,6 @@ var _ = Describe("Flux source integration", func() {
It("rejects the tarball and fails with a retry", func() {
resetWaitForStack()
waitForStackFailure(stack)
refetch(stack)
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReconcilingCondition)).To(BeTrue())
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReadyCondition)).To(BeFalse())
})
Expand All @@ -327,7 +322,6 @@ var _ = Describe("Flux source integration", func() {

It("marks the Stack as failed and to be retried", func() {
waitForStackFailure(stack)
refetch(stack)
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReconcilingCondition)).To(BeTrue())
Expect(apimeta.IsStatusConditionTrue(stack.Status.Conditions, pulumiv1.ReadyCondition)).To(BeFalse())
})
Expand Down
11 changes: 6 additions & 5 deletions test/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,17 +151,18 @@ func resetWaitForStack() {
waitForStackSince = time.Now()
}

// internalWaitForStackState refetches the given stack, until its .lastUpdated.state field matches
// the one given. NB it fetches into the pointer given, so mutates the struct it's pointing at.
func internalWaitForStackState(stack *pulumiv1.Stack, state shared.StackUpdateStateMessage) {
EventuallyWithOffset(2 /* called by other helpers */, func() bool {
k := client.ObjectKeyFromObject(stack)
var s pulumiv1.Stack
err := k8sClient.Get(context.TODO(), k, &s)
err := k8sClient.Get(context.TODO(), k, stack)
if err != nil {
return false
}
return s.Status.LastUpdate != nil &&
s.Status.LastUpdate.State == state &&
s.Status.LastUpdate.LastResyncTime.Time.After(waitForStackSince)
return stack.Status.LastUpdate != nil &&
stack.Status.LastUpdate.State == state &&
stack.Status.LastUpdate.LastResyncTime.Time.After(waitForStackSince)
}, "30s", "1s").Should(BeTrue(), fmt.Sprintf("stack %q reaches state %q", stack.Name, state))
}

Expand Down

0 comments on commit 531fe1c

Please sign in to comment.