feat(cmd): Add '--rollout' flag when creating by craciunoiuc · Pull Request #309 · unikraft-cloud/cli

craciunoiuc · 2026-05-07T13:41:14Z

Open to UI/UX suggestions, right now it prints every new instance that was rolled

Closes: TOOL-794

Copilot

Pull request overview

Adds support for a --rollout mode to instance create, intended to replace existing instances in the same service group that use the same image “base name”, while also adjusting how service-group and volume identifiers are parsed during instance creation.

Changes:

Add --rollout flag to instance create and route execution through a new runRollout workflow.
Modify parsing behavior for --service and --volume-style inputs (notably InstanceService.UnmarshalText and InstanceVolume.UnmarshalText).
Remove the client-side “volume metro mismatch” validation during instance creation.

Comments suppressed due to low confidence (1)

internal/cmd/instances.go:783

The volume creation path no longer validates vol.Metro against the instance metro (unlike the service-group check below). If a volume is specified via structured input (--set volumes.0.metro=..., JSON/YAML, or if text parsing supports metro/...), the CLI will now proceed and fail later with a less actionable API error; consider restoring the metro-mismatch validation for volumes when vol.Metro is set.

		case "volumes":
			for _, vol := range field.Create.Set.([]*InstanceVolume) {
				reqVol := platform.CreateInstanceRequestVolume{
					At: vol.At,
				}
				if vol.UUID != "" {
					reqVol.Uuid = &vol.UUID
				}
				if vol.Name != "" {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

craciunoiuc · 2026-05-12T15:45:01Z

I reworked the implementation as you asked @jedevc

Right now only instance rollout is implemented, but we can do other also I think

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

internal/cmd/instances.go:904

The volume link type (Link[Volume]) includes a Metro field, but the create path no longer validates that an explicitly metro-qualified volume matches the instance metro. Without this check, a user can specify e.g. "sfo/myvol" while creating in another metro and the metro will be silently ignored in the API request (only Name/UUID are sent). Consider restoring the metro-mismatch validation (similar to the service group check) or explicitly rejecting metro-qualified volume references here.

		case "volumes":
			for _, vol := range field.Create.Set.([]*InstanceVolume) {
				reqVol := platform.CreateInstanceRequestVolume{
					At: vol.At,
				}
				if vol.UUID != "" {
					reqVol.Uuid = &vol.UUID
				}
				if vol.Name != "" {
					reqVol.Name = &vol.Name
				}
				if vol.Size > 0 {
					reqVol.SizeMb = new(uint64(vol.Size))
				}
				if vol.Readonly {
					reqVol.Readonly = &vol.Readonly
				}
				req.Volumes = append(req.Volumes, reqVol)
			}

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

internal/cmd/instances.go:1229

Calling Instance{}.Delete directly bypasses the sandbox wrappers used by resource commands, so --rollout can delete matching service instances even when they are not tracked by the active sandbox. This breaks sandbox isolation and can remove resources that normal delete commands would reject.

		delErr := (Instance{}).Delete(ctx, []resource.Resource{old})

craciunoiuc · 2026-05-19T05:25:40Z

something funky going on with tests, probably server is misbehaving. Will need to wait till I can make sure tests pass

craciunoiuc · 2026-06-02T12:10:15Z

One thing left to discuss here is if we're going for the loop-wait-retry logic

If yes, then feel free to merge, if not, then we got to wait for the platform changes to go in

I would say we leave as is because a rollout can take significantly more than a simple start or restart, si it's fine to have a more complex logic in place (at least conceptually)

This adds sequential rolling over services Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>

Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>

jedevc · 2026-07-28T08:33:31Z

Before merging the timeouts stuff here, could we get #223 in first?

jedevc · 2026-07-28T08:34:14Z

+	if rollout && req.Image == nil && req.Template == nil {
+		return nil, fmt.Errorf("--rollout requires an image or template (use --image or --template)")
+	}


Unnecessary, duplicate check below.

jedevc · 2026-07-28T08:36:32Z

 	return results, nil
 }

+func (Instance) Rollout(ctx context.Context, created []resource.Resource, fields []resource.Field) error {


Which of this logic is actually specific to just instances. I was imagining that we would just be able to use the existing delete methods.

If it's this specialized, there's no point in it being a special case like this, your choice to go back, or to make it more generic.

At some point we wanted to do rollout for all resources, not just instances

But each wait and creation is unique so you got to implement this in every resource right?

jedevc · 2026-07-28T08:37:30Z

+		if inst.Image.Reference == nil {
+			continue
+		}
+		if inst.Image.Reference.Name() == newImageName {


I thought we didn't want to do rollouts based on image name - we moved over from doing that in kraftlet.

I think we should just do it based on the service.

Hmm, I didn't know about this?

So just rollout everything in a service?

jedevc · 2026-07-28T08:38:47Z

+			// Create an additional replacement instance with a modified name.
+			rolloutFields := slices.Clone(fields)
+			for j := range rolloutFields {


Why are we doing more creation after our existing creation? We should do all the creation in one step, then do all our deletions.

Also, surely we could do bulk creation using replicas.

Well this is the exact opposite of what we want 😓

If you have a service of max size 16 (default), and you have 9 instances in it, it means you can't rollout

If you do it sequentially, it means it will actually fit even if you are at 15/16 used

Alternatively we can add a --sequential flag to rollout

jedevc · 2026-07-28T08:40:01Z

+		if waitErr := group.DoMetro(ctx, g, metro, func(ctx context.Context, mc multimetro.MetroClient) error {
+			_, err := mc.WaitInstanceByUUID(ctx, newInst.UUID, platform.WaitInstanceByUUIDRequestBody{
+				State: platform.InstanceStateRunning,
+			})
+			return err
+		}); waitErr != nil {
+			return fmt.Errorf("waiting for new instance to be running: %w", waitErr)
+		}


I think this should be configurable - we should be able to configure some timeout, in which we check that after the instance has started running that it is still running after t time.

We don't really have a good way to check the health of an instance, so we need some way to make sure that we don't start up an instance, it immediately crashes, and then we delete the old one.

How would that flag look like like --recheck 5s

Aka, wait 5s and check if the instance is still running? I can add that, but one could argue that it's on them if they deploy a service that crashes after X seconds

If it were to crash at boot it would do starting -> stopped without going into running I think

jedevc · 2026-07-28T08:41:32Z

+	if extra := len(toRemove) - len(created); extra > 0 {
+		var baseName string
+		for key, field := range resource.IterFields(fields) {
+			if key.String() == "name" && field.Create != nil && field.Create.Set != nil {


resource.GetFieldByPath instead of iteration.

jedevc · 2026-07-28T08:43:17Z

 	Features     []string `group:"flag-create" shortcut:"features" help:"Instance features." placeholder:"feature"`
 	Template     string   `group:"flag-create" shortcut:"template" help:"Create from instance template." placeholder:"name"`
 	DeleteOnStop bool     `group:"flag-create" name:"rm" help:"Automatically delete the instance when it stops."`
+	Rollout      bool     `group:"flag-create" shortcut:"rollout" help:"Replace old instances with new ones matching image name"`


I wonder if we should rename this to be more specific --service-rollout. We rollout on a service.

The original intent/discussion was to move this to be generic and implementable for all resources

Not sure if we still want that?

jedevc · 2026-07-28T08:43:38Z

+	if rollout && req.Replicas != nil && *req.Replicas > 0 {
+		return nil, fmt.Errorf("--replicas cannot be used with --rollout")
+	}


It would make the math complicated and confusing? What would replicas mean in this context?

If I'm rolling ~6 instances in a service, and I specify 2 replicas, does it mean I want 2 instances in the end or 6*2==12?

We would need to be very clear in this case

jedevc · 2026-07-28T08:43:59Z

 	Delete(ctx context.Context, keys []string) error
 }

+type RolloutableResource interface {


In hindsight, maybe this abstraction isn't right. Apologies.

jedevc · 2026-07-28T09:02:14Z

A couple thoughts. There are two modes of rollout I think:

"Rolling" rollout. As implemented, create new instances, wait for healthy, delete old ones (or delete new ones if never healthy). Very nice, everyone is happy.
"Replace" rollout. Not implemented, but creates new instances, stops old instances, starts new ones, wait for healthy, delete old ones (or delete new ones, and restore old ones).
- Imagine a database. Each running database needs exclusive access to it's volume, so we should only actually be running one at a time. We'd still like to be able to do rollout, but we can't start any in parallel, so we should try and stop the old one and restart it later if the rollout fails.

Does that make sense? I don't really know what to call these "modes", but I think we should have both of them.

Also, as far as I can tell, the current implementation doesn't have the failure mode, where we restore back to the "known" good state. It assumes that every rollout is perfectly successful, which it may not be.

craciunoiuc · 2026-07-28T09:17:56Z

for rolling -- true, there is no rolling back, but you can't really do that if you're deleting instances while you're going 😅

for replace -- yes, we could do that, and I guess that should also allow for doing the health check after some time, and would only use the same amount of space in the service group.

I think we can do both, with rolling being the fast "destructive" one that just waits for instances to start, and replace being he "safe" one

With the small note that running a database in a service group is conceptually wrong so if we want to truly support database rollout we need to support non-servicegroup based rollout

jedevc · 2026-07-28T10:28:10Z

With the small note that running a database in a service group is conceptually wrong so if we want to truly support database rollout we need to support non-servicegroup based rollout

Hm, so how would you imagine database rollout working? I think service group still makes sense here - it's about the database service that you care about, but I'm curious why this is wrong.

for rolling -- true, there is no rolling back, but you can't really do that if you're deleting instances while you're going 😅

Indeed, that's why I was suggesting creating them all, and only deleting once you're convinced of the health of the others. Similar to how rolling deployments work in k8s.

I think we can do both, with rolling being the fast "destructive" one that just waits for instances to start, and replace being he "safe" one

I think the safety of them is the same no? replace results in downtime, but ensures that we never have more than n instances actively running, while rolling potentially allows up to 2n instances running, but never dipping below n.

I don't actually know actually if the names rolling/replace are right here. Maybe there's some better name here, or some better way of expressing the constraint (like max_running = n).

jedevc · 2026-07-28T10:35:11Z

Note, the platform doesn't really have a concept of instance health… this is sad.

IMO, we should currently think of healthy as:

Instance starts and enters a running state
We monitor the instance for "time"
We check that the instance is still running
Note instance as healthy

However, as we build the rollout flag, etc, we should consider that at some point, we would add a "real" healthcheck system (or add the ability to monitor health in new ways, like by checking logs, etc)

craciunoiuc requested review from Copilot and jedevc May 7, 2026 13:41

Copilot started reviewing on behalf of craciunoiuc May 7, 2026 13:42 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go Outdated

craciunoiuc force-pushed the craciunoiuc/add-rollout branch from 926d9db to ce661f7 Compare May 7, 2026 15:15

jedevc reviewed May 11, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

jedevc reviewed May 11, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

jedevc reviewed May 11, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

craciunoiuc force-pushed the craciunoiuc/add-rollout branch 2 times, most recently from 8ccf7d3 to c3adcf6 Compare May 12, 2026 15:44

craciunoiuc requested review from Copilot and jedevc May 12, 2026 15:45

Copilot started reviewing on behalf of craciunoiuc May 12, 2026 15:45 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread internal/resource/cmd/cmd.go

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go

craciunoiuc force-pushed the craciunoiuc/add-rollout branch 2 times, most recently from 143ed09 to 89cd18a Compare May 13, 2026 10:05

craciunoiuc requested a review from Copilot May 15, 2026 14:08

Copilot started reviewing on behalf of craciunoiuc May 15, 2026 14:08 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

craciunoiuc force-pushed the craciunoiuc/add-rollout branch 5 times, most recently from c68b276 to 144843c Compare May 18, 2026 19:02

jedevc reviewed Jun 1, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go Outdated

craciunoiuc force-pushed the craciunoiuc/add-rollout branch from 144843c to a2e506a Compare June 2, 2026 12:06

craciunoiuc requested a review from jedevc June 2, 2026 12:20

feat(cmd): Add '--rollout' flag when creating

d0260b3

This adds sequential rolling over services Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>

craciunoiuc force-pushed the craciunoiuc/add-rollout branch from a2e506a to d0260b3 Compare July 28, 2026 08:01

fixup! fix: Remove client-side wait and force autostart

9111e2e

Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>

jedevc reviewed Jul 28, 2026

View reviewed changes

craciunoiuc marked this pull request as draft July 28, 2026 12:15

Uh oh!

Conversation

craciunoiuc commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craciunoiuc commented May 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craciunoiuc commented May 19, 2026

Uh oh!

Uh oh!

Uh oh!

craciunoiuc commented Jun 2, 2026

Uh oh!

jedevc commented Jul 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedevc commented Jul 28, 2026

craciunoiuc commented May 7, 2026 •

edited

Loading

jedevc commented Jul 28, 2026 •

edited

Loading