Skip to content

Commit 3e55a01

Browse files
apricoteapricote
authored
refactor: unnecessary API call in instance reconciliation (#386)
Calls to `InstanceV2.InstanceMetadata()` caused 2 API calls, one to retrieve the server and the second to check if the configured network exists. The second call is not necessary because we already know all networks that the server is attached to exist. By just comparing the `nameOrID` locally we can achieve the same result. Related to #384
1 parent e86b990 commit 3e55a01

3 files changed

Lines changed: 28 additions & 52 deletions

File tree

‎hcloud/cloud.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ func newCloud(config io.Reader) (cloudprovider.Interface, error) {
131131
klog.Infof("%s: %s empty", op, hcloudNetworkENVVar)
132132
}
133133

134+
// Validate that the provided token works, and we have network connectivity to the Hetzner Cloud API
134135
_, _, err := client.Server.List(context.Background(), hcloud.ServerListOpts{})
135136
if err != nil {
136137
return nil, fmt.Errorf("%s: %w", op, err)
@@ -164,7 +165,7 @@ func newCloud(config io.Reader) (cloudprovider.Interface, error) {
164165

165166
return &cloud{
166167
client: client,
167-
instances: newInstances(client, instancesAddressFamily),
168+
instances: newInstances(client, instancesAddressFamily, networkID),
168169
loadBalancer: loadBalancers,
169170
routes: nil,
170171
networkID: networkID,

‎hcloud/instances.go‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package hcloud
1919
import (
2020
"context"
2121
"fmt"
22-
"os"
2322

2423
"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics"
2524
"github.com/hetznercloud/hcloud-go/hcloud"
@@ -38,10 +37,11 @@ const (
3837
type instances struct {
3938
client *hcloud.Client
4039
addressFamily addressFamily
40+
networkID int
4141
}
4242

43-
func newInstances(client *hcloud.Client, addressFamily addressFamily) *instances {
44-
return &instances{client, addressFamily}
43+
func newInstances(client *hcloud.Client, addressFamily addressFamily, networkID int) *instances {
44+
return &instances{client, addressFamily, networkID}
4545
}
4646

4747
// lookupServer attempts to locate the corresponding hcloud.Server for a given v1.Node
@@ -105,20 +105,20 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloud
105105
return &cloudprovider.InstanceMetadata{
106106
ProviderID: serverIDToProviderID(server.ID),
107107
InstanceType: server.ServerType.Name,
108-
NodeAddresses: i.nodeAddresses(ctx, server),
108+
NodeAddresses: nodeAddresses(i.addressFamily, i.networkID, server),
109109
Zone: server.Datacenter.Name,
110110
Region: server.Datacenter.Location.Name,
111111
}, nil
112112
}
113113

114-
func (i *instances) nodeAddresses(ctx context.Context, server *hcloud.Server) []v1.NodeAddress {
114+
func nodeAddresses(addressFamily addressFamily, networkID int, server *hcloud.Server) []v1.NodeAddress {
115115
var addresses []v1.NodeAddress
116116
addresses = append(
117117
addresses,
118118
v1.NodeAddress{Type: v1.NodeHostName, Address: server.Name},
119119
)
120120

121-
if i.addressFamily == AddressFamilyIPv4 || i.addressFamily == AddressFamilyDualStack {
121+
if addressFamily == AddressFamilyIPv4 || addressFamily == AddressFamilyDualStack {
122122
if !server.PublicNet.IPv4.IsUnspecified() {
123123
addresses = append(
124124
addresses,
@@ -127,7 +127,7 @@ func (i *instances) nodeAddresses(ctx context.Context, server *hcloud.Server) []
127127
}
128128
}
129129

130-
if i.addressFamily == AddressFamilyIPv6 || i.addressFamily == AddressFamilyDualStack {
130+
if addressFamily == AddressFamilyIPv6 || addressFamily == AddressFamilyDualStack {
131131
if !server.PublicNet.IPv6.IsUnspecified() {
132132
// For a given IPv6 network of 2001:db8:1234::/64, the instance address is 2001:db8:1234::1
133133
hostAddress := server.PublicNet.IPv6.IP
@@ -140,19 +140,15 @@ func (i *instances) nodeAddresses(ctx context.Context, server *hcloud.Server) []
140140
}
141141
}
142142

143-
n := os.Getenv(hcloudNetworkENVVar)
144-
if len(n) > 0 {
145-
network, _, _ := i.client.Network.Get(ctx, n)
146-
if network != nil {
147-
for _, privateNet := range server.PrivateNet {
148-
if privateNet.Network.ID == network.ID {
149-
addresses = append(
150-
addresses,
151-
v1.NodeAddress{Type: v1.NodeInternalIP, Address: privateNet.IP.String()},
152-
)
153-
}
143+
// Add private IP from network if network is specified
144+
if networkID > 0 {
145+
for _, privateNet := range server.PrivateNet {
146+
if privateNet.Network.ID == networkID {
147+
addresses = append(
148+
addresses,
149+
v1.NodeAddress{Type: v1.NodeInternalIP, Address: privateNet.IP.String()},
150+
)
154151
}
155-
156152
}
157153
}
158154
return addresses

‎hcloud/instances_test.go‎

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"encoding/json"
2222
"net"
2323
"net/http"
24-
"os"
2524
"reflect"
2625
"testing"
2726

@@ -57,7 +56,7 @@ func TestInstances_InstanceExists(t *testing.T) {
5756
json.NewEncoder(w).Encode(schema.ServerListResponse{Servers: servers})
5857
})
5958

60-
instances := newInstances(env.Client, AddressFamilyIPv4)
59+
instances := newInstances(env.Client, AddressFamilyIPv4, 0)
6160

6261
tests := []struct {
6362
name string
@@ -126,7 +125,7 @@ func TestInstances_InstanceShutdown(t *testing.T) {
126125
})
127126
})
128127

129-
instances := newInstances(env.Client, AddressFamilyIPv4)
128+
instances := newInstances(env.Client, AddressFamilyIPv4, 0)
130129

131130
tests := []struct {
132131
name string
@@ -183,7 +182,7 @@ func TestInstances_InstanceMetadata(t *testing.T) {
183182
})
184183
})
185184

186-
instances := newInstances(env.Client, AddressFamilyIPv4)
185+
instances := newInstances(env.Client, AddressFamilyIPv4, 0)
187186

188187
metadata, err := instances.InstanceMetadata(context.TODO(), &v1.Node{
189188
Spec: v1.NodeSpec{ProviderID: "hcloud://1"},
@@ -208,12 +207,12 @@ func TestInstances_InstanceMetadata(t *testing.T) {
208207
}
209208
}
210209

211-
func TestInstances_nodeAddresses(t *testing.T) {
210+
func TestNodeAddresses(t *testing.T) {
212211
tests := []struct {
213212
name string
214213
addressFamily addressFamily
215214
server *hcloud.Server
216-
privateNetwork string
215+
privateNetwork int
217216
expected []v1.NodeAddress
218217
}{
219218
{
@@ -318,7 +317,7 @@ func TestInstances_nodeAddresses(t *testing.T) {
318317
{
319318
name: "unknown private network",
320319
addressFamily: AddressFamilyIPv4,
321-
privateNetwork: "unknown-network",
320+
privateNetwork: 1,
322321
server: &hcloud.Server{
323322
Name: "foobar",
324323
},
@@ -329,7 +328,7 @@ func TestInstances_nodeAddresses(t *testing.T) {
329328
{
330329
name: "server attached to private network",
331330
addressFamily: AddressFamilyIPv4,
332-
privateNetwork: "test-existing-nw",
331+
privateNetwork: 1,
333332
server: &hcloud.Server{
334333
Name: "foobar",
335334
PrivateNet: []hcloud.ServerPrivateNet{
@@ -350,7 +349,7 @@ func TestInstances_nodeAddresses(t *testing.T) {
350349
{
351350
name: "server not attached to private network",
352351
addressFamily: AddressFamilyIPv4,
353-
privateNetwork: "test-existing-nw",
352+
privateNetwork: 1,
354353
server: &hcloud.Server{
355354
Name: "foobar",
356355
PrivateNet: []hcloud.ServerPrivateNet{
@@ -369,32 +368,12 @@ func TestInstances_nodeAddresses(t *testing.T) {
369368
},
370369
}
371370

372-
env := newTestEnv()
373-
defer env.Teardown()
374-
375-
env.Mux.HandleFunc("/networks", func(w http.ResponseWriter, r *http.Request) {
376-
var networks []schema.Network
377-
if r.URL.RawQuery == "name=test-existing-nw" {
378-
networks = append(networks, schema.Network{ID: 1, Name: "test-existing-nw"})
379-
}
380-
json.NewEncoder(w).Encode(schema.NetworkListResponse{Networks: networks})
381-
})
382-
383371
for _, test := range tests {
384372
t.Run(test.name, func(t *testing.T) {
385-
instances := newInstances(env.Client, test.addressFamily)
386-
387-
// nodeAddresses reads from environment variables at the moment
388-
if test.privateNetwork != "" {
389-
previousValue := os.Getenv(hcloudNetworkENVVar)
390-
os.Setenv(hcloudNetworkENVVar, test.privateNetwork)
391-
defer os.Setenv(hcloudNetworkENVVar, previousValue)
392-
}
393-
394-
nodeAddresses := instances.nodeAddresses(context.TODO(), test.server)
373+
addresses := nodeAddresses(test.addressFamily, test.privateNetwork, test.server)
395374

396-
if !reflect.DeepEqual(nodeAddresses, test.expected) {
397-
t.Fatalf("Expected nodeAddresses %+v but got %+v", test.expected, nodeAddresses)
375+
if !reflect.DeepEqual(addresses, test.expected) {
376+
t.Fatalf("Expected addresses %+v but got %+v", test.expected, addresses)
398377
}
399378
})
400379
}

0 commit comments

Comments
 (0)