So this prioritize the queue but can cause that a multiple workers can be stuck on the pendingQueue.
Maybe we don't want to do it. Maybe simple stack that pops when a migration is finished would be sufficient.

Edit: queue is required in order to be able to cycle candidates

Having 2 queues is scary I agree, but we need a way to keep the main queue short, or we'll keep starving active migrations when we have a lot in pending.
So far, this is the only thing I could come up with that doesn't fundamentally change how we do controllers...
Keep in mind this is a draft, I opened it mostly to see how many tests would blow up (none it seems!), I have not yet committed to this approach.
I'm not sure what you mean by simple stack that pops when a migration is finished, is that describing current behavior or is it a new idea to fix the issue at hand?
Thanks!

I'm not sure what you mean by simple stack that pops when a migration is finished, is that describing current behavior or is it a new idea to fix the issue at hand?

Yes, I am thinking about a possible solution.
I think it comes down to, we need a way to sleep migrations and awake them once a migration is finished. WDYT?

If by "sleep" you mean "not re-enqueue them" then yes, anything that keeps them out of the queue will do.
We could keep that secondary queue and move 1 item from it to the main queue when a migration finishes, however that scares more than the current state of this PR...

Do we actually need (*queue) ?

Surprisingly yes, not sure why, thought go would deref for you as usual...

Just an alternative solution, feel free to choose:

func (c *MigrationController) flushPendingQueue(oldVMIM, newVMIM *virtv1.VirtualMachineInstanceMigration) {
	if oldVMIM.Status.Phase == newVMIM.Status.Phase || !newVMIM.IsFinal() {
		return
	}
	c.pendingQueueLock.Lock()
	for _, key := range c.pendingQueue {
		c.Queue.AddRateLimited(key)
	}
	c.pendingQueue = []string{}
	c.pendingQueueLock.Unlock()
}

Should we perform this after the UpdateStatus is performed?
I mean, if the update status fails there is the possibility that the migrations we have enqueued will be pushed back to the pending queue. No?

nit:

Expect(controller.migrationIndexer.Add(migration)).To(Succeed())

What I wrote matches what the other functions do.
We could change them all, but it might be a bit out of scope for this PR.
Also, I actually find the 2-line version to be more readable somehow...
I don't feel strongly about it though, let me know if you do!

No strong opinion. It was just a nit, maybe for the future. I'm fine with leaving like this

don't we need to lock before reading?

I don't think so. The only thing that touches pendingQueue is Execute(), which we run manually in our own thread.
There's no controller loop running and each test has its own controller.
The only concern could be about the informer, but it doesn't touch pendingQueue (and has no reason to trigger right now anyway).
Or am I missing something?

Yes, you are right. It was more of a theoretical comment than anything else :)

Just for confirmation: Can you explain this?

We return when it's false just above

Let's say that there are X running migrations (on the main queue) and Y pending migrations, and that Y is much bigger than X.

In this case IIUC we'd return to the state where we have a queue that's blown out with pending migrations. They will, in turn, be enqueued again to the pending queue, but perhaps we can limit the amount of migrations we flush into the main queue in a better way.

For example, how about we enqueue ParallelOutboundMigrationsPerNode, or even double than that, just to ensure we don't blow up the main queue again?

Yes, that was a compromise we made to avoid forgotten migrations... And it was still not enough, as you found!
Note that today we just keep re-enqueuing everything always, so this is still an improvement!
Maybe the added flush-pending-on-empty-queue will allow for partial flushes, I need to think about it!

Nice!
I wonder if it'd be valuable to create another test with multiple pending migrations that are expected to be flushed, just to cover this case as well (or maybe I'm being over-paranoid here?)

You're handing out the original slice here rather than a copy, so the lock is redundant, or you should switch to returning a copy.

In addition, maybe it's just me, but I find the function name a bit confusing, since queue usually means a queue object (like RateLimitingInterface) although here it's a string slice, but that's an implementation detail.

WDYT about

I almost made the same comment to Federico when he suggested this, but decided to test it in the Go playground, and the returned slice does appear to be a copy, unaffected by changes to the original slice.
Maybe slices are passed as "references" when calling a function, but copied on return?
Either way, as discussed, this function is only used in unit tests, where the lock is not necessary.

https://play.golang.com/p/wl2VYExfZSg
Are you sure? :)

Code:

type obj struct {
	s []string
}

func getObj() obj {
	return obj{
		s: []string{"one", "two"},
	}
}

func (o *obj) getSlice() []string {
	return o.s
}

func main() {
	// Initiate an object with a slice
	o := getObj()
	
	// Return the slice with copying it
	s := o.getSlice()
	
	// Change a slice element
	s[0] = "ten"
	
	// Print the object's internal slice
	fmt.Println(o.getSlice())
}

Output:

[ten two]