Skip to content

Commit

Permalink
Ensure app has reached its desired state
Browse files Browse the repository at this point in the history
The app state is not a final one. An app can be stopped, then started
and then stopped again. Therefore, relying on the `Ready` condition
solely is not realiable enough.
Setting the desired state on the app spec does not clear the `Ready`
status condition, and thus the `Ready` state might have been set by an
unrelated change.

This change makes the app repository not only wait for the app `Ready`
condition, but also ensure that the app desired and actual state do
match.
  • Loading branch information
danail-branekov committed May 23, 2024
1 parent fb12ff7 commit d56fd53
Show file tree
Hide file tree
Showing 6 changed files with 182 additions and 57 deletions.
14 changes: 13 additions & 1 deletion api/repositories/app_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,19 @@ func (f *AppRepo) SetAppDesiredState(ctx context.Context, authInfo authorization
return AppRecord{}, fmt.Errorf("failed to set app desired state: %w", apierrors.FromK8sError(err, AppResourceType))
}

if _, err := f.appAwaiter.AwaitCondition(ctx, userClient, cfApp, korifiv1alpha1.StatusConditionReady); err != nil {
_, err = f.appAwaiter.AwaitState(ctx, userClient, cfApp, func(a *korifiv1alpha1.CFApp) error {
if _, readyConditionErr := f.appAwaiter.AwaitCondition(ctx, userClient, a, korifiv1alpha1.StatusConditionReady); err != nil {
return readyConditionErr
}

if a.Spec.DesiredState != korifiv1alpha1.AppState(message.DesiredState) ||
a.Status.ActualState != korifiv1alpha1.AppState(message.DesiredState) {
return fmt.Errorf("desired state %q not reached; actual state: %q", message.DesiredState, a.Status.ActualState)
}

return nil
})
if err != nil {
return AppRecord{}, apierrors.FromK8sError(err, AppResourceType)
}

Expand Down
44 changes: 36 additions & 8 deletions api/repositories/app_repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1218,12 +1218,26 @@ var _ = Describe("AppRepository", func() {
Expect(returnedAppRecord.SpaceGUID).To(Equal(cfSpace.Name))
})

It("waits for the app ready condition", func() {
Expect(appAwaiter.AwaitConditionCallCount()).To(Equal(1))
actualCFApp, actualCondition := appAwaiter.AwaitConditionArgsForCall(0)
It("waits for the started app state", func() {
Expect(appAwaiter.AwaitStateCallCount()).To(Equal(1))
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfApp), cfApp)).To(Succeed())

actualCFApp, actualStateCheck := appAwaiter.AwaitStateArgsForCall(0)
Expect(actualCFApp.GetName()).To(Equal(cfApp.Name))
Expect(actualCFApp.GetNamespace()).To(Equal(cfApp.Namespace))
Expect(actualCondition).To(Equal(korifiv1alpha1.StatusConditionReady))
Expect(actualStateCheck(&korifiv1alpha1.CFApp{
Spec: korifiv1alpha1.CFAppSpec{
DesiredState: korifiv1alpha1.AppState(desiredAppState),
},
Status: korifiv1alpha1.CFAppStatus{
Conditions: []metav1.Condition{{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
ObservedGeneration: cfApp.Generation,
}},
ActualState: korifiv1alpha1.AppState(desiredAppState),
},
})).To(Succeed())
})

It("changes the desired state of the App", func() {
Expand All @@ -1243,12 +1257,26 @@ var _ = Describe("AppRepository", func() {
Expect(returnedErr).ToNot(HaveOccurred())
})

It("waits for the app ready condition", func() {
Expect(appAwaiter.AwaitConditionCallCount()).To(Equal(1))
actualCFApp, actualCondition := appAwaiter.AwaitConditionArgsForCall(0)
It("waits for the stopped app state", func() {
Expect(appAwaiter.AwaitStateCallCount()).To(Equal(1))
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfApp), cfApp)).To(Succeed())

actualCFApp, actualStateCheck := appAwaiter.AwaitStateArgsForCall(0)
Expect(actualCFApp.GetName()).To(Equal(cfApp.Name))
Expect(actualCFApp.GetNamespace()).To(Equal(cfApp.Namespace))
Expect(actualCondition).To(Equal(korifiv1alpha1.StatusConditionReady))
Expect(actualStateCheck(&korifiv1alpha1.CFApp{
Spec: korifiv1alpha1.CFAppSpec{
DesiredState: korifiv1alpha1.AppState(desiredAppState),
},
Status: korifiv1alpha1.CFAppStatus{
Conditions: []metav1.Condition{{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
ObservedGeneration: cfApp.Generation,
}},
ActualState: korifiv1alpha1.AppState(desiredAppState),
},
})).To(Succeed())
})

It("changes the desired state of the App", func() {
Expand Down
18 changes: 12 additions & 6 deletions api/repositories/conditions/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ func NewConditionAwaiter[T RuntimeObjectWithStatusConditions, L any, PL ObjectLi
}

func (a *Awaiter[T, L, PL]) AwaitCondition(ctx context.Context, k8sClient client.WithWatch, object client.Object, conditionType string) (T, error) {
return a.AwaitState(ctx, k8sClient, object, func(o T) error {
return checkConditionIsTrue(o, conditionType)
})
}

func (a *Awaiter[T, L, PL]) AwaitState(ctx context.Context, k8sClient client.WithWatch, object client.Object, checkState func(T) error) (T, error) {
var empty T
objList := PL(new(L))

Expand All @@ -47,25 +53,25 @@ func (a *Awaiter[T, L, PL]) AwaitCondition(ctx context.Context, k8sClient client
}
defer watch.Stop()

var conditionCheckErr error
var checkStateErr error
for e := range watch.ResultChan() {
obj, ok := e.Object.(T)
if !ok {
continue
}

conditionCheckErr = checkConditionIsTrue(ctx, obj, conditionType)
if conditionCheckErr == nil {
checkStateErr = checkState(obj)
if checkStateErr == nil {
return obj, nil
}
}

return empty, fmt.Errorf("object %s/%s status condition %s did not become true in %.2f s: %s",
object.GetNamespace(), object.GetName(), conditionType, a.timeout.Seconds(), conditionCheckErr.Error(),
return empty, fmt.Errorf("object %s/%s state has not been met in %.2f s: %s",
object.GetNamespace(), object.GetName(), a.timeout.Seconds(), checkStateErr.Error(),
)
}

func checkConditionIsTrue[T RuntimeObjectWithStatusConditions](ctx context.Context, obj T, conditionType string) error {
func checkConditionIsTrue[T RuntimeObjectWithStatusConditions](obj T, conditionType string) error {
condition := meta.FindStatusCondition(obj.StatusConditions(), conditionType)

if condition == nil {
Expand Down
127 changes: 85 additions & 42 deletions api/repositories/conditions/await_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package conditions_test

import (
"errors"
"sync"
"time"

Expand Down Expand Up @@ -56,67 +57,109 @@ var _ = Describe("ConditionAwaiter", func() {
Expect(k8sClient.Create(ctx, task)).To(Succeed())
})

JustBeforeEach(func() {
awaitedTask, awaitErr = awaiter.AwaitCondition(ctx, k8sClient, task, korifiv1alpha1.StatusConditionReady)
})
Describe("AwaitCondition", func() {
JustBeforeEach(func() {
awaitedTask, awaitErr = awaiter.AwaitCondition(ctx, k8sClient, task, korifiv1alpha1.StatusConditionReady)
})

It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("condition Ready not set yet")))
})
It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("condition Ready not set yet")))
})

When("the condition becomes false", func() {
BeforeEach(func() {
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionFalse,
Reason: "initialized",
ObservedGeneration: task.Generation,
When("the condition becomes false", func() {
BeforeEach(func() {
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionFalse,
Reason: "initialized",
ObservedGeneration: task.Generation,
})
})
})
})

It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("expected the Ready condition to be true")))
It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("expected the Ready condition to be true")))
})
})
})

When("the condition becomes true", func() {
BeforeEach(func() {
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
Reason: "initialized",
ObservedGeneration: task.Generation,
When("the condition becomes true", func() {
BeforeEach(func() {
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
Reason: "initialized",
ObservedGeneration: task.Generation,
})
})
})

It("succeeds and returns the updated object", func() {
Expect(awaitErr).NotTo(HaveOccurred())
Expect(awaitedTask).NotTo(BeNil())

Expect(awaitedTask.Name).To(Equal(task.Name))
Expect(meta.IsStatusConditionTrue(awaitedTask.Status.Conditions, korifiv1alpha1.StatusConditionReady)).To(BeTrue())
})
})

It("succeeds and returns the updated object", func() {
Expect(awaitErr).NotTo(HaveOccurred())
Expect(awaitedTask).NotTo(BeNil())
When("the condition becomes true but is outdated", func() {
BeforeEach(func() {
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(task), task)).To(Succeed())
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
Reason: "initialized",
ObservedGeneration: task.Generation - 1,
})
})
})

Expect(awaitedTask.Name).To(Equal(task.Name))
Expect(meta.IsStatusConditionTrue(awaitedTask.Status.Conditions, korifiv1alpha1.StatusConditionReady)).To(BeTrue())
It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("condition Ready is outdated")))
})
})
})

When("the condition becomes true but is outdated", func() {
Describe("AwaitState", func() {
var checkState func(t *korifiv1alpha1.CFTask) error

BeforeEach(func() {
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(task), task)).To(Succeed())
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
meta.SetStatusCondition(&cfTask.Status.Conditions, metav1.Condition{
Type: korifiv1alpha1.StatusConditionReady,
Status: metav1.ConditionTrue,
Reason: "initialized",
ObservedGeneration: task.Generation - 1,
})
})
checkState = func(t *korifiv1alpha1.CFTask) error {
if t.Labels["test-label"] != "test-value" {
return errors.New("await-state-err")
}

return nil
}
})

JustBeforeEach(func() {
awaitedTask, awaitErr = awaiter.AwaitState(ctx, k8sClient, task, checkState)
})

It("returns an error", func() {
Expect(awaitErr).To(MatchError(ContainSubstring("condition Ready is outdated")))
Expect(awaitErr).To(MatchError(ContainSubstring("await-state-err")))
})

When("the state is reached", func() {
BeforeEach(func() {
asyncPatchTask(func(cfTask *korifiv1alpha1.CFTask) {
cfTask.Labels = map[string]string{
"test-label": "test-value",
}
})
})

It("succeeds and returns the updated object", func() {
Expect(awaitErr).NotTo(HaveOccurred())
Expect(awaitedTask).NotTo(BeNil())

Expect(awaitedTask.Name).To(Equal(task.Name))
Expect(awaitedTask.Labels).To(HaveKeyWithValue("test-label", "test-value"))
})
})
})
})
35 changes: 35 additions & 0 deletions api/repositories/fakeawaiter/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ type FakeAwaiter[T conditions.RuntimeObjectWithStatusConditions, L any, PL condi
conditionType string
}
AwaitConditionStub func(context.Context, client.WithWatch, client.Object, string) (T, error)
AwaitStateStub func(context.Context, client.WithWatch, client.Object, func(T) error) (T, error)
awaitStateCalls []struct {
obj client.Object
checkState func(T) error
}
}

func (a *FakeAwaiter[T, L, PL]) AwaitCondition(ctx context.Context, k8sClient client.WithWatch, object client.Object, conditionType string) (T, error) {
Expand Down Expand Up @@ -44,3 +49,33 @@ func (a *FakeAwaiter[T, L, PL]) AwaitConditionCallCount() int {
func (a *FakeAwaiter[T, L, PL]) AwaitConditionArgsForCall(i int) (client.Object, string) {
return a.awaitConditionCalls[i].obj, a.awaitConditionCalls[i].conditionType
}

func (a *FakeAwaiter[T, L, PL]) AwaitState(ctx context.Context, k8sClient client.WithWatch, object client.Object, checkState func(T) error) (T, error) {
a.awaitStateCalls = append(a.awaitStateCalls, struct {
obj client.Object
checkState func(T) error
}{
object,
checkState,
})

if a.AwaitStateStub == nil {
return object.(T), nil
}

return a.AwaitStateStub(ctx, k8sClient, object, checkState)
}

func (a *FakeAwaiter[T, L, PL]) AwaitStateReturns(object T, err error) {
a.AwaitStateStub = func(ctx context.Context, k8sClient client.WithWatch, object client.Object, _ func(T) error) (T, error) {
return object.(T), err
}
}

func (a *FakeAwaiter[T, L, PL]) AwaitStateCallCount() int {
return len(a.awaitStateCalls)
}

func (a *FakeAwaiter[T, L, PL]) AwaitStateArgsForCall(i int) (client.Object, func(T) error) {
return a.awaitStateCalls[i].obj, a.awaitStateCalls[i].checkState
}
1 change: 1 addition & 0 deletions api/repositories/shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type RepositoryCreator interface {

type Awaiter[T runtime.Object] interface {
AwaitCondition(context.Context, client.WithWatch, client.Object, string) (T, error)
AwaitState(context.Context, client.WithWatch, client.Object, func(T) error) (T, error)
}

func getLastUpdatedTime(obj client.Object) *time.Time {
Expand Down

0 comments on commit d56fd53

Please sign in to comment.