filter out inactive negs in isZoneChange

[midavadim]

1. Upgrade Starts: The E2E test triggers the cluster upgrade. The node in us-central1-a is cordoned and becomes temporarily NotReady.
2. Controller Restarts: The NEG controller pod is rescheduled onto the new, upgraded master.
3. The Race Condition: The new controller starts up and immediately runs a sync loop. Crucially, this happens in the small time window when the node is still NotReady.
4. Incorrect State Detection: The controller's syncInternal() logic queries for the current state of the cluster. Since the only node is NotReady, the zoneGetter reports that there are zero active nodes in us-central1-a.
5. NEG Marked INACTIVE: The controller compares the current state (zero active nodes) with the pre-upgrade state (one active NEG in us-central1-a). It concludes that the zone is no longer active and correctly, based on this limited information, patches the ServiceNetworkEndpointGroup CR to set the NEG's state to INACTIVE.
6. Node Becomes Ready: A moment later, the node finishes its upgrade and becomes Ready.
7. The Bug is Exposed: On the next sync, the controller should see that us-central1-a is active again and switch the NEG back to ACTIVE. This is where it fails. The isZoneChange() function, which is supposed to detect this, has a flaw. It checks the zones of all NEGs in the CR status, including INACTIVE ones.

• existingZones from the CR status is {"us-central1-a"} (from the INACTIVE NEG).
• currZones from the now-ready node is {"us-central1-a"}.
  The function sees no difference ({"us-central1-a"} == {"us-central1-a"}) and returns false.

8. Stuck State: Because isZoneChange() returns false, the controller doesn't trigger the reconciliation logic needed to switch the NEG back to ACTIVE. The test waits for an active NEG, which never happens, and times out.

Solution

This is why the proposed change to isZoneChange is so critical. By making it ignore INACTIVE NEGs, it can correctly detect the transition from "no active zones" to "one active zone," triggering the necessary reconciliation to bring the NEG back to an ACTIVE state and fix the load balancer.

[k8s-ci-robot]

Hi @midavadim. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[midavadim]

/assign @swetharepakula

[midavadim]

/hold

[midavadim]

@swetharepakula please check if provided update looks fine for you and can fix this problem. If yes I will proceed with testing

[swetharepakula]

/ok-to-test

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: midavadim
Once this PR has been reviewed and has the lgtm label, please ask for approval from swetharepakula. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[midavadim]

@swetharepakula

I have added unittest for this update.
So we can proceed with review.
Please let me know if you are fine with this update.

[midavadim]

/unhold

[swetharepakula]

Can we make all of the NEGs inactive instead of just 1?

This makes the what the flag does consistent. Otherwise, it is a little non-obvious that we are only affecting the first one to be inactive.

Or I would change negCRStateInactive to be instead be a list of zones where inactive negs exist. And we can make the refs considering that.