Skip to content

Ensure .status.phase Is Saved During Resource Creation #189

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.

Sign up for GitHub

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

Open
alansy-nscale wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Ensure .status.phase Is Saved During Resource Creation #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7796c9
fix: persist `.status.phase` after `client.Create()` and default to `…
alansy-nscale Oct 10, 2025
c9e7d0a
feat: make `server.status.phase` optional and return `Unknown` when t…
alansy-nscale Oct 14, 2025
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
2 changes: 1 addition & 1 deletion pkg/apis/unikorn/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ type ServerStatus struct {
// Current service state of a cluster manager.
Conditions []unikornv1core.Condition `json:"conditions,omitempty"`
// Phase is the current lifecycle phase of the server.
Phase InstanceLifecyclePhase `json:"phase,omitempty"`
Phase *InstanceLifecyclePhase `json:"phase,omitempty"`
Copy link
Contributor

@spjmurray spjmurray Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, as this is really a string, the zero value of "" will trigger nothing to be generated. But mistakes can happen I guess, so the difference between set but empty, and not set may be handy. Although it causes the code to be a bit more messy :/

// PrivateIP is the private IP address of the server.
PrivateIP *string `json:"privateIP,omitempty"`
// PublicIP is the public IP address of the server.
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/unikorn/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions pkg/handler/server/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (

kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/utils/ptr"

"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -135,8 +136,6 @@ func (c *Client) Create(ctx context.Context, organizationID, projectID, identity
return nil, err
}

resource.Status.Phase = unikornv1.InstanceLifecyclePhasePending

if err := c.client.Create(ctx, resource); err != nil {
return nil, errors.OAuth2ServerError("unable to create server").WithError(err)
}
Expand Down Expand Up @@ -263,7 +262,7 @@ func (c *Client) Start(ctx context.Context, organizationID, projectID, identityI

// REVIEW_ME: Do we want to track who started the server, and when?
updated := current.DeepCopy()
updated.Status.Phase = unikornv1.InstanceLifecyclePhasePending
updated.Status.Phase = ptr.To(unikornv1.InstanceLifecyclePhasePending)
Copy link
Contributor

@spjmurray spjmurray Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, I thought you'd not use this API, I am pleasantly surprised 💯


if err := c.client.Status().Patch(ctx, updated, client.MergeFrom(current)); err != nil {
return errors.OAuth2ServerError("failed to patch server").WithError(err)
Expand Down Expand Up @@ -300,7 +299,7 @@ func (c *Client) Stop(ctx context.Context, organizationID, projectID, identityID

// REVIEW_ME: Do we want to track who stopped the server, and when?
updated := current.DeepCopy()
updated.Status.Phase = unikornv1.InstanceLifecyclePhaseStopping
updated.Status.Phase = ptr.To(unikornv1.InstanceLifecyclePhaseStopping)

if err := c.client.Status().Patch(ctx, updated, client.MergeFrom(current)); err != nil {
return errors.OAuth2ServerError("failed to patch server").WithError(err)
Expand Down
10 changes: 7 additions & 3 deletions pkg/handler/server/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,12 @@ func convertUserData(in []byte) *[]byte {
return &in
}

func convertInstanceLifecyclePhase(in unikornv1.InstanceLifecyclePhase) openapi.InstanceLifecyclePhase {
switch in {
func convertInstanceLifecyclePhase(in *unikornv1.InstanceLifecyclePhase) openapi.InstanceLifecyclePhase {
if in == nil {
return openapi.Unknown
}

switch *in {
case unikornv1.InstanceLifecyclePhasePending:
return openapi.Pending
case unikornv1.InstanceLifecyclePhaseRunning:
Expand All @@ -150,7 +154,7 @@ func convertInstanceLifecyclePhase(in unikornv1.InstanceLifecyclePhase) openapi.
case unikornv1.InstanceLifecyclePhaseStopped:
return openapi.Stopped
default:
return ""
return openapi.Unknown
}
}

Expand Down
163 changes: 82 additions & 81 deletions pkg/openapi/schema.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/openapi/server.spec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,7 @@ components:
description: The lifecycle phase of an instance.
type: string
enum:
- Unknown
- Pending
- Running
- Stopping
Expand Down
1 change: 1 addition & 0 deletions pkg/openapi/types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions pkg/providers/openstack/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/utils/ptr"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -1669,8 +1670,8 @@ func setServerHealthStatus(server *unikornv1.Server, openstackserver *servers.Se
// https://docs.openstack.org/api-guide/compute/server_concepts.html
func setServerPhase(ctx context.Context, server *unikornv1.Server, openstackserver *servers.Server) {
// Default to `Pending` if the phase is not already set. This should only happen to old servers created before we had phases.
if server.Status.Phase == "" {
server.Status.Phase = unikornv1.InstanceLifecyclePhasePending
if server.Status.Phase == nil {
server.Status.Phase = ptr.To(unikornv1.InstanceLifecyclePhasePending)
}

if openstackserver == nil {
Expand All @@ -1681,9 +1682,9 @@ func setServerPhase(ctx context.Context, server *unikornv1.Server, openstackserv
case servers.NOSTATE:
// No state information available. We will keep the phase as it is.
case servers.RUNNING:
server.Status.Phase = unikornv1.InstanceLifecyclePhaseRunning
server.Status.Phase = ptr.To(unikornv1.InstanceLifecyclePhaseRunning)
case servers.SHUTDOWN:
server.Status.Phase = unikornv1.InstanceLifecyclePhaseStopped
server.Status.Phase = ptr.To(unikornv1.InstanceLifecyclePhaseStopped)
case servers.CRASHED:
// REVIEW_ME: What should we do when the server crashes?
case servers.PAUSED, servers.SUSPENDED:
Expand Down
Loading