Provisioning: Improve connection deletion error handling with undelete
When finalizer removal fails, the connection is now "undeleted" by removing the DeletionTimestamp. This prevents connections from being stuck in deletion state and allows users to retry deletion later. Additionally: - Expand retry logic to handle more transient errors (not just ServiceUnavailable) - Add isTransientError helper to detect retriable errors - Add comprehensive tests for undelete behavior and transient error detection This ensures that if the controller cannot remove the finalizer due to transient errors (network issues, API timeouts, etc.), the connection returns to normal state rather than remaining stuck in deletion.
This commit is contained in:
Roberto Jimenez Sanchez
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
@@ -156,13 +157,14 @@ func (cc *ConnectionController) processNextWorkItem(ctx context.Context) bool {
|
||||
return true
|
||||
}
|
||||
|
||||
if !apierrors.IsServiceUnavailable(err) {
|
||||
logger.Info("ConnectionController will not retry")
|
||||
// Check if error is transient and should be retried
|
||||
if !isTransientError(err) {
|
||||
logger.Info("ConnectionController will not retry (non-transient error)")
|
||||
cc.queue.Forget(item)
|
||||
return true
|
||||
}
|
||||
|
||||
logger.Info("ConnectionController will retry as service is unavailable")
|
||||
logger.Info("ConnectionController will retry (transient error)")
|
||||
utilruntime.HandleError(fmt.Errorf("%v failed with: %v", item, err))
|
||||
cc.queue.AddRateLimited(item)
|
||||
|
||||
@@ -313,12 +315,77 @@ func (cc *ConnectionController) handleDelete(ctx context.Context, conn *provisio
|
||||
FieldManager: "provisioning-connection-controller",
|
||||
})
|
||||
if err != nil {
|
||||
return fmt.Errorf("remove finalizer: %w", err)
|
||||
// If we can't remove the finalizer, undelete the connection so it can be retried later
|
||||
// This prevents the connection from being stuck in deletion state
|
||||
logger.Error("failed to remove finalizer, undeleting connection", "error", err)
|
||||
undeleteErr := cc.undeleteConnection(ctx, conn, err)
|
||||
if undeleteErr != nil {
|
||||
return fmt.Errorf("remove finalizer: %w; failed to undelete: %w", err, undeleteErr)
|
||||
}
|
||||
return fmt.Errorf("remove finalizer: %w (connection has been undeleted, deletion can be retried)", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// undeleteConnection removes the DeletionTimestamp to "undelete" the connection
|
||||
// This is used when finalizer removal fails, allowing the deletion to be retried later
|
||||
func (cc *ConnectionController) undeleteConnection(ctx context.Context, conn *provisioning.Connection, originalErr error) error {
|
||||
logger := logging.FromContext(ctx)
|
||||
logger.Info("undeleting connection due to finalizer removal failure", "error", originalErr.Error())
|
||||
|
||||
// Remove DeletionTimestamp by patching it to null
|
||||
_, err := cc.client.Connections(conn.GetNamespace()).
|
||||
Patch(ctx, conn.Name, types.JSONPatchType, []byte(`[
|
||||
{ "op": "remove", "path": "/metadata/deletionTimestamp" }
|
||||
]`), metav1.PatchOptions{
|
||||
FieldManager: "provisioning-connection-controller",
|
||||
})
|
||||
if err != nil {
|
||||
logger.Error("failed to undelete connection", "error", err)
|
||||
return fmt.Errorf("undelete connection: %w", err)
|
||||
}
|
||||
|
||||
logger.Info("connection undeleted successfully, deletion can be retried")
|
||||
return nil
|
||||
}
|
||||
|
||||
// isTransientError determines if an error is transient and should be retried
|
||||
func isTransientError(err error) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check for Kubernetes API transient errors
|
||||
if apierrors.IsServiceUnavailable(err) {
|
||||
return true
|
||||
}
|
||||
if apierrors.IsServerTimeout(err) {
|
||||
return true
|
||||
}
|
||||
if apierrors.IsTooManyRequests(err) {
|
||||
return true
|
||||
}
|
||||
if apierrors.IsInternalError(err) {
|
||||
return true
|
||||
}
|
||||
if apierrors.IsTimeout(err) {
|
||||
return true
|
||||
}
|
||||
|
||||
// Check for network errors
|
||||
var netErr net.Error
|
||||
if errors.As(err, &netErr) {
|
||||
if netErr.Timeout() {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
// Check for connection errors
|
||||
var opErr *net.OpError
|
||||
return errors.As(err, &opErr)
|
||||
}
|
||||
|
||||
// shouldCheckHealth determines if a connection health check should be performed.
|
||||
func (cc *ConnectionController) shouldCheckHealth(conn *provisioning.Connection) bool {
|
||||
// If the connection has been updated, always check health
|
||||
|
||||
@@ -3,15 +3,18 @@ package controller
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/mock"
|
||||
"github.com/stretchr/testify/require"
|
||||
apierrors "k8s.io/apimachinery/pkg/api/errors"
|
||||
"k8s.io/apimachinery/pkg/apis/meta/internalversion"
|
||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
||||
"k8s.io/apimachinery/pkg/runtime"
|
||||
"k8s.io/apimachinery/pkg/runtime/schema"
|
||||
"k8s.io/apimachinery/pkg/types"
|
||||
"k8s.io/apimachinery/pkg/watch"
|
||||
"k8s.io/client-go/rest"
|
||||
@@ -527,7 +530,7 @@ func TestConnectionController_handleDelete(t *testing.T) {
|
||||
expectFinalizerRemoved: false,
|
||||
},
|
||||
{
|
||||
name: "error removing finalizer, should return error",
|
||||
name: "error removing finalizer, should undelete connection",
|
||||
connection: &provisioning.Connection{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
Name: "test-conn",
|
||||
@@ -542,11 +545,24 @@ func TestConnectionController_handleDelete(t *testing.T) {
|
||||
}, nil)
|
||||
},
|
||||
connectionSetup: func(m *mockConnectionInterface) {
|
||||
m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
|
||||
// First patch fails (remove finalizer)
|
||||
m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.MatchedBy(func(data []byte) bool {
|
||||
return string(data) == `[
|
||||
{ "op": "remove", "path": "/metadata/finalizers" }
|
||||
]`
|
||||
}), metav1.PatchOptions{
|
||||
FieldManager: "provisioning-connection-controller",
|
||||
}, mock.Anything).Return(nil, errors.New("patch error"))
|
||||
}, mock.Anything).Return(nil, errors.New("patch error")).Once()
|
||||
// Second patch succeeds (undelete - remove DeletionTimestamp)
|
||||
m.On("Patch", ctx, "test-conn", types.JSONPatchType, mock.MatchedBy(func(data []byte) bool {
|
||||
return string(data) == `[
|
||||
{ "op": "remove", "path": "/metadata/deletionTimestamp" }
|
||||
]`
|
||||
}), metav1.PatchOptions{
|
||||
FieldManager: "provisioning-connection-controller",
|
||||
}, mock.Anything).Return(&provisioning.Connection{}, nil).Once()
|
||||
},
|
||||
expectedError: "remove finalizer: patch error",
|
||||
expectedError: "remove finalizer: patch error (connection has been undeleted, deletion can be retried)",
|
||||
expectFinalizerRemoved: false,
|
||||
},
|
||||
{
|
||||
@@ -612,12 +628,70 @@ func TestConnectionController_handleDelete(t *testing.T) {
|
||||
connInterface.AssertCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{
|
||||
FieldManager: "provisioning-connection-controller",
|
||||
}, mock.Anything)
|
||||
} else {
|
||||
connInterface.AssertNotCalled(t, "Patch", ctx, "test-conn", types.JSONPatchType, mock.Anything, metav1.PatchOptions{}, mock.Anything)
|
||||
} else if tt.expectedError != "" && strings.Contains(tt.expectedError, "undeleted") {
|
||||
// For undelete case, we expect both patches to be called (remove finalizer fails, then undelete succeeds)
|
||||
connInterface.AssertNumberOfCalls(t, "Patch", 2)
|
||||
}
|
||||
// For other error cases (repositories exist), no successful patch should occur
|
||||
|
||||
repoLister.AssertExpectations(t)
|
||||
connInterface.AssertExpectations(t)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsTransientError(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
err error
|
||||
expected bool
|
||||
}{
|
||||
{
|
||||
name: "nil error",
|
||||
err: nil,
|
||||
expected: false,
|
||||
},
|
||||
{
|
||||
name: "service unavailable",
|
||||
err: apierrors.NewServiceUnavailable("service unavailable"),
|
||||
expected: true,
|
||||
},
|
||||
{
|
||||
name: "server timeout",
|
||||
err: apierrors.NewServerTimeout(schema.GroupResource{}, "operation", 0),
|
||||
expected: true,
|
||||
},
|
||||
{
|
||||
name: "too many requests",
|
||||
err: apierrors.NewTooManyRequests("too many requests", 0),
|
||||
expected: true,
|
||||
},
|
||||
{
|
||||
name: "internal error",
|
||||
err: apierrors.NewInternalError(errors.New("internal error")),
|
||||
expected: true,
|
||||
},
|
||||
{
|
||||
name: "not found error",
|
||||
err: apierrors.NewNotFound(schema.GroupResource{}, "resource"),
|
||||
expected: false,
|
||||
},
|
||||
{
|
||||
name: "forbidden error",
|
||||
err: apierrors.NewForbidden(schema.GroupResource{}, "resource", errors.New("forbidden")),
|
||||
expected: false,
|
||||
},
|
||||
{
|
||||
name: "generic error",
|
||||
err: errors.New("generic error"),
|
||||
expected: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
result := isTransientError(tt.err)
|
||||
assert.Equal(t, tt.expected, result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user