🐛 clusterctl: retry github i/o operations

[jackfrancis]

What this PR does / why we need it:

In response to a test flake, this PR introduces retry tolerance to GitHub API-dependent operations in clusterctl.

The passing (unchanged) UT should prove functional equivalence between this change and existing (no retry) behavior.

Reference test flake:

https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/periodic-cluster-api-provider-azure-e2e-workload-upgrade-1-19-1-20-main/1516508031641194496

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[jackfrancis]

cc @CecileRobertMichon

[fabriziopandini]

Given that GitHub is rate limited I suggest having a longer interval and shorter Timeout
Otherwise lgtm

[jackfrancis]

interval increased to every 10 seconds, timeout to 1 minute

[fabriziopandini]

/test pull-cluster-api-e2e-full-main

[jackfrancis]

/retest

[ykakarap]

If I understood this correctly, the intension of this PR (and the linked test flake) is to make github repository client resilient to network errors. In that case the approach in this PR may not work because wait.PollImmediate will return immediately (and will not retry) if the condition function returns an error. For example, this would mean if the client.Repositories.GetReleaseByTag call fails because of the network error and returns an error the client will return the error immediately and fail.

Since we are looking to basically retry when we hit network errors we might want to do something like this:

var err error
_ := wait.PollImmediate(retryableOperationInterval, retryableOperationTimeout, func() (bool, error) {
		var getReleasesErr error
		release, _, getReleasesErr = client.Repositories.GetReleaseByTag(context.TODO(), g.owner, g.repository, tag)
		if getReleasesErr != nil {
			err = getReleasesErr // Do this so we can capture the last error and return that to the layer above. We probably dont care about returning the poll timeout error.
                         // TODO: Here we should be check to see if err is ratelimting err and return immediately. No point in retrying if we are hitting a rate limitting error.
                        return false, nil
		}

		if release == nil {
			return false, nil
		}
		return true, nil
	})
if err != nil {
	return nil, err
}

[jackfrancis]

@ykakarap that's not my understanding of how PollImmediate works:

https://github.com/kubernetes/apimachinery/blob/v0.23.5/pkg/util/wait/wait.go#L501

The advantage of using PollImmediate over PullUntil is that the first operation is attempted straight away, while Poll waits for one interval (configurable) first of all before running.

[ykakarap]

@ykakarap that's not my understanding of how PollImmediate works:

https://github.com/kubernetes/apimachinery/blob/v0.23.5/pkg/util/wait/wait.go#L501

The advantage of using PollImmediate over PullUntil is that the first operation is attempted straight away, while Poll waits for one interval (configurable) first of all before running.

That is correct. PollImmediate will run the condition function immediately. I was talking about how we want to handle if the call to function client.Repositories.GetReleaseByTag returns an error because of a network flake. I assumed that was the intention of this PR. I might have misunderstood.

My understanding: The intention of this PR to to retry the github calls if it fails because of network flakes.

[jackfrancis]

@ykakarap you're correct, thank you!

https://go.dev/play/p/Z8ynq-10nhS

Hang on for a correct implementation

@fabriziopandini sorry, I'll submit a follow-up to #6431 with the correct implementation

[jackfrancis]

@ykakarap updated, PTAL

[ykakarap]

Overall LGTM for the approach pending the comment fix and the lint fixes.

[jackfrancis]

/test pull-cluster-api-e2e-full-main

[jackfrancis]

/retest

[ykakarap]

/lgtm

[jackfrancis]

/test pull-cluster-api-e2e-full-main

[sbueringer]

Looks good overall.

Given that it seems hard to add unit tests for this, did we do a bit of manual testing to verify we're hitting the cases correctly? (I'm aware we can't test all of them)

[jackfrancis]

@sbueringer these changes actually influence existing UT, so there is coverage there (this is why I manually override the timeout in UT so they don't take 1 min for each case :))

[ykakarap]

/lgtm

[sbueringer]

/lgtm

[fabriziopandini]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jackfrancis]

/cherry-pick release-1.1

[k8s-infra-cherrypick-robot]

@jackfrancis: #6430 failed to apply on top of branch "release-1.1":

Applying: clusterctl: retry github i/o operations
Using index info to reconstruct a base tree...
M	cmd/clusterctl/client/repository/repository_github.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/clusterctl/client/repository/repository_github.go
CONFLICT (content): Merge conflict in cmd/clusterctl/client/repository/repository_github.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 clusterctl: retry github i/o operations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.