-
Notifications
You must be signed in to change notification settings - Fork 190
feat: Update sandbox status with phase #121
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,24 @@ import ( | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| // SandboxPhase is a simple, high-level summary of where the Sandbox is in its lifecycle. | ||
| type SandboxPhase string | ||
|
|
||
| const ( | ||
| // SandboxPhasePending means the sandbox is being created | ||
| SandboxPhasePending SandboxPhase = "Pending" | ||
| // SandboxPhaseRunning means the sandbox is running | ||
| SandboxPhaseRunning SandboxPhase = "Running" | ||
| // SandboxPhasePaused means the sandbox is paused | ||
| SandboxPhasePaused SandboxPhase = "Paused" | ||
| // SandboxPhaseTerminating means the sandbox is terminating | ||
| SandboxPhaseTerminating SandboxPhase = "Terminating" | ||
| // SandboxPhaseFailed means the sandbox has failed | ||
| SandboxPhaseFailed SandboxPhase = "Failed" | ||
| // SandboxPhaseExpired means the sandbox has expired. This is a terminal phase that can only be set when the ShutdownPolicy is Retain. | ||
| SandboxPhaseExpired SandboxPhase = "Expired" | ||
| ) | ||
|
|
||
| // ConditionType is a type of condition for a resource. | ||
| type ConditionType string | ||
|
|
||
|
|
@@ -149,6 +167,23 @@ type Lifecycle struct { | |
|
|
||
| // SandboxStatus defines the observed state of Sandbox. | ||
| type SandboxStatus struct { | ||
| // The phase of a Sandbox is a simple, high-level summary of where the Sandbox is in its lifecycle. | ||
| // The conditions array, the reason and message fields, and the individual container status arrays are | ||
| // more detail about the pod's status. | ||
| // There are five possible phase values: | ||
| // | ||
| // Pending: The Sandbox has been accepted by the Kubernetes system, but one or more of the Pod | ||
| // startup steps is not yet complete. | ||
| // Running: The Sandbox has been bound to a node, and all of the Pods have been created. At least | ||
| // one Pod is still running, or is in the process of starting or restarting. | ||
| // Paused: The Sandbox has been paused. | ||
| // Failed: All Pods in the Sandbox have terminated, and at least one Pod has terminated in a failure | ||
| // (exited with a non-zero exit code or was terminated by the system). | ||
| // Terminating: The Sandbox is terminating. | ||
| // | ||
| // +optional | ||
| Phase SandboxPhase `json:"phase,omitempty"` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Clients need to monitor the individual conditions that the controller exposes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you propose one condition per phase with true/false ? Or a single condition that is always true but reason is the phase.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be just a phase with a different field name. We need to avoid the anti-pattern of using enum, i.e. clients must know all possible reasons to understand the state. Also, condition status needs to be true/false/unknown. For example, we should have these conditions (some from standard conditions): Initialized, Ready, Expired, Terminating. Paused/Failed should be represented by Ready by setting it to false with appropriate Reason and Message.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As part of the Suspend and resume operation, we want to include very clear condition on when the Sandbox is being Suspended. More details on the lifecycle of the condition during a suspend and resume operation: Design. |
||
|
|
||
| // FQDN that is valid for default cluster settings | ||
| // Limitation: Hardcoded to the domain .cluster.local | ||
| // e.g. sandbox-example.default.svc.cluster.local | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,16 +102,25 @@ func (r *SandboxReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |
| // If the sandbox is being deleted, do nothing | ||
| if !sandbox.ObjectMeta.DeletionTimestamp.IsZero() { | ||
| log.Info("Sandbox is being deleted") | ||
| //sandbox.Status.Phase = sandboxv1alpha1.SandboxPhaseTerminating | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: is this comment intended ? |
||
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| // Set a default phase | ||
| if sandbox.Status.Phase == "" { | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhasePending | ||
| } | ||
|
|
||
| // Check if already marked as expired to avoid repeated operations, including cleanups | ||
| if sandboxMarkedExpired(sandbox) { | ||
| log.Info("Sandbox is already marked as expired") | ||
| oldStatus := sandbox.Status.DeepCopy() | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhaseExpired | ||
| err := r.updateStatus(ctx, oldStatus, sandbox) | ||
| // Note: The sandbox won't be deleted if shutdown policy is changed to delete after expiration. | ||
| // To delete an expired sandbox, the user should delete the sandbox instead of updating it. | ||
| // This keeps the controller code simple. | ||
| return ctrl.Result{}, nil | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Initialize trace ID for active resources missing an ID | ||
|
|
@@ -176,9 +185,20 @@ func (r *SandboxReconciler) reconcileChildResources(ctx context.Context, sandbox | |
| if pod == nil { | ||
| sandbox.Status.Replicas = 0 | ||
| sandbox.Status.LabelSelector = "" | ||
| if sandbox.Spec.Replicas != nil && *sandbox.Spec.Replicas == 0 { | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhasePaused | ||
| } | ||
| } else { | ||
| sandbox.Status.Replicas = 1 | ||
| sandbox.Status.LabelSelector = fmt.Sprintf("%s=%s", sandboxLabel, NameHash(sandbox.Name)) | ||
| switch pod.Status.Phase { | ||
| case corev1.PodRunning: | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhaseRunning | ||
| case corev1.PodPending: | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhasePending | ||
| case corev1.PodFailed: | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhaseFailed | ||
| } | ||
| } | ||
|
|
||
| // Reconcile Service | ||
|
|
@@ -225,9 +245,9 @@ func (r *SandboxReconciler) computeReadyCondition(sandbox *sandboxv1alpha1.Sandb | |
| } | ||
| } | ||
| } else { | ||
| if sandbox.Spec.Replicas != nil && *sandbox.Spec.Replicas == 0 { | ||
| message = "Pod does not exist, replicas is 0" | ||
| if sandbox.Status.Phase == sandboxv1alpha1.SandboxPhasePaused { | ||
| // This is intended behaviour. So marking it ready. | ||
| message = "Sandbox is paused" | ||
| podReady = true | ||
| } else { | ||
| message = "Pod does not exist" | ||
|
|
@@ -554,6 +574,7 @@ func (r *SandboxReconciler) handleSandboxExpiry(ctx context.Context, sandbox *sa | |
| if err := r.Delete(ctx, sandbox); err != nil && !k8serrors.IsNotFound(err) { | ||
| allErrors = errors.Join(allErrors, fmt.Errorf("failed to delete sandbox: %w", err)) | ||
| } else { | ||
| sandbox.Status.Phase = sandboxv1alpha1.SandboxPhaseTerminating | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only changes the in-memory sandbox. Will this sandbox status be updated? |
||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3977,6 +3977,8 @@ spec: | |
| - type | ||
| type: object | ||
| type: array | ||
| phase: | ||
| type: string | ||
| replicas: | ||
| format: int32 | ||
| type: integer | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.