This seems to be assuming that req.maxOldLogRouters is less than or equal to tlogs.size+1. Either that, or we're just ignoring maxOldLogRouters.

Can we make that an ASSERT if that's the case?

The code is already written so that it can recruit multiple log routers on the same worker, so the maxOldLogRouters was a suggested maximum number of workers, but returning fewer is okay

My concern was the opposite -- namely what if maxOldLogRouters is smaller than the number of logs. If such a thing were possible, this would result in there being more old log routers than the max.

looking at the code, the list of workers does not determine the number of log routers, so in that case it would just ignore the additional interfaces

So this logic means that we will always have 1 fewer old log router than tlogs (except when there's only 1)? Is there any reason not to use all of the tlogs when none of them are the cluster controller?

If what's written is the intent, the else block could be simplified to something like:

for(int i = 0; i < oldLogRouters.size() && result.oldRouters.size() < oldLogRouters.size() - 1; ++i) {
	if(oldLogRouters[i].interf.locality.processId() != clusterControllerProcessId) {
		result.oldLogRouters.push_back(oldLogRouters[i].interf);
	}
}

Since this loop is written a bunch of times, it might be a good candidate for extracting into a function.