Skip to content

Commit 5cd046d

Browse files
committed
OCPBUGS-29469: fix Azure API SKU calls timing out - part 2
I erroneously assumed that Azure was not returning context errors but that was not actually the case. It was happening as a result of the way we structured the API call: ``` for page, err := client.List(ctx); page.NotDone(); err = page.NextWithContext(ctx) { if err != nil { return nil, fmt.Errorf("failed to list SKUs: %w", err) } } ``` If the `client.List` call fails (e.g, context deadline, auth error), `page.NotDone()` will return `false` and we'll never enter the `for` loop to check for `err`. Instead, we should make the `List` call and check for error *before* entering the loop.
1 parent 1c54fa1 commit 5cd046d

File tree

1 file changed

+26
-16
lines changed

1 file changed

+26
-16
lines changed

pkg/asset/installconfig/azure/client.go

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,14 @@ func (c *Client) GetDiskSkus(ctx context.Context, region string) ([]azsku.Resour
180180

181181
var sku []azsku.ResourceSku
182182

183-
for skuPage, err := client.List(ctx); skuPage.NotDone(); err = skuPage.NextWithContext(ctx) {
183+
// This has to be initialized outside the `for` because we need access to
184+
// `err`. If initialized in the loop and the API call fails right away,
185+
// `page.NotDone()` will return `false` and we'll never check for the error
186+
skuPage, err := client.List(ctx)
187+
if err != nil {
188+
return nil, fmt.Errorf("failed to list SKUs: %w", err)
189+
}
190+
for ; skuPage.NotDone(); err = skuPage.NextWithContext(ctx) {
184191
if err != nil {
185192
return nil, fmt.Errorf("error fetching SKU pages: %w", err)
186193
}
@@ -197,12 +204,6 @@ func (c *Client) GetDiskSkus(ctx context.Context, region string) ([]azsku.Resour
197204
return sku, nil
198205
}
199206

200-
// Azure does not return an error in case of context deadline, so we need
201-
// to check it ourselves
202-
if err := ctx.Err(); err != nil {
203-
return nil, fmt.Errorf("failed to list SKUs: %w", err)
204-
}
205-
206207
return nil, fmt.Errorf("no disks for specified subscription in region %s", region)
207208
}
208209

@@ -228,7 +229,11 @@ func (c *Client) ListResourceIDsByGroup(ctx context.Context, groupName string) (
228229
defer cancel()
229230

230231
var res []string
231-
for resPage, err := client.ListByResourceGroup(ctx, groupName, "", "", nil); resPage.NotDone(); err = resPage.NextWithContext(ctx) {
232+
resPage, err := client.ListByResourceGroup(ctx, groupName, "", "", nil)
233+
if err != nil {
234+
return nil, fmt.Errorf("failed to list resources: %w", err)
235+
}
236+
for ; resPage.NotDone(); err = resPage.NextWithContext(ctx) {
232237
if err != nil {
233238
return nil, fmt.Errorf("error fetching resource pages: %w", err)
234239
}
@@ -248,7 +253,14 @@ func (c *Client) GetVirtualMachineSku(ctx context.Context, name, region string)
248253
ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
249254
defer cancel()
250255

251-
for page, err := client.List(ctx); page.NotDone(); err = page.NextWithContext(ctx) {
256+
// This has to be initialized outside the `for` because we need access to
257+
// `err`. If initialized in the loop and the API call fails right away,
258+
// `page.NotDone()` will return `false` and we'll never check for the error
259+
page, err := client.List(ctx)
260+
if err != nil {
261+
return nil, fmt.Errorf("failed to list SKUs: %w", err)
262+
}
263+
for ; page.NotDone(); err = page.NextWithContext(ctx) {
252264
if err != nil {
253265
return nil, fmt.Errorf("error fetching SKU pages: %w", err)
254266
}
@@ -270,12 +282,6 @@ func (c *Client) GetVirtualMachineSku(ctx context.Context, name, region string)
270282
}
271283
}
272284

273-
// Azure does not return an error in case of context deadline, so we need
274-
// to check it ourselves
275-
if err := ctx.Err(); err != nil {
276-
return nil, fmt.Errorf("failed to list SKUs: %w", err)
277-
}
278-
279285
return nil, nil
280286
}
281287

@@ -392,7 +398,11 @@ func (c *Client) GetLocationInfo(ctx context.Context, region string, instanceTyp
392398

393399
// Only supported filter atm is `location`
394400
filter := fmt.Sprintf("location eq '%s'", region)
395-
for res, err := client.List(ctx, filter, "false"); res.NotDone(); err = res.NextWithContext(ctx) {
401+
res, err := client.List(ctx, filter, "false")
402+
if err != nil {
403+
return nil, fmt.Errorf("failed to list SKUs: %w", err)
404+
}
405+
for ; res.NotDone(); err = res.NextWithContext(ctx) {
396406
if err != nil {
397407
return nil, err
398408
}

0 commit comments

Comments
 (0)