CMP-4178: Add machineset scale up after remediations applied

[Anna-Koudelkova]

Based on a discussion we had with Xiaojie and Taimur, it is important to keep a check that machinesets are scalable after applying remediations and the deletePolicy: Newest would always delete the newly added machinesets first when asked to scale down.
This was previously a customer bug.

[vickeybrown]

/retest

[vickeybrown]

Should we use t.Errorf() for these error checks?

[Anna-Koudelkova]

That makes sense, thank you! I have originally used tFatalf() because the whole test cases uses it, but we probably do not need the test to fail immediately on failure in these added steps.

[taimurhafeez]

/retest

[taimurhafeez]

/retest

[openshift-ci]

@Anna-Koudelkova: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ocp4-cis	8740bce	link	true	/test e2e-aws-ocp4-cis
ci/prow/e2e-aws-rhcos4-moderate	8740bce	link	true	/test e2e-aws-rhcos4-moderate
ci/prow/e2e-aws-ocp4-stig	8740bce	link	true	/test e2e-aws-ocp4-stig

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[abushkin-redhat]

Hi Anna,

Looked over the PR and everything seems fine to me. There's one small bug that Claude picked up on which, while it doesn't break the testing, is a quick fix for correctness' sake (that avoids an unlikely, though possible edge case):

---

helpers/utilities.go:1877 — getMachineSetState reads status.readyReplicas to snapshot the original count, but this value is later used to set spec.replicas for scaling and cleanup restoration. These two fields can diverge — spec.replicas is the desired count while status.readyReplicas is the observed count. If a machine happens to
be provisioning or unhealthy when the test runs, the cleanup would permanently shrink the MachineSet to the lower observed count.

In practice this is unlikely since the validation runs after MCP rollout completes and all nodes should be Ready, but it's still reading the wrong field. Suggest changing line 1877 from reading status.readyReplicas to spec.replicas:

originalReplicas, foundReplicas, _ := unstructured.NestedInt64(msObj.Object, "spec", "replicas")
if !foundReplicas {
    originalReplicas = 1
}

...Defaulting to 1 rather than 0, since the MachineSet must have at least one machine for the test to have found a worker node.

---

That being said, I think this is a minor issue and shouldn't be blocking for the PR, so /lgtm

[abushkin-redhat]

/lgtm