Skip to content

Commit 5d978ba

Browse files
committed
fix(lifecycle): clarify one-shot completion semantics
1 parent 9c44325 commit 5d978ba

5 files changed

Lines changed: 73 additions & 4 deletions

File tree

cmd/agent/main.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ func main() {
120120
Metrics: mc,
121121
Net: prodNet,
122122
Alloc: alloc,
123+
Recorder: mgr.GetEventRecorderFor("imp-agent"),
123124
}).SetupWithManager(mgr); err != nil {
124125
log.Error(err, "Unable to set up ImpVMReconciler")
125126
os.Exit(1)

internal/agent/reconciler.go

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@ import (
1212
"go.opentelemetry.io/otel"
1313
"go.opentelemetry.io/otel/attribute"
1414
"go.opentelemetry.io/otel/trace"
15+
corev1 "k8s.io/api/core/v1"
1516
apierrors "k8s.io/apimachinery/pkg/api/errors"
1617
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1718
"k8s.io/apimachinery/pkg/runtime"
19+
"k8s.io/client-go/tools/record"
1820
"k8s.io/client-go/util/retry"
1921
ctrl "sigs.k8s.io/controller-runtime"
2022
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -44,6 +46,8 @@ type ImpVMReconciler struct {
4446
// StartTimeout is how long a VM may remain in Starting before being
4547
// transitioned to Failed. Defaults to 5 minutes when zero.
4648
StartTimeout time.Duration
49+
// Recorder emits lifecycle events for VM completion/failure.
50+
Recorder record.EventRecorder
4751
}
4852

4953
// +kubebuilder:rbac:groups=imp.dev,resources=impvms,verbs=get;list;watch;update;patch
@@ -266,8 +270,9 @@ func (r *ImpVMReconciler) handleRunning(ctx context.Context, vm *impdevv1alpha1.
266270
}
267271
}
268272

269-
log.Info("VM process exited", "lifecycle", vm.Spec.Lifecycle)
270-
if vm.Spec.Lifecycle == impdevv1alpha1.VMLifecycleEphemeral {
273+
lifecycle := vmLifecycleOrDefault(vm)
274+
log.Info("VM process exited", "lifecycle", lifecycle)
275+
if lifecycle == impdevv1alpha1.VMLifecycleEphemeral {
271276
return r.finishSucceeded(ctx, vm)
272277
}
273278
return r.finishFailed(ctx, vm)
@@ -317,11 +322,15 @@ func (r *ImpVMReconciler) finishSucceeded(ctx context.Context, vm *impdevv1alpha
317322
// Status patch — take base AFTER spec patch so resourceVersion is current.
318323
base := vm.DeepCopy()
319324
vm.Status.Phase = impdevv1alpha1.VMPhaseSucceeded
325+
vm.Status.StartedAt = nil
320326
vm.Status.IP = ""
321327
vm.Status.RuntimePID = 0
322328
if err := r.Status().Patch(ctx, vm, client.MergeFrom(base)); err != nil {
323329
return ctrl.Result{}, err
324330
}
331+
if r.Recorder != nil {
332+
r.Recorder.Event(vm, corev1.EventTypeNormal, "Completed", "VM exited and was marked Succeeded")
333+
}
325334
return ctrl.Result{}, nil
326335
}
327336

@@ -333,9 +342,19 @@ func (r *ImpVMReconciler) finishFailed(ctx context.Context, vm *impdevv1alpha1.I
333342
if err := r.Status().Patch(ctx, vm, client.MergeFrom(base)); err != nil {
334343
return ctrl.Result{}, err
335344
}
345+
if r.Recorder != nil {
346+
r.Recorder.Event(vm, corev1.EventTypeWarning, "ProcessExited", "VM process exited and was marked Failed")
347+
}
336348
return ctrl.Result{}, nil
337349
}
338350

351+
func vmLifecycleOrDefault(vm *impdevv1alpha1.ImpVM) impdevv1alpha1.VMLifecycle {
352+
if vm.Spec.Lifecycle == "" {
353+
return impdevv1alpha1.VMLifecycleEphemeral
354+
}
355+
return vm.Spec.Lifecycle
356+
}
357+
339358
// clearOwnership clears spec.nodeName + status ip/pid after Terminating stop.
340359
func (r *ImpVMReconciler) clearOwnership(ctx context.Context, vm *impdevv1alpha1.ImpVM) (ctrl.Result, error) {
341360
specBase := vm.DeepCopy()

internal/agent/reconciler_test.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,43 @@ var _ = Describe("ImpVM Agent: ephemeral exit → Succeeded", func() {
100100
Expect(updated.Status.Phase).To(Equal(impdevv1alpha1.VMPhaseSucceeded))
101101
Expect(updated.Spec.NodeName).To(BeEmpty())
102102
})
103+
104+
It("treats empty lifecycle as ephemeral default and marks Succeeded", func() {
105+
driver := NewStubDriver()
106+
107+
vm := &impdevv1alpha1.ImpVM{
108+
ObjectMeta: metav1.ObjectMeta{
109+
Name: "tc2-default-ephemeral", Namespace: "default",
110+
Finalizers: []string{"imp/finalizer"},
111+
},
112+
Spec: impdevv1alpha1.ImpVMSpec{
113+
NodeName: testNode,
114+
// Lifecycle intentionally omitted to verify default behavior on old/un-defaulted objects.
115+
},
116+
}
117+
Expect(k8sClient.Create(ctx, vm)).To(Succeed())
118+
DeferCleanup(func() { k8sClient.Delete(ctx, vm) }) //nolint:errcheck
119+
120+
pid, err := driver.Start(ctx, vm)
121+
Expect(err).NotTo(HaveOccurred())
122+
base := vm.DeepCopy()
123+
vm.Status.Phase = impdevv1alpha1.VMPhaseRunning
124+
vm.Status.RuntimePID = pid
125+
vm.Status.IP = "192.168.100.11"
126+
Expect(k8sClient.Status().Patch(ctx, vm, client.MergeFrom(base))).To(Succeed())
127+
128+
Expect(driver.Stop(ctx, vm)).To(Succeed())
129+
130+
_, err = newReconciler(driver).Reconcile(ctx, reconcile.Request{
131+
NamespacedName: types.NamespacedName{Name: "tc2-default-ephemeral", Namespace: "default"},
132+
})
133+
Expect(err).NotTo(HaveOccurred())
134+
135+
updated := &impdevv1alpha1.ImpVM{}
136+
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "tc2-default-ephemeral", Namespace: "default"}, updated)).To(Succeed())
137+
Expect(updated.Status.Phase).To(Equal(impdevv1alpha1.VMPhaseSucceeded))
138+
Expect(updated.Spec.NodeName).To(BeEmpty())
139+
})
103140
})
104141

105142
var _ = Describe("ImpVM Agent: persistent exit → Failed", func() {

internal/controller/impvm_conditions.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ func setReadyFromPhase(vm *impdevv1alpha1.ImpVM) {
4141
switch vm.Status.Phase {
4242
case impdevv1alpha1.VMPhaseRunning:
4343
setCondition(vm, ConditionReady, metav1.ConditionTrue, "Running", "VM is running")
44-
case impdevv1alpha1.VMPhaseFailed, impdevv1alpha1.VMPhaseSucceeded:
45-
setCondition(vm, ConditionReady, metav1.ConditionFalse, string(vm.Status.Phase), "VM is not running")
44+
case impdevv1alpha1.VMPhaseSucceeded:
45+
setCondition(vm, ConditionReady, metav1.ConditionFalse, "Completed", "VM completed successfully")
46+
case impdevv1alpha1.VMPhaseFailed:
47+
setCondition(vm, ConditionReady, metav1.ConditionFalse, "Failed", "VM failed")
4648
default:
4749
setCondition(vm, ConditionReady, metav1.ConditionUnknown, "Waiting", "Waiting for VM to start")
4850
}

internal/controller/impvm_coverage_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,16 @@ var _ = Describe("setReadyFromPhase", func() {
117117
Entry("Succeeded → False", impdevv1alpha1.VMPhaseSucceeded, metav1.ConditionFalse),
118118
Entry("Pending → Unknown", impdevv1alpha1.VMPhasePending, metav1.ConditionUnknown),
119119
)
120+
121+
It("sets explicit completion reason/message for Succeeded phase", func() {
122+
vm := &impdevv1alpha1.ImpVM{}
123+
vm.Status.Phase = impdevv1alpha1.VMPhaseSucceeded
124+
setReadyFromPhase(vm)
125+
c := apimeta.FindStatusCondition(vm.Status.Conditions, ConditionReady)
126+
Expect(c).NotTo(BeNil())
127+
Expect(c.Reason).To(Equal("Completed"))
128+
Expect(c.Message).To(Equal("VM completed successfully"))
129+
})
120130
})
121131

122132
// ─── countRunningVMs ─────────────────────────────────────────────────────────

0 commit comments

Comments
 (0)