gce: dynamically refresh managed zones on resource cache miss and periodic sync (#1328)

Fixes dynamic zone discovery in external CCM mode where newly created
nodes register without topology zone labels, preventing node informer
event handlers from detecting unmanaged zones during initial bootstrap.

1. Trigger on-demand dynamic zone refresh on cache miss in getInstanceByName,
   getFoundInstanceByNames, and GetDiskByNameUnknownZone.
2. Support ProviderID fallback in getZone for uninitialized nodes.
3. Spawn a periodic background sync in Initialize to discover newly allowlisted
   zones proactively.
4. Add comprehensive unit tests covering uninitialized node metadata lookup,
   single/batch instance lookup, and disk lookup.

BUG=516475109
TAG=agy
CONV=b9619406-e590-4c1f-8747-80bf30fe7633

Co-authored-by: Arvind Bright <arvindbright@google.com>

Author: k8s-infra-cherrypick-robot

providers/gce/gce.go

@@ -45,6 +45,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/informers"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -781,6 +782,9 @@ func (g *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder,
 
 	go g.watchClusterID(stop)
 	go g.metricsCollector.Run(stop)
+	if g.dynamicZones {
+		go g.syncManagedZonesPeriodically(stop)
+	}
 }
 
 // LoadBalancer returns an implementation of LoadBalancer for Google Compute Engine.
@@ -1128,3 +1132,11 @@ func (g *Cloud) refreshManagedZones() error {
 
 	return nil
 }
+
+func (g *Cloud) syncManagedZonesPeriodically(stop <-chan struct{}) {
+	wait.Until(func() {
+		if err := g.refreshManagedZones(); err != nil {
+			klog.Errorf("Periodic refresh of GCE managed zones failed: %v", err)
+		}
+	}, 5*time.Minute, stop)
+}

providers/gce/gce_disks.go

@@ -929,33 +929,13 @@ func (g *Cloud) getRegionalDiskByName(diskName string) (*Disk, error) {
 	return disk, err
 }
 
-// GetDiskByNameUnknownZone scans all managed zones to return the GCE PD
-// Prefer getDiskByName, if the zone can be established
-// Return cloudprovider.DiskNotFound if the given disk cannot be found in any zone
-func (g *Cloud) GetDiskByNameUnknownZone(diskName string) (*Disk, error) {
-	regionalDisk, err := g.getRegionalDiskByName(diskName)
-	if err == nil {
-		return regionalDisk, err
-	}
-
-	// Note: this is the gotcha right now with GCE PD support:
-	// disk names are not unique per-region.
-	// (I can create two volumes with name "myvol" in e.g. us-central1-b & us-central1-f)
-	// For now, this is simply undefined behaviour.
-	//
-	// In future, we will have to require users to qualify their disk
-	// "us-central1-a/mydisk".  We could do this for them as part of
-	// admission control, but that might be a little weird (values changing
-	// on create)
-
+func (g *Cloud) findDiskInZones(diskName string, zones []string) (*Disk, error) {
 	var found *Disk
-	for _, zone := range g.getManagedZones() {
+	for _, zone := range zones {
 		disk, err := g.findDiskByName(diskName, zone)
 		if err != nil {
 			return nil, err
 		}
-		// findDiskByName returns (nil,nil) if the disk doesn't exist, so we can't
-		// assume that a disk was found unless disk is non-nil.
 		if disk == nil {
 			continue
 		}
@@ -974,9 +954,55 @@ func (g *Cloud) GetDiskByNameUnknownZone(diskName string) (*Disk, error) {
 		}
 		found = disk
 	}
+	return found, nil
+}
+
+// GetDiskByNameUnknownZone scans all managed zones to return the GCE PD
+// Prefer getDiskByName, if the zone can be established
+// Return cloudprovider.DiskNotFound if the given disk cannot be found in any zone
+func (g *Cloud) GetDiskByNameUnknownZone(diskName string) (*Disk, error) {
+	regionalDisk, err := g.getRegionalDiskByName(diskName)
+	if err == nil {
+		return regionalDisk, err
+	}
+
+	// Note: this is the gotcha right now with GCE PD support:
+	// disk names are not unique per-region.
+	// (I can create two volumes with name "myvol" in e.g. us-central1-b & us-central1-f)
+	// For now, this is simply undefined behaviour.
+	//
+	// In future, we will have to require users to qualify their disk
+	// "us-central1-a/mydisk".  We could do this for them as part of
+	// admission control, but that might be a little weird (values changing
+	// on create)
+
+	initialZones := g.getManagedZones()
+	found, err := g.findDiskInZones(diskName, initialZones)
+	if err != nil {
+		return nil, err
+	}
 	if found != nil {
 		return found, nil
 	}
+
+	if g.dynamicZones {
+		if err := g.refreshManagedZones(); err != nil {
+			klog.Errorf("Failed to refresh GCE managed zones for disk %s: %v", diskName, err)
+		} else {
+			refreshedZones := g.getManagedZones()
+			newZones := sets.NewString(refreshedZones...).Difference(sets.NewString(initialZones...)).List()
+			if len(newZones) > 0 {
+				found, err := g.findDiskInZones(diskName, newZones)
+				if err != nil {
+					return nil, err
+				}
+				if found != nil {
+					return found, nil
+				}
+			}
+		}
+	}
+
 	klog.Warningf("GCE persistent disk %q not found in managed zones (%s)",
 		diskName, strings.Join(g.getManagedZones(), ","))
 

providers/gce/gce_disks_test.go

@@ -21,18 +21,19 @@ package gce
 
 import (
 	"context"
-	"testing"
-
 	"fmt"
+	"net/http"
+	"testing"
 
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	computealpha "google.golang.org/api/compute/v0.alpha"
 	computebeta "google.golang.org/api/compute/v0.beta"
 	compute "google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
-	cloudprovider "k8s.io/cloud-provider"
 )
 
 // TODO TODO write a test for GetDiskByNameUnknownZone and make sure casting logic works
@@ -916,7 +917,7 @@ func (manager *FakeServiceManager) GetDiskFromCloudProvider(
 	zone string, diskName string) (*Disk, error) {
 
 	if manager.zonalDisks[zone] == "" {
-		return nil, cloudprovider.DiskNotFound
+		return nil, &googleapi.Error{Code: http.StatusNotFound}
 	}
 
 	if manager.resourceInUse {
@@ -941,7 +942,7 @@ func (manager *FakeServiceManager) GetRegionalDiskFromCloudProvider(
 	diskName string) (*Disk, error) {
 
 	if _, ok := manager.regionalDisks[diskName]; !ok {
-		return nil, cloudprovider.DiskNotFound
+		return nil, &googleapi.Error{Code: http.StatusNotFound}
 	}
 
 	if manager.resourceInUse {
@@ -1019,3 +1020,42 @@ func createNodeZones(zones []string) map[string]sets.String {
 	}
 	return nodeZones
 }
+
+func TestGetDiskByNameUnknownZone_DynamicRefresh(t *testing.T) {
+	gceProjectID := "test-project"
+	gceRegion := "us-central1"
+	fakeManager := newFakeManager(gceProjectID, gceRegion)
+	fakeManager.zonalDisks["us-central1-c"] = "my-disk"
+
+	gce := Cloud{
+		manager:      fakeManager,
+		managedZones: []string{"us-central1-b"},
+		projectID:    gceProjectID,
+		region:       gceRegion,
+		dynamicZones: true,
+	}
+
+	mockGCE := cloud.NewMockGCE(&gceProjectRouter{&gce})
+	keyC := meta.GlobalKey("key-c")
+	mockGCE.MockZones.Objects[*keyC] = &cloud.MockZonesObj{
+		Obj: &compute.Zone{Name: "us-central1-c", Region: gce.getRegionLink("us-central1")},
+	}
+	keyB := meta.GlobalKey("key-b")
+	mockGCE.MockZones.Objects[*keyB] = &cloud.MockZonesObj{
+		Obj: &compute.Zone{Name: "us-central1-b", Region: gce.getRegionLink("us-central1")},
+	}
+	gce.c = mockGCE
+
+	disk, err := gce.GetDiskByNameUnknownZone("my-disk")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if disk == nil || disk.Name != "my-disk" {
+		t.Fatalf("expected disk my-disk, got %v", disk)
+	}
+
+	expectedZones := []string{"us-central1-b", "us-central1-c"}
+	if !sets.NewString(gce.getManagedZones()...).Equal(sets.NewString(expectedZones...)) {
+		t.Fatalf("expected managed zones %v, got %v", expectedZones, gce.getManagedZones())
+	}
+}

providers/gce/gce_instances.go

@@ -72,11 +72,17 @@ func getZone(node *v1.Node) string {
 		return zone
 	}
 	zone, ok = node.Labels[v1.LabelFailureDomainBetaZone]
-	if !ok {
-		klog.Warningf("Node without zone label, returning %q as zone. Node name: %v, node labels: %v", emptyZone, node.Name, node.Labels)
-		return emptyZone
+	if ok {
+		return zone
 	}
-	return zone
+	if node.Spec.ProviderID != "" {
+		_, zone, _, err := splitProviderID(node.Spec.ProviderID)
+		if err == nil && zone != "" {
+			return zone
+		}
+	}
+	klog.Warningf("Node without zone label or providerID, returning %q as zone. Node name: %v, node labels: %v", emptyZone, node.Name, node.Labels)
+	return emptyZone
 }
 
 func makeHostURL(projectsAPIEndpoint, projectID, zone, host string) string {
@@ -715,7 +721,8 @@ func (g *Cloud) getFoundInstanceByNames(names []string) ([]*gceInstance, error)
 		found[name] = nil
 	}
 
-	for _, zone := range g.getManagedZones() {
+	initialZones := g.getManagedZones()
+	for _, zone := range initialZones {
 		if remaining == 0 {
 			break
 		}
@@ -745,6 +752,47 @@ func (g *Cloud) getFoundInstanceByNames(names []string) ([]*gceInstance, error)
 		}
 	}
 
+	if remaining > 0 && g.dynamicZones {
+		if err := g.refreshManagedZones(); err != nil {
+			klog.Errorf("Failed to refresh GCE managed zones for remaining instances: %v", err)
+		} else {
+			refreshedZones := g.getManagedZones()
+			checkedZones := sets.NewString(initialZones...)
+			for _, zone := range refreshedZones {
+				if remaining == 0 {
+					break
+				}
+				if checkedZones.Has(zone) {
+					continue
+				}
+				instances, err := g.c.Instances().List(ctx, zone, filter.Regexp("name", nodeInstancePrefix+".*"))
+				if err != nil {
+					return nil, err
+				}
+				for _, inst := range instances {
+					if remaining == 0 {
+						break
+					}
+					if _, ok := found[inst.Name]; !ok {
+						continue
+					}
+					if found[inst.Name] != nil {
+						klog.Errorf("Instance name %q was duplicated (in zone %q and %q)", inst.Name, zone, found[inst.Name].Zone)
+						continue
+					}
+					found[inst.Name] = &gceInstance{
+						Zone:  zone,
+						Name:  inst.Name,
+						ID:    inst.Id,
+						Disks: inst.Disks,
+						Type:  lastComponent(inst.MachineType),
+					}
+					remaining--
+				}
+			}
+		}
+	}
+
 	var ret []*gceInstance
 	var failed []string
 	for name, instance := range found {
@@ -761,12 +809,8 @@ func (g *Cloud) getFoundInstanceByNames(names []string) ([]*gceInstance, error)
 	return ret, nil
 }
 
-// Gets the named instance, returning cloudprovider.InstanceNotFound if the instance is not found
-func (g *Cloud) getInstanceByName(name string) (*gceInstance, error) {
-	klog.Infof("Searching node %s in managed zones %v", name, g.getManagedZones())
-
-	// Avoid changing behaviour when not managing multiple zones
-	for _, zone := range g.getManagedZones() {
+func (g *Cloud) findInstanceInZones(name string, zones []string) (*gceInstance, error) {
+	for _, zone := range zones {
 		instance, err := g.getInstanceFromProjectInZoneByName(g.projectID, zone, name)
 		if err != nil {
 			if isHTTPErrorCode(err, http.StatusNotFound) {
@@ -777,6 +821,33 @@ func (g *Cloud) getInstanceByName(name string) (*gceInstance, error) {
 		}
 		return instance, nil
 	}
+	return nil, nil
+}
+
+// Gets the named instance, returning cloudprovider.InstanceNotFound if the instance is not found
+func (g *Cloud) getInstanceByName(name string) (*gceInstance, error) {
+	initialZones := g.getManagedZones()
+	klog.Infof("Searching node %s in managed zones %v", name, initialZones)
+
+	if instance, err := g.findInstanceInZones(name, initialZones); err != nil || instance != nil {
+		return instance, err
+	}
+
+	if g.dynamicZones {
+		klog.Infof("Node %s not found in managed zones %v; attempting dynamic zone refresh", name, initialZones)
+		if err := g.refreshManagedZones(); err != nil {
+			klog.Errorf("Failed to refresh GCE managed zones: %v", err)
+		} else {
+			refreshedZones := g.getManagedZones()
+			newZones := sets.NewString(refreshedZones...).Difference(sets.NewString(initialZones...)).List()
+			if len(newZones) > 0 {
+				klog.Infof("Searching node %s in newly discovered zones %v", name, newZones)
+				if instance, err := g.findInstanceInZones(name, newZones); err != nil || instance != nil {
+					return instance, err
+				}
+			}
+		}
+	}
 
 	return nil, cloudprovider.InstanceNotFound
 }