feat(internal)!: Add timeouts to all supported endpoints

[jedevc]

Set timeouts in:

• start
• delete
• volimport
• instance template creation
  The rest already had timeouts.

Also remove some hacks while refactoring.

Depends on: https://github.com/unikraft-cloud/platform/pull/937

Closes: TOOL-1134

[craciunoiuc]

I think we need to keep the sleeps so I'll drop the commit (in kraft we had also a 1s sleep after the creation commands or so)

For the delete commit this was already done and merged so it can be dropped

Next I'm doing the loop retry for 5 minutes

[Copilot]

Pull request overview

This PR updates the Unikraft CLI’s instance lifecycle operations to use looping, bounded waits and explicit timeouts, aligning with newer platform API behavior (including the DestinationPort pointer change).

Changes:

• Update platform.Service.DestinationPort call sites to pass pointers.
• Add client-side looping waits for instance start/stop/delete (with per-call max wait) and introduce --wait duration flags for instance start/instance stop.
• Adjust instance create/autostart behavior and update tests/fixtures accordingly.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/volimport/volimport.go	Pass DestinationPort by pointer for volimport service group creation.
internal/cmd/services.go	Pass DestinationPort by pointer when creating/patching service groups.
internal/cmd/instances.go	Add wait loops for start/stop/delete, add --wait flags, set API timeouts.
go.mod	Bump unikraft.com/cloud/sdk dependency version.
go.sum	Update checksums for the bumped SDK version.
cmd/unikraft/testdata/TestGolden/instances/help	Remove wait-timeout field from help golden output.
cmd/unikraft/instances_test.go	Enable start/stop test and add a start/stop test that uses --wait.

Comments suppressed due to low confidence (2)

internal/cmd/instances.go:1649

• Same issue as waitForInstancesToRun: if WaitInstances errors and GetInstances fails, no entries are added to nextPending, so the function can return nil even though some instances never reached stopped. Return getErr (or keep refs pending) instead of silently ignoring it.

			getResp, getErr := c.GetInstances(ctx, refs.NameOrUUIDs(), platform.GetInstancesOpts{})
			if getErr == nil {
				var stillPending multimetro.Keys
				for _, inst := range getResp.Data.Instances {
					if inst.Status != nil && *inst.Status != platform.ResponseStatusSuccess {
						continue
					}
					switch inst.State {
					case platform.InstanceStateStopped:
						// Done, don't add to pending.
					case platform.InstanceStateStopping, platform.InstanceStateDraining:
						stillPending = append(stillPending, multimetro.Key{
							Metro: c.Metro.Name,
							Name:  inst.Name,
							UUID:  inst.Uuid,
						})
					default:
						return nil, fmt.Errorf("instance %q transitioned to unexpected state %q while waiting to stop", inst.Uuid, inst.State)
					}
				}
				mu.Lock()
				nextPending = append(nextPending, stillPending...)
				mu.Unlock()
			}

			return refs, nil
		})

cmd/unikraft/testdata/TestGolden/instances/help:64

• This golden help output was updated to remove wait-timeout, but it does not reflect the newly added --wait flag on instance start/instance stop (added in internal/cmd/instances.go). Regenerate/update this fixture so the start/stop help sections include the --wait flag and its default, otherwise the golden help test will drift/fail.

	  zero.stateful, scale-to-zero.cooldown-time, scale-to-zero.notify-time
	  timing, timing.uptime, timing.boot-time, timing.net-time
	  restart, restart.policy, restart.start-count, restart.restart-count
	  autostart
	  replicas
	  features
	  vsock
	  template
	  stop, stop.reason, stop.origin, stop.errno, stop.exit-code

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

internal/cmd/instances.go:1634

• Same issue as in waitForInstancesToRun: after a WaitInstances error, instances with non-success Status (or missing from the response) are skipped and never added back to nextPending, which can make the wait loop return nil even though some instances never reached the stopped state. Iterate by index with refs and handle non-success/missing responses explicitly (error or keep pending).

			var stillPending multimetro.Keys
			for _, inst := range getResp.Data.Instances {
				if inst.Status != nil && *inst.Status != platform.ResponseStatusSuccess {
					continue
				}

[craciunoiuc]

@jedevc I don't really have an answer to how to cleanly configure the wait time for delete and create

They are very abstract (and kinda weird to me)

In kraft what I did is to not wait for autostart on create as the create operation does not really make sense to be blocking. The fact that the instance starts automatically does not alter the idea that the command just creates an instance.

Similar for the delete operation. I also have my doubts with that. It's fine to wait for the max API time for it to go away, but the operation is anyway async, so why should we make it synchronous? But if we do want to do it, then why don't we do it for all API operations, theoretically we can.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

internal/cmd/instances.go:1635

• Same issue as wait-for-running: the GetInstances fallback iterates response items without ensuring a 1:1 correspondence with refs. Missing/non-returned instances can be dropped from pendingKeys, causing the wait loop to finish while some instances are still stopping. Consider validating response length and iterating by index to handle missing/non-success items deterministically.

			var stillPending multimetro.Keys
			for _, inst := range getResp.Data.Instances {
				if inst.Status != nil && *inst.Status != platform.ResponseStatusSuccess {
					return nil, fmt.Errorf("instance %q has non-success status while waiting to stop", inst.Uuid)
				}
				switch inst.State {

[jedevc]

It's useful to make it synchronous for scripting and similar. If you delete an instance, but don't wait, and then try and delete a volume that was mounted you can get "still mounted" as an error. We should wait, so that when it returns it's all guaranteed to be definitely gone and all it's resources disposed of.

[jedevc]

It would be useful to have some sort of option to skip this synchronizing tbf - but imo it does make sense to synchronize as a default.

[jedevc]

Maybe this is more complex though. And needs some sort of visual indicator while it's waiting to start but after the instance has been created.

[craciunoiuc]

Ah sorry, didn't think you'd get/answer to the notification 😓

The option is right now I believe to set the wait to 0, so all good (I still need to do some tweaks)

And needs some sort of visual indicator while it's waiting to start but after the instance has been created.

Agreed, maybe some spinning thingy like the one in kraft that did propagating but in this case waiting for this to start

[craciunoiuc]

The good part though is that 90% of cases finish in less than 1 second, and 99.9% of cases finish in less than 10s

Above that it's safe to say that te rest will take less than 20s

[jedevc]

Wondering if we should set drain_timeout_ms? Or force? Or whatever.

Not really in scope here, but maybe worth thinking about, since start is now updated too.

[craciunoiuc]

Hmm true, but I guess it would change functionality from default stop to:

• force stop if timeout is 0
• drain connections if timeout >0 or -1

This would change a bit how it works though. So maybe we leave force out of it for now and we only work around drain? Worth having a discussion on

[craciunoiuc]

rebased

main change is that it now applies to instance checkpoints (which do not have a fallback btw)

[craciunoiuc]

Will revisit after we decide if we should actually remove the hacks now

[craciunoiuc]

Killed comments, did a round which found some small quirk things, fixed CI

Should be good for a review but no rush

You might dislike still the things that do the retry in place, but at least now not many things will call them

[craciunoiuc]

I kept some comments that I thought to be informative

[craciunoiuc]

Note: the whole utils.go and utils_test.go and where they are used are there just for the hacky retry

Without it this change would be like ~400 lines I think

[craciunoiuc]

you can do a round now @jedevc

[craciunoiuc]

ready for another round @jedevc Addressed your comments

I moved the timeout functions to a separate shared package and made them generic enough

[jedevc]

Rebased it. LGTM!

Reviewed-by: Justin Chadwell <justin@unikraft.com>
Approved-by: Justin Chadwell <justin@unikraft.com>

[craciunoiuc]

end of an era :0