fix: do not block drifted nodes from being terminated if consolidation is disabled

[saurav-agarwalla]

Fixes #N/A

Description

There was a regression introduced in #2180:

        // Filter out empty candidates. If there was an empty node that wasn't consolidated before this, we should
        // assume that it was due to budgets. If we don't filter out budgets, users who set a budget for `empty`
        // can find their nodes disrupted here, which while that in itself isn't an issue for empty nodes, it could
        // constrain the `drift` budget.
        if len(candidate.reschedulablePods) == 0 {
            continue
        }

This doesn't account for cases where consolidation is disabled (or for that matter has a really long consolidation period). Drift shouldn't be blocked on consolidation since they are independent disruption methods.

So, what happens is that emptiness doesn't disrupt the node since consolidation is disabled. But drift also doesn't terminate the node since the node is empty and it incorrectly assumes that emptiness would always disrupt it.

The original intent makes sense though which is why I am also making a change to prioritize non-empty nodes over empty nodes for drift. Updated this based on the discussion with @jmdeal. Drifting non-empty nodes first could mean that pods from those nodes just reschedule on the empty nodes and will need to move again when those nodes get disrupted. Hence, disrupting empty nodes first reduces the overall churn.

With this change, it is totally possible that nodes which aren't eligible to be consolidated at a moment in time get disrupted due to drift and consume a disruption budget but I think it is more important to still allow drift to happen so that we don't have stale nodes (either with outdated AMI or otherwise) even if they are empty because those empty nodes can still get pods scheduled on them which means that the user runs a risk of having their applications run on outdated AMIs.

This is also why I somehow feel that the original priority order of drift being the first disruption method makes sense since it should be more important for customers to be running a more up-to-date node than being more efficient with their workloads but as the original PR mentions, there are rough edges to that approach especially when customers drift nodes for non-critical reasons.

All of this could've been prevented in the first place if we actually had separate buckets for each disruption reason. Something that #2387 intends to solve.

How was this change tested?

make presubmit

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[coveralls]

Pull Request Test Coverage Report for Build 17133705499

Details

• 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
• 6 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 81.842%

---

Files with Coverage Reduction	New Missed Lines	%
pkg/controllers/node/termination/controller.go	2	77.78%
pkg/apis/v1/zz_generated.deepcopy.go	4	64.89%

Totals
Change from base Build 17132038986:	-0.02%
Covered Lines:	10488
Relevant Lines:	12815

---

💛 - Coveralls

[jmdeal]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmdeal, saurav-agarwalla

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jmdeal]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment