since nil is a valid status, should we check and return if s == nil, in other functions as well?

In theory yes, but in practice it is not that necessary because we don't call this on nil or a concrect Success status. (If deem needed, it's even more necessary to guard AppendReason() than this)

ok, we can do that as a follow up.

shouldn't we also include Error?

I read Error more as an abnormal fault, which is usually not associated with the plugin's semantics, such as "node not found", "internal labelSelector deducing error", etc. So my original idea was that for type of Error, we don't set failedPlugin, so in runtime:

• if a Pod returns error on all nodes, its aggregated failedPlugin set is nil, so we move this pod upon any cluster event unconditionally
• if a Pod return non-error on some nodes, we intersect the associated plugin name upon a cluster event with those failedPlugins

This may prevent intermittent internal errors from disabling the pod's move. WDYT?

Just for the sake of separation of concerns, shouldn't the logic receiving the status decide how to deal with different codes?

Hmm, even we set failedPlugin upon Error, that info wouldn't be carried all the way to the logic which decides to move pods or not:

Upon an error in (Pre)Filter, diagnosis will be discarded:

	feasibleNodes, diagnosis, err := g.findNodesThatFitPod(ctx, fwk, state, pod)
	if err != nil {
		return result, err
	}

Moreover during Filter(), error would be immediately returned once we hit the first Error (other in-parallel FIlter() would cancel()). So even we carry the failedPlugin (on errro) all the way to Pod moving logic, this failedPlugin might be misleading.

But you have access to that when building the "diagnosis". In any case, my point is that "failed" != unschedulable. So we either include all failure modes, or name the field to indicate its limited scope.

In any case, my point is that "failed" != unschedulable.

That's a fair point. I think I will leave the failedPlugin in Status (more specifically, just Error, Unschedulable and UnschedulableAndUnresolvable, not including Success, Wait and Skip). And in diagnosis, I will rename failedPlugin to unschedulablePlugins.

changing the name to unschedulablePlugins sounds good to me.

leave a TODO as comment in the code too.

leave as comment in code