From 3758ab62d12acc346b9d33072683326eb4fad195 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Fri, 17 Apr 2020 15:52:47 +0200 Subject: [PATCH] Avoid unnecessary GCE API calls for IP-alias calls This is to avoid unnecessary GCE API calls done by getInstanceByName helper, which is iterating over all zones to find in which zone the VM exists. ProviderID already contains all the information - it's in the form: gce:// (VM URL contains project, zone, VM name). ProviderID is propagated by Kubelet on node registration and in case of bugs backfilled by node-controller. --- pkg/controller/nodeipam/ipam/adapter.go | 19 ++++++++++---- .../nodeipam/ipam/cloud_cidr_allocator.go | 5 +++- pkg/controller/nodeipam/ipam/sync/sync.go | 10 +++---- .../nodeipam/ipam/sync/sync_test.go | 8 +++--- .../gce/gce_instances.go | 26 +++++++++---------- 5 files changed, 40 insertions(+), 28 deletions(-) diff --git a/pkg/controller/nodeipam/ipam/adapter.go b/pkg/controller/nodeipam/ipam/adapter.go index 8bb13843f02..348feb21ab0 100644 --- a/pkg/controller/nodeipam/ipam/adapter.go +++ b/pkg/controller/nodeipam/ipam/adapter.go @@ -21,6 +21,7 @@ package ipam import ( "context" "encoding/json" + "fmt" "net" "k8s.io/klog" @@ -60,8 +61,12 @@ func newAdapter(k8s clientset.Interface, cloud *gce.Cloud) *adapter { return ret } -func (a *adapter) Alias(ctx context.Context, nodeName string) (*net.IPNet, error) { - cidrs, err := a.cloud.AliasRanges(types.NodeName(nodeName)) +func (a *adapter) Alias(ctx context.Context, node *v1.Node) (*net.IPNet, error) { + if node.Spec.ProviderID == "" { + return nil, fmt.Errorf("node %s doesn't have providerID", node.Name) + } + + cidrs, err := a.cloud.AliasRangesByProviderID(node.Spec.ProviderID) if err != nil { return nil, err } @@ -72,7 +77,7 @@ func (a *adapter) Alias(ctx context.Context, nodeName string) (*net.IPNet, error case 1: break default: - klog.Warningf("Node %q has more than one alias assigned (%v), defaulting to the first", nodeName, cidrs) + klog.Warningf("Node %q has more than one alias assigned (%v), defaulting to the first", node.Name, cidrs) } _, cidrRange, err := net.ParseCIDR(cidrs[0]) @@ -83,8 +88,12 @@ func (a *adapter) Alias(ctx context.Context, nodeName string) (*net.IPNet, error return cidrRange, nil } -func (a *adapter) AddAlias(ctx context.Context, nodeName string, cidrRange *net.IPNet) error { - return a.cloud.AddAliasToInstance(types.NodeName(nodeName), cidrRange) +func (a *adapter) AddAlias(ctx context.Context, node *v1.Node, cidrRange *net.IPNet) error { + if node.Spec.ProviderID == "" { + return fmt.Errorf("node %s doesn't have providerID", node.Name) + } + + return a.cloud.AddAliasToInstanceByProviderID(node.Spec.ProviderID, cidrRange) } func (a *adapter) Node(ctx context.Context, name string) (*v1.Node, error) { diff --git a/pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go b/pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go index 19adb41026c..63975dc76c1 100644 --- a/pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go +++ b/pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go @@ -249,8 +249,11 @@ func (ca *cloudCIDRAllocator) updateCIDRAllocation(nodeName string) error { klog.Errorf("Failed while getting node %v for updating Node.Spec.PodCIDR: %v", nodeName, err) return err } + if node.Spec.ProviderID == "" { + return fmt.Errorf("node %s doesn't have providerID", nodeName) + } - cidrs, err := ca.cloud.AliasRanges(types.NodeName(nodeName)) + cidrs, err := ca.cloud.AliasRangesByProviderID(node.Spec.ProviderID) if err != nil { nodeutil.RecordNodeStatusChange(ca.recorder, node, "CIDRNotAvailable") return fmt.Errorf("failed to allocate cidr: %v", err) diff --git a/pkg/controller/nodeipam/ipam/sync/sync.go b/pkg/controller/nodeipam/ipam/sync/sync.go index ee95392b8ff..733f76c592c 100644 --- a/pkg/controller/nodeipam/ipam/sync/sync.go +++ b/pkg/controller/nodeipam/ipam/sync/sync.go @@ -43,9 +43,9 @@ const ( // cloudAlias is the interface to the cloud platform APIs. type cloudAlias interface { // Alias returns the IP alias for the node. - Alias(ctx context.Context, nodeName string) (*net.IPNet, error) + Alias(ctx context.Context, node *v1.Node) (*net.IPNet, error) // AddAlias adds an alias to the node. - AddAlias(ctx context.Context, nodeName string, cidrRange *net.IPNet) error + AddAlias(ctx context.Context, node *v1.Node, cidrRange *net.IPNet) error } // kubeAPI is the interface to the Kubernetes APIs. @@ -204,7 +204,7 @@ func (op *updateOp) run(sync *NodeSync) error { op.node = node } - aliasRange, err := sync.cloudAlias.Alias(ctx, sync.nodeName) + aliasRange, err := sync.cloudAlias.Alias(ctx, op.node) if err != nil { klog.Errorf("Error getting cloud alias for node %q: %v", sync.nodeName, err) return err @@ -293,7 +293,7 @@ func (op *updateOp) updateAliasFromNode(ctx context.Context, sync *NodeSync, nod return err } - if err := sync.cloudAlias.AddAlias(ctx, node.Name, aliasRange); err != nil { + if err := sync.cloudAlias.AddAlias(ctx, node, aliasRange); err != nil { klog.Errorf("Could not add alias %v for node %q: %v", aliasRange, node.Name, err) return err } @@ -325,7 +325,7 @@ func (op *updateOp) allocateRange(ctx context.Context, sync *NodeSync, node *v1. // If addAlias returns a hard error, cidrRange will be leaked as there // is no durable record of the range. The missing space will be // recovered on the next restart of the controller. - if err := sync.cloudAlias.AddAlias(ctx, node.Name, cidrRange); err != nil { + if err := sync.cloudAlias.AddAlias(ctx, node, cidrRange); err != nil { klog.Errorf("Could not add alias %v for node %q: %v", cidrRange, node.Name, err) return err } diff --git a/pkg/controller/nodeipam/ipam/sync/sync_test.go b/pkg/controller/nodeipam/ipam/sync/sync_test.go index 8c80b2c6453..a10da6e2f45 100644 --- a/pkg/controller/nodeipam/ipam/sync/sync_test.go +++ b/pkg/controller/nodeipam/ipam/sync/sync_test.go @@ -58,13 +58,13 @@ type fakeAPIs struct { results []error } -func (f *fakeAPIs) Alias(ctx context.Context, nodeName string) (*net.IPNet, error) { - f.calls = append(f.calls, fmt.Sprintf("alias %v", nodeName)) +func (f *fakeAPIs) Alias(ctx context.Context, node *v1.Node) (*net.IPNet, error) { + f.calls = append(f.calls, fmt.Sprintf("alias %v", node.Name)) return f.aliasRange, f.aliasErr } -func (f *fakeAPIs) AddAlias(ctx context.Context, nodeName string, cidrRange *net.IPNet) error { - f.calls = append(f.calls, fmt.Sprintf("addAlias %v %v", nodeName, cidrRange)) +func (f *fakeAPIs) AddAlias(ctx context.Context, node *v1.Node, cidrRange *net.IPNet) error { + f.calls = append(f.calls, fmt.Sprintf("addAlias %v %v", node.Name, cidrRange)) return f.addAliasErr } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instances.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instances.go index f2e33f97bd1..71aa4c59e94 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instances.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instances.go @@ -361,21 +361,20 @@ func (g *Cloud) CurrentNodeName(ctx context.Context, hostname string) (types.Nod return types.NodeName(hostname), nil } -// AliasRanges returns a list of CIDR ranges that are assigned to the +// AliasRangesByProviderID returns a list of CIDR ranges that are assigned to the // `node` for allocation to pods. Returns a list of the form // "/". -func (g *Cloud) AliasRanges(nodeName types.NodeName) (cidrs []string, err error) { +func (g *Cloud) AliasRangesByProviderID(providerID string) (cidrs []string, err error) { ctx, cancel := cloud.ContextWithCallTimeout() defer cancel() - var instance *gceInstance - instance, err = g.getInstanceByName(mapNodeNameToInstanceName(nodeName)) + _, zone, name, err := splitProviderID(providerID) if err != nil { - return + return nil, err } var res *computebeta.Instance - res, err = g.c.BetaInstances().Get(ctx, meta.ZonalKey(instance.Name, lastComponent(instance.Zone))) + res, err = g.c.BetaInstances().Get(ctx, meta.ZonalKey(canonicalizeInstanceName(name), zone)) if err != nil { return } @@ -388,28 +387,29 @@ func (g *Cloud) AliasRanges(nodeName types.NodeName) (cidrs []string, err error) return } -// AddAliasToInstance adds an alias to the given instance from the named +// AddAliasToInstanceByProviderID adds an alias to the given instance from the named // secondary range. -func (g *Cloud) AddAliasToInstance(nodeName types.NodeName, alias *net.IPNet) error { +func (g *Cloud) AddAliasToInstanceByProviderID(providerID string, alias *net.IPNet) error { ctx, cancel := cloud.ContextWithCallTimeout() defer cancel() - v1instance, err := g.getInstanceByName(mapNodeNameToInstanceName(nodeName)) + _, zone, name, err := splitProviderID(providerID) if err != nil { return err } - instance, err := g.c.BetaInstances().Get(ctx, meta.ZonalKey(v1instance.Name, lastComponent(v1instance.Zone))) + + instance, err := g.c.BetaInstances().Get(ctx, meta.ZonalKey(canonicalizeInstanceName(name), zone)) if err != nil { return err } switch len(instance.NetworkInterfaces) { case 0: - return fmt.Errorf("instance %q has no network interfaces", nodeName) + return fmt.Errorf("instance %q has no network interfaces", providerID) case 1: default: klog.Warningf("Instance %q has more than one network interface, using only the first (%v)", - nodeName, instance.NetworkInterfaces) + providerID, instance.NetworkInterfaces) } iface := &computebeta.NetworkInterface{} @@ -420,7 +420,7 @@ func (g *Cloud) AddAliasToInstance(nodeName types.NodeName, alias *net.IPNet) er SubnetworkRangeName: g.secondaryRangeName, }) - mc := newInstancesMetricContext("add_alias", v1instance.Zone) + mc := newInstancesMetricContext("add_alias", zone) err = g.c.BetaInstances().UpdateNetworkInterface(ctx, meta.ZonalKey(instance.Name, lastComponent(instance.Zone)), iface.Name, iface) return mc.Observe(err) }