Skip to content

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

Open
craciunoiuc wants to merge 1 commit into
stagingfrom
craciunoiuc/add-rollout
Open

feat(cmd): Add '--rollout' flag when creating#309
craciunoiuc wants to merge 1 commit into
stagingfrom
craciunoiuc/add-rollout

Conversation

@craciunoiuc
Copy link
Copy Markdown
Member

@craciunoiuc craciunoiuc commented May 7, 2026

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

Closes: TOOL-794

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

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 craciunoiuc force-pushed the craciunoiuc/add-rollout branch from 926d9db to ce661f7 Compare May 7, 2026 15:15
Comment thread internal/cmd/instances.go Outdated
Comment thread internal/cmd/instances.go Outdated
Comment thread internal/cmd/instances.go Outdated
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/add-rollout branch 2 times, most recently from 8ccf7d3 to c3adcf6 Compare May 12, 2026 15:44
@craciunoiuc
Copy link
Copy Markdown
Member Author

I reworked the implementation as you asked @jedevc

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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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)
			}

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 craciunoiuc force-pushed the craciunoiuc/add-rollout branch 2 times, most recently from 143ed09 to 89cd18a Compare May 13, 2026 10:05
@craciunoiuc craciunoiuc requested a review from Copilot May 15, 2026 14:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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})

Comment thread internal/cmd/instances.go
Comment thread internal/cmd/instances.go Outdated
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 Outdated
Comment thread internal/resource/cmd/cmd.go Outdated
Comment thread internal/resource/cmd/cmd.go
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/add-rollout branch 4 times, most recently from 1d5b645 to c68b276 Compare May 18, 2026 18:04
This adds sequential rolling over services

Signed-off-by: Cezar Craciunoiu <cezar@unikraft.io>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/add-rollout branch from c68b276 to 144843c Compare May 18, 2026 19:02
@craciunoiuc
Copy link
Copy Markdown
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants