Mark NEG refs from deleted subnet as to-be-deleted state.

[sawsa307]

/assign @gauravkghildiyal

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sawsa307
Once this PR has been reviewed and has the lgtm label, please ask for approval from gauravkghildiyal. For more information see the Kubernetes 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

[gauravkghildiyal]

Haven't reviewed the tests yet, but first want to understand the implementation and the expected behaviour.

[gauravkghildiyal]

Can we do a walk through of what happens when:

1. NEG already exists at a GKE version which has no knowledge of MSC and SvcNEG do not have subnet information.
2. GKE cluster is upgraded to a GKE version which has all of these changes.
3. So for the first time, how will the SvcNEG stats upgrades look like?

Do we have the code in place to update existing SvcNEGs with the subnet information such that it is available here for the first time?

[sawsa307]

What we are doing here is comparing the subnet of NEGs in NEG CR with the current set of subnets from nodeTopology CRD/subnetConfigs.

After upgrade, before customer adding any subnets:
The SvcNeg will only have NEG refs from the default subnet, and the nodeTopology CRD will only have the default subnet, so we won't find any NEGs that belongs to a subnet that are not in the current set of subnets.

The update of SvcNeg with subnet information is happening in updateInitStatus. We will do ensureNEG for each subnet and zone, and update the NEG CR with the returned NEG from ensure.

[gauravkghildiyal]

Thanks!

Still have some questions:

So for the first time after that upgrade, origNegRef.SubnetURL will be empty right? Which means we will run into this error case below because we'll fail to ParseResourceURL(). This seems to be a bit destructive, especially if someone also has some NEG transitioning to INACTIVE, which then we'll fail to mark and it will get lost. Is this right? Can we think of any other problems?

Is the answer to simply swap the order of checking Inactive followed by ToBeDeleted. Thinking about this, it seems logical to say we first classify things as Inactive, and out of those Inactive, we then further classify them as ToBeDeleted. If the current ordering was intended to cause some other issues, we may need to think of something else.