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

[craciunoiuc]

Keeping in draft until box is fixed, but can be reviewed @jedevc

[jedevc]

@craciunoiuc can you revive this one plsss ❤️

It'll fix https://linear.app/unikraft/issue/TOOL-1134/vm-template-create-can-return-references-not-found.

[jedevc]

As discussed: if a call fails because of a timeout, just warn, and keep going.

[craciunoiuc]

Ok we have a problem here then

I revisited this and saw the comment

If we try to continue it means we'll have to rearch how our usage of commands works. For example: when a create endpoint call errors out with a timeout, it will not provide its instance information and it will just return the error message

This means that in order to "continue" we need to do a Get when the call times out and try to backfill data on the create response which feels ugly?

Anyway, have a rebase:

[jedevc]

Hmmmm, I thought a timeout would still return a partial success with some info. Does it not? If not, that does feel like a thing that the platform API should be updated to support.

[craciunoiuc]

ok, so there is a partial success, I'm picking again up to add this

[jedevc]

Be aware of #400. Which adds some partial success stuff.

[craciunoiuc]

draft till I know 100% it works

[craciunoiuc]

CI is being annoying, I'm investigating, but note that, stable will fail on that test till updated.

I will take stable/prod out of it until bumped there also

[jedevc]

Can this ever happen? This is weird, we created something, but Get can't see it??? Even if that's an async operation going on, that feels weird from the API surface point-of-view.

It makes for a lot of complexity, when really, IMO, once something is returned as "it exists", the API should be externally consistent.

If this code path is actually being exercised, I think we need to report this to the platform team.

[craciunoiuc]

Yes it can happen, I tested a case here: https://discord.com/channels/879723098881028167/1534550849299284129/1534897084212187290

A few examples:

• You create an instances with delete-on-stop and it crashes/finishes on boot (deleted before GET)
• You start an async operation that finishes after he timeout and GET
• You accidentally delete the instance you created before the GET happened (hard but doable)
• You used autokill and it got autokilled before the GET happened
  and so on

I think there are plenty of cases where this can happen, resulting in the original bug report TOOL-1134.

I would really like to keep this fix (or an alternative to it) in.

[jedevc]

URGH you're not wrong. Okay fine, do we just get the UUID/name in the Get or is it the full object? Like, could we always just return that?

If not, I guess there's not much we can do, and we need this ugly hack.

[craciunoiuc]

I think you meant in the Create not Get?

Yeah, you get the uuid/name but also some other info for some reason lol. I sent on discord but have here also:

metro:     dev-ccraciunoiu1
name:      nginx-57jo6
uuid:      98be47fc-b7d7-443b-aadb-4cafc6a84bb8
state:     starting
service:
  name:    muddy-grass-3jjl1whm
  uuid:    75f77131-753b-43a1-9646-5ea3684ec356
  domains:
  - fqdn:  muddy-grass-3jjl1whm.dev-ccraciunoiu1.apw.unikraft.internal

So also the service information and the state

[jedevc]

But no memory or vcpus 🤔 weird. Yeah, fine I guess we can have this fallback 😞

[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