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.

Sign up for GitHub

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

fix ready condition not being set to true #303

Merged
merged 1 commit into from
Mar 8, 2024
Merged

fix ready condition not being set to true #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 13 additions & 10 deletions internal/controller/ansibleRun/ansibleRun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext

// disable checkMode for real action
c.runner.EnableCheckMode(false)
if err := c.runAnsible(cr); err != nil {
if err := c.runAnsible(ctx, cr); err != nil {
return managed.ExternalUpdate{}, fmt.Errorf("running ansible: %w", err)
}

Expand Down Expand Up @@ -448,6 +448,10 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
isLastSyncOK := (desired.GetCondition(xpv1.TypeSynced).Status == v1.ConditionTrue)

if isUpToDate && isLastSyncOK {
desired.SetConditions(xpv1.Available())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When setting Ready condition right after a successful run, various transient errors can happen preventing this status update, and, potentially, leaving the condition False forever. This ensures Ready is set even if such errors occur

if err := c.kube.Status().Update(ctx, desired); err != nil {
return managed.ExternalObservation{}, fmt.Errorf("updating status: %w", err)
}
// nothing to do for this run
return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
}
Expand All @@ -472,7 +476,7 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
return managed.ExternalObservation{}, err
}

if err := c.runAnsible(desired); err != nil {
if err := c.runAnsible(ctx, desired); err != nil {
return managed.ExternalObservation{}, fmt.Errorf("running ansible: %w", err)
}

Expand All @@ -483,7 +487,7 @@ func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alph
return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
}

func (c *external) runAnsible(cr *v1alpha1.AnsibleRun) error {
func (c *external) runAnsible(ctx context.Context, cr *v1alpha1.AnsibleRun) error {
dc, _, err := c.runner.Run()
if err != nil {
return err
Expand All @@ -493,16 +497,15 @@ func (c *external) runAnsible(cr *v1alpha1.AnsibleRun) error {
cond := xpv1.Unavailable()
cond.Message = err.Error()
cr.SetConditions(cond)

return err
} else {
cr.SetConditions(xpv1.Available())
}

cr.SetConditions(xpv1.Available())

// no need to persist status update explicitly, cr modifications are in-place and will
// be persisted by crossplane-runtime
if err := c.kube.Status().Update(ctx, cr); err != nil {
return fmt.Errorf("updating status: %w", err)
}

return nil
return err
}

func addBehaviorVars(pc *v1alpha1.ProviderConfig) map[string]string {
Expand Down
16 changes: 12 additions & 4 deletions internal/controller/ansibleRun/ansibleRun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,9 @@ func TestObserve(t *testing.T) {
reason: "We should not run ansible when spec has not changed and last sync was successful",
fields: fields{
kube: &test.MockClient{
MockGet: test.NewMockGetFn(nil),
MockUpdate: test.NewMockUpdateFn(nil),
MockGet: test.NewMockGetFn(nil),
MockUpdate: test.NewMockUpdateFn(nil),
MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
},
runner: &MockRunner{
MockAnsibleRunPolicy: func() *ansible.RunPolicy {
Expand All @@ -639,8 +640,9 @@ func TestObserve(t *testing.T) {
reason: "We should run ansible when spec has not changed but last sync was unsuccessful",
fields: fields{
kube: &test.MockClient{
MockGet: test.NewMockGetFn(nil),
MockUpdate: test.NewMockUpdateFn(nil),
MockGet: test.NewMockGetFn(nil),
MockUpdate: test.NewMockUpdateFn(nil),
MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
},
runner: &MockRunner{
MockAnsibleRunPolicy: func() *ansible.RunPolicy {
Expand Down Expand Up @@ -769,6 +771,9 @@ func TestCreateOrUpdate(t *testing.T) {
mg: &v1alpha1.AnsibleRun{},
},
fields: fields{
kube: &test.MockClient{
MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
},
runner: &MockRunner{
MockAnsibleRunPolicy: func() *ansible.RunPolicy {
return &ansible.RunPolicy{
Expand Down Expand Up @@ -816,6 +821,9 @@ func TestCreateOrUpdate(t *testing.T) {
mg: &v1alpha1.AnsibleRun{},
},
fields: fields{
kube: &test.MockClient{
MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
},
runner: &MockRunner{
MockAnsibleRunPolicy: func() *ansible.RunPolicy {
return &ansible.RunPolicy{
Expand Down
Loading