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.

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

A reconciler should not require the DSCI to perform cleanup action to avoid deadlock #1429

Merged
merged 1 commit into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion controllers/dscinitialization/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ func TestDataScienceClusterInitialization(t *testing.T) {

var testScheme = runtime.NewScheme()

//nolint:fatcontext
var _ = BeforeSuite(func() {
// can't use suite's context as the manager should survive the function
//nolint:fatcontext
Expand Down
1 change: 0 additions & 1 deletion controllers/webhook/webhook_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ func TestAPIs(t *testing.T) {
RunSpecs(t, "Webhook Suite")
}

//nolint:fatcontext
var _ = BeforeSuite(func() {
// can't use suite's context as the manager should survive the function
//nolint:fatcontext
Expand Down
94 changes: 63 additions & 31 deletions pkg/controller/reconciler/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,48 +108,80 @@
return ctrl.Result{}, client.IgnoreNotFound(err)
}

dscil := dsciv1.DSCInitializationList{}
if err := r.Client.List(ctx, &dscil); err != nil {
return ctrl.Result{}, err
if !res.GetDeletionTimestamp().IsZero() {
if err := r.delete(ctx, res); err != nil {
return ctrl.Result{}, err
}

Check warning on line 114 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L111-L114

Added lines #L111 - L114 were not covered by tests
}

if len(dscil.Items) != 1 {
return ctrl.Result{}, errors.New("unable to find DSCInitialization")
if err := r.apply(ctx, res); err != nil {
return ctrl.Result{}, err

Check warning on line 118 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L117-L118

Added lines #L117 - L118 were not covered by tests
}

return ctrl.Result{}, nil

Check warning on line 121 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L121

Added line #L121 was not covered by tests
}

func (r *Reconciler[T]) delete(ctx context.Context, res client.Object) error {
l := log.FromContext(ctx)
l.Info("delete")

Check warning on line 127 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L124-L127

Added lines #L124 - L127 were not covered by tests
rr := types.ReconciliationRequest{
Client: r.Client,
Manager: r.m,
Instance: res,
DSCI: &dscil.Items[0],
Release: r.Release,
Manifests: make([]types.ManifestInfo, 0),

// The DSCI should not be required when deleting a component, if the
// component requires some additional info, then such info should be
// stored as part of the spec/status
DSCI: nil,

Check warning on line 138 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L134-L138

Added lines #L134 - L138 were not covered by tests
}

// Handle deletion
if !res.GetDeletionTimestamp().IsZero() {
// Execute finalizers
for _, action := range r.Finalizer {
l.V(3).Info("Executing finalizer", "action", action)

actx := log.IntoContext(
ctx,
l.WithName(actions.ActionGroup).WithName(action.String()),
)

if err := action(actx, &rr); err != nil {
se := odherrors.StopError{}
if !errors.As(err, &se) {
l.Error(err, "Failed to execute finalizer", "action", action)
return ctrl.Result{}, err
}

l.V(3).Info("detected stop marker", "action", action)
break
// Execute finalizers
for _, action := range r.Finalizer {
l.V(3).Info("Executing finalizer", "action", action)

actx := log.IntoContext(
ctx,
l.WithName(actions.ActionGroup).WithName(action.String()),
)

if err := action(actx, &rr); err != nil {
se := odherrors.StopError{}
if !errors.As(err, &se) {
l.Error(err, "Failed to execute finalizer", "action", action)
return err

Check warning on line 154 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L141-L154

Added lines #L141 - L154 were not covered by tests
}

l.V(3).Info("detected stop marker", "action", action)
break

Check warning on line 158 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L157-L158

Added lines #L157 - L158 were not covered by tests
}
}

return nil

Check warning on line 162 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L162

Added line #L162 was not covered by tests
}

return ctrl.Result{}, nil
func (r *Reconciler[T]) apply(ctx context.Context, res client.Object) error {
l := log.FromContext(ctx)
l.Info("apply")

dscil := dsciv1.DSCInitializationList{}
if err := r.Client.List(ctx, &dscil); err != nil {
return err
}

Check warning on line 172 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L165-L172

Added lines #L165 - L172 were not covered by tests

if len(dscil.Items) != 1 {
return errors.New("unable to find DSCInitialization")
}

Check warning on line 176 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L174-L176

Added lines #L174 - L176 were not covered by tests

rr := types.ReconciliationRequest{
Client: r.Client,
Manager: r.m,
Instance: res,
DSCI: &dscil.Items[0],
Release: r.Release,
Manifests: make([]types.ManifestInfo, 0),

Check warning on line 184 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L178-L184

Added lines #L178 - L184 were not covered by tests
}

// Execute actions
Expand All @@ -165,24 +197,24 @@
se := odherrors.StopError{}
if !errors.As(err, &se) {
l.Error(err, "Failed to execute action", "action", action)
return ctrl.Result{}, err
return err

Check warning on line 200 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L200

Added line #L200 was not covered by tests
}

l.V(3).Info("detected stop marker", "action", action)
break
}
}

err = r.Client.ApplyStatus(
err := r.Client.ApplyStatus(

Check warning on line 208 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L208

Added line #L208 was not covered by tests
ctx,
rr.Instance,
client.FieldOwner(r.name),
client.ForceOwnership,
)

if err != nil {
return ctrl.Result{}, err
return err

Check warning on line 216 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L216

Added line #L216 was not covered by tests
}

return ctrl.Result{}, err
return nil

Check warning on line 219 in pkg/controller/reconciler/reconciler.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/reconciler/reconciler.go#L219

Added line #L219 was not covered by tests
}
Loading