feat(cmd): Add rollout mechanics when creating

[craciunoiuc]

Open to UI/UX suggestions, right now it prints every new instance that was rolled
Completely reworked, now, as discussed below, added extra:

• Two modes, rolling/replace, one starting + deleting, one stopping + starting + deleting
• A configurable wait time to recheck that state is ok --rollout-healthy-after
• Made only for instances in services

Notable ickies still open to suggestions:

• Code needed to be added in the cmd resource to make sure that fields are accessed only once instead of twice
• Not quite so plug-and-play: I split things in a rollout package, but still a lot of helpers were needed in the instance.go itself

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]

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]

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

[craciunoiuc]

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)

[jedevc]

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

[jedevc]

resource.GetFieldByPath instead of iteration.

[jedevc]

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]

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]

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]

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]

Ok so I think the summary is:

• Two modes, rolling/replace doing what we said above
• A rollout healthcheck flag to optionally recheck health after X seconds
• Implemented just for instances with services for now, in the interest of time

[jedevc]

Yup. For the last one, eventually we could do things like support rollout over tags. Might be worth planning what the CLI interface looks like? So it's nice and consistent for when we do.

[Copilot]

🟡 Changes recommended

There are confirmed correctness issues in the new rollout field override helper and in create-time save/validation behavior when wrappers override resolved create patches.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The rollout “instances up” gate currently relies on a single immediate state read-back (and uses time.After in a cancelable wait), which can make rollouts flaky and incorrectly unwind.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite