diff --git a/pkg/workloadmanager/codeinterpreter_controller.go b/pkg/workloadmanager/codeinterpreter_controller.go index bce73a0d..1921eef7 100644 --- a/pkg/workloadmanager/codeinterpreter_controller.go +++ b/pkg/workloadmanager/codeinterpreter_controller.go @@ -91,7 +91,7 @@ func (r *CodeInterpreterReconciler) Reconcile(ctx context.Context, req ctrl.Requ } // Update status with ready condition - if err := r.updateStatus(ctx, codeInterpreter); err != nil { + if err := r.updateStatus(ctx, codeInterpreter, true, "Reconciled", "CodeInterpreter is ready"); err != nil { logger.Error(err, "failed to update status") return ctrl.Result{}, err } @@ -102,30 +102,49 @@ func (r *CodeInterpreterReconciler) Reconcile(ctx context.Context, req ctrl.Requ // updateStatus updates the CodeInterpreter status. It skips the API write // when the status is already up-to-date to avoid triggering a new watch event // that would re-enqueue the object unnecessarily. -func (r *CodeInterpreterReconciler) updateStatus(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter) error { +func (r *CodeInterpreterReconciler) updateStatus(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter, ready bool, reason, message string) error { + conditionStatus := metav1.ConditionFalse + if ready { + conditionStatus = metav1.ConditionTrue + } + existing := apimeta.FindStatusCondition(ci.Status.Conditions, "Ready") - if ci.Status.Ready && + if ci.Status.Ready == ready && existing != nil && - existing.Status == metav1.ConditionTrue && + existing.Status == conditionStatus && + existing.Reason == reason && + existing.Message == message && existing.ObservedGeneration == ci.Generation { return nil } - ci.Status.Ready = true + ci.Status.Ready = ready // SetStatusCondition only updates LastTransitionTime when the condition // Status actually changes, preventing spurious status writes that would // trigger an infinite reconciliation loop. apimeta.SetStatusCondition(&ci.Status.Conditions, metav1.Condition{ Type: "Ready", - Status: metav1.ConditionTrue, - Reason: "Reconciled", - Message: "CodeInterpreter is ready", + Status: conditionStatus, + Reason: reason, + Message: message, ObservedGeneration: ci.Generation, }) return r.Status().Update(ctx, ci) } +func (r *CodeInterpreterReconciler) validateChildOwnership(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter, child metav1.Object, kind string) error { + if metav1.IsControlledBy(child, ci) { + return nil + } + + ownershipErr := fmt.Errorf("existing %s %s/%s is not controlled by CodeInterpreter %s", kind, child.GetNamespace(), child.GetName(), ci.Name) + if err := r.updateStatus(ctx, ci, false, "OwnershipConflict", ownershipErr.Error()); err != nil { + return fmt.Errorf("%v: failed to update CodeInterpreter status: %w", ownershipErr, err) + } + return ownershipErr +} + // ensureSandboxTemplate ensures that a SandboxTemplate exists for this CodeInterpreter func (r *CodeInterpreterReconciler) ensureSandboxTemplate(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter) (ctrl.Result, error) { logger := log.FromContext(ctx) @@ -168,14 +187,15 @@ func (r *CodeInterpreterReconciler) ensureSandboxTemplate(ctx context.Context, c } if err := r.Create(ctx, sandboxTemplate); err != nil { - if !errors.IsAlreadyExists(err) { - return ctrl.Result{}, fmt.Errorf("failed to create SandboxTemplate: %w", err) - } + return ctrl.Result{}, fmt.Errorf("failed to create SandboxTemplate: %w", err) } return ctrl.Result{}, nil } else if err != nil { return ctrl.Result{}, fmt.Errorf("failed to get SandboxTemplate: %w", err) } + if err := r.validateChildOwnership(ctx, ci, sandboxTemplate, "SandboxTemplate"); err != nil { + return ctrl.Result{}, err + } // Update existing SandboxTemplate if needed. needsUpdate := false @@ -228,14 +248,15 @@ func (r *CodeInterpreterReconciler) ensureSandboxWarmPool(ctx context.Context, c } if err := r.Create(ctx, warmPool); err != nil { - if !errors.IsAlreadyExists(err) { - return fmt.Errorf("failed to create SandboxWarmPool: %w", err) - } + return fmt.Errorf("failed to create SandboxWarmPool: %w", err) } return nil } else if err != nil { return fmt.Errorf("failed to get SandboxWarmPool: %w", err) } + if err := r.validateChildOwnership(ctx, ci, warmPool, "SandboxWarmPool"); err != nil { + return err + } // Update existing SandboxWarmPool if needed needsUpdate := false @@ -267,8 +288,14 @@ func (r *CodeInterpreterReconciler) deleteSandboxWarmPool(ctx context.Context, c } else if err != nil { return fmt.Errorf("failed to get SandboxWarmPool: %w", err) } + if err := r.validateChildOwnership(ctx, ci, warmPool, "SandboxWarmPool"); err != nil { + return err + } - if err := r.Delete(ctx, warmPool); err != nil { + if err := r.Delete(ctx, warmPool, client.Preconditions{ + UID: &warmPool.UID, + ResourceVersion: &warmPool.ResourceVersion, + }); err != nil { if !errors.IsNotFound(err) { return fmt.Errorf("failed to delete SandboxWarmPool: %w", err) } @@ -287,8 +314,14 @@ func (r *CodeInterpreterReconciler) deleteSandboxTemplate(ctx context.Context, c } else if err != nil { return fmt.Errorf("failed to get SandboxTemplate: %w", err) } + if err := r.validateChildOwnership(ctx, ci, sandboxTemplate, "SandboxTemplate"); err != nil { + return err + } - if err := r.Delete(ctx, sandboxTemplate); err != nil { + if err := r.Delete(ctx, sandboxTemplate, client.Preconditions{ + UID: &sandboxTemplate.UID, + ResourceVersion: &sandboxTemplate.ResourceVersion, + }); err != nil { if !errors.IsNotFound(err) { return fmt.Errorf("failed to delete SandboxTemplate: %w", err) } diff --git a/pkg/workloadmanager/codeinterpreter_controller_test.go b/pkg/workloadmanager/codeinterpreter_controller_test.go index b33c65b2..6e3c50ac 100644 --- a/pkg/workloadmanager/codeinterpreter_controller_test.go +++ b/pkg/workloadmanager/codeinterpreter_controller_test.go @@ -22,9 +22,12 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" runtimev1alpha1 "github.com/volcano-sh/agentcube/pkg/apis/runtime/v1alpha1" @@ -53,7 +56,11 @@ func newTestReconcilerWithObjects(objects ...runtime.Object) *CodeInterpreterRec _ = extensionsv1alpha1.AddToScheme(scheme) _ = corev1.AddToScheme(scheme) - client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objects...).Build() + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&runtimev1alpha1.CodeInterpreter{}). + WithRuntimeObjects(objects...). + Build() return &CodeInterpreterReconciler{ Client: client, @@ -61,6 +68,25 @@ func newTestReconcilerWithObjects(objects ...runtime.Object) *CodeInterpreterRec } } +type replacingDeleteClient struct { + client.Client + replacement client.Object +} + +func (c *replacingDeleteClient) Delete(ctx context.Context, object client.Object, opts ...client.DeleteOption) error { + if err := c.Client.Delete(ctx, object); err != nil { + return err + } + if err := c.Create(ctx, c.replacement); err != nil { + return err + } + return c.Client.Delete(ctx, object, opts...) +} + +func replaceObjectBeforeDelete(reconciler *CodeInterpreterReconciler, replacement client.Object) { + reconciler.Client = &replacingDeleteClient{Client: reconciler.Client, replacement: replacement} +} + func stringPtr(s string) *string { return &s } @@ -71,6 +97,7 @@ func testCodeInterpreterWithWarmPool() *runtimev1alpha1.CodeInterpreter { ObjectMeta: metav1.ObjectMeta{ Name: "test-code-interpreter", Namespace: "default", + UID: "test-code-interpreter", }, Spec: runtimev1alpha1.CodeInterpreterSpec{ AuthMode: runtimev1alpha1.AuthModeNone, @@ -105,6 +132,9 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing. ObjectMeta: metav1.ObjectMeta{ Name: ci.Name, Namespace: ci.Namespace, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(ci, runtimev1alpha1.GroupVersion.WithKind("CodeInterpreter")), + }, }, Spec: extensionsv1alpha1.SandboxTemplateSpec{ NetworkPolicyManagement: extensionsv1alpha1.NetworkPolicyManagementManaged, @@ -118,7 +148,7 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing. }, }, } - reconciler := newTestReconcilerWithObjects(existing) + reconciler := newTestReconcilerWithObjects(ci, existing) _, err := reconciler.ensureSandboxTemplate(context.Background(), ci) assert.NoError(t, err) @@ -132,6 +162,181 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing. assert.Equal(t, extensionsv1alpha1.NetworkPolicyManagementUnmanaged, sandboxTemplate.Spec.NetworkPolicyManagement) } +func TestEnsureSandboxTemplateRejectsUnownedTemplate(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + existing := &extensionsv1alpha1.SandboxTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + }, + Spec: extensionsv1alpha1.SandboxTemplateSpec{ + PodTemplate: sandboxv1alpha1.PodTemplate{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "existing", + Image: "existing-image", + }}, + }, + }, + }, + } + reconciler := newTestReconcilerWithObjects(ci, existing) + + _, err := reconciler.ensureSandboxTemplate(context.Background(), ci) + assert.ErrorContains(t, err, "is not controlled by CodeInterpreter") + + sandboxTemplate := &extensionsv1alpha1.SandboxTemplate{} + err = reconciler.Get(context.Background(), types.NamespacedName{ + Name: ci.Name, + Namespace: ci.Namespace, + }, sandboxTemplate) + assert.NoError(t, err) + assert.Equal(t, "existing-image", sandboxTemplate.Spec.PodTemplate.Spec.Containers[0].Image) + + storedCI := &runtimev1alpha1.CodeInterpreter{} + err = reconciler.Get(context.Background(), types.NamespacedName{ + Name: ci.Name, + Namespace: ci.Namespace, + }, storedCI) + assert.NoError(t, err) + condition := apimeta.FindStatusCondition(storedCI.Status.Conditions, "Ready") + if assert.NotNil(t, condition) { + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, "OwnershipConflict", condition.Reason) + } +} + +func TestEnsureSandboxWarmPoolRejectsUnownedWarmPool(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + existing := &extensionsv1alpha1.SandboxWarmPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + }, + Spec: extensionsv1alpha1.SandboxWarmPoolSpec{ + Replicas: 7, + TemplateRef: extensionsv1alpha1.SandboxTemplateRef{ + Name: "existing-template", + }, + }, + } + reconciler := newTestReconcilerWithObjects(ci, existing) + + err := reconciler.ensureSandboxWarmPool(context.Background(), ci) + assert.ErrorContains(t, err, "is not controlled by CodeInterpreter") + + warmPool := &extensionsv1alpha1.SandboxWarmPool{} + err = reconciler.Get(context.Background(), types.NamespacedName{ + Name: ci.Name, + Namespace: ci.Namespace, + }, warmPool) + assert.NoError(t, err) + assert.Equal(t, int32(7), warmPool.Spec.Replicas) + assert.Equal(t, "existing-template", warmPool.Spec.TemplateRef.Name) +} + +func TestDeleteSandboxTemplateRejectsUnownedTemplate(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + existing := &extensionsv1alpha1.SandboxTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + }, + } + reconciler := newTestReconcilerWithObjects(ci, existing) + + err := reconciler.deleteSandboxTemplate(context.Background(), ci) + assert.ErrorContains(t, err, "is not controlled by CodeInterpreter") + + err = reconciler.Get(context.Background(), types.NamespacedName{ + Name: ci.Name, + Namespace: ci.Namespace, + }, &extensionsv1alpha1.SandboxTemplate{}) + assert.NoError(t, err) +} + +func TestDeleteSandboxWarmPoolRejectsUnownedWarmPool(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + existing := &extensionsv1alpha1.SandboxWarmPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + }, + } + reconciler := newTestReconcilerWithObjects(ci, existing) + + err := reconciler.deleteSandboxWarmPool(context.Background(), ci) + assert.ErrorContains(t, err, "is not controlled by CodeInterpreter") + + err = reconciler.Get(context.Background(), types.NamespacedName{ + Name: ci.Name, + Namespace: ci.Namespace, + }, &extensionsv1alpha1.SandboxWarmPool{}) + assert.NoError(t, err) +} + +func TestDeleteSandboxTemplateRejectsReplacement(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + original := &extensionsv1alpha1.SandboxTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + UID: "original-template", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(ci, runtimev1alpha1.GroupVersion.WithKind("CodeInterpreter")), + }, + }, + } + replacement := &extensionsv1alpha1.SandboxTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + UID: "replacement-template", + }, + } + reconciler := newTestReconcilerWithObjects(ci, original) + replaceObjectBeforeDelete(reconciler, replacement) + + err := reconciler.deleteSandboxTemplate(context.Background(), ci) + assert.True(t, apierrors.IsConflict(err)) + + stored := &extensionsv1alpha1.SandboxTemplate{} + err = reconciler.Get(context.Background(), client.ObjectKeyFromObject(replacement), stored) + assert.NoError(t, err) + assert.Equal(t, replacement.UID, stored.UID) +} + +func TestDeleteSandboxWarmPoolRejectsReplacement(t *testing.T) { + ci := testCodeInterpreterWithWarmPool() + original := &extensionsv1alpha1.SandboxWarmPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + UID: "original-warm-pool", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(ci, runtimev1alpha1.GroupVersion.WithKind("CodeInterpreter")), + }, + }, + } + replacement := &extensionsv1alpha1.SandboxWarmPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: ci.Name, + Namespace: ci.Namespace, + UID: "replacement-warm-pool", + }, + } + reconciler := newTestReconcilerWithObjects(ci, original) + replaceObjectBeforeDelete(reconciler, replacement) + + err := reconciler.deleteSandboxWarmPool(context.Background(), ci) + assert.True(t, apierrors.IsConflict(err)) + + stored := &extensionsv1alpha1.SandboxWarmPool{} + err = reconciler.Get(context.Background(), client.ObjectKeyFromObject(replacement), stored) + assert.NoError(t, err) + assert.Equal(t, replacement.UID, stored.UID) +} + func TestConvertToPodTemplate_RuntimeClassName_TableDriven(t *testing.T) { reconciler := setupTestReconciler()