✨ Reduce github api requests in clusterctl by querying go modules #7192

chrischdi · 2022-09-08T11:45:02Z

What this PR does / why we need it:

Reduce github api requests in clusterctl

Removes additional cert-manager latest version detection because it always gets overwritten afterwards anyway.
Uses goproxy instead of github api for listing repository versions.

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

cmd/clusterctl/client/repository/repository_github.go

cmd/clusterctl/client/cluster/cert_manager.go

sbueringer · 2022-09-09T13:17:12Z

cc @ykakarap

killianmuldoon

Looks awesome - do we have any idea how much this helps? Is there many fewer calls in a clusterctl init for example?

ykakarap

Just took a high-level look at this and this looks great.

I think technically a provider can be written in any language and not necessarily be in golang. This would enfore that the provider be a go project for clusterctl to be able to use the github repo to perform any actions.
I dont know if there are any providers that are actually not written in go (I dont imagine there are) but I wanted to call out the dependency this will establish.

How about adding a fallback to list the versions using the old github method? This would also cover the case if the provider's go module name does not match the github owner name (like "kubernetes-sigs" -> "sigs.k8s.io" and "kubernetes" -> "k8s.io").

chrischdi · 2022-09-16T07:54:17Z

Just took a high-level look at this and this looks great.

I think technically a provider can be written in any language and not necessarily be in golang. This would enfore that the provider be a go project for clusterctl to be able to use the github repo to perform any actions. I dont know if there are any providers that are actually not written in go (I dont imagine there are) but I wanted to call out the dependency this will establish.

How about adding a fallback to list the versions using the old github method? This would also cover the case if the provider's go module name does not match the github owner name (like "kubernetes-sigs" -> "sigs.k8s.io" and "kubernetes" -> "k8s.io").

I agree with that, looks even more to 👍 for the fallback to the old method as in #7192 (comment) :-)

chrischdi · 2022-09-16T08:45:56Z

Looks awesome - do we have any idea how much this helps? Is there many fewer calls in a clusterctl init for example?

Yes:

The cert-manager part removes 1 request to detect the latest version, which was always made without any outcome because we pin the version anyway.
The list releases part: this replaces 1 request per provider by using the goproxy (if no fallback is done)

I analyzed it by adding print statements in the code.

Prior to this change for installing the AWS provider:

❯ AWS_B64ENCODED_CREDENTIALS=$(echo foo | base64) go run ./cmd/clusterctl init --infrastructure aws
Fetching providers
>> client.Repositories.ListReleases g.owner kubernetes-sigs g.repository cluster-api
>> client.Repositories.GetReleaseByTag g.owner kubernetes-sigs g.repository cluster-api tag v1.2.2
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826385
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826397
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826387
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826389
>> client.Repositories.ListReleases g.owner kubernetes-sigs g.repository cluster-api-provider-aws
>> client.Repositories.GetReleaseByTag g.owner kubernetes-sigs g.repository cluster-api-provider-aws tag v1.5.0
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api-provider-aws *assetID 74081055
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api-provider-aws *assetID 74081043
Installing cert-manager Version="v1.9.1"
>> client.Repositories.ListReleases g.owner cert-manager g.repository cert-manager
>> client.Repositories.GetReleaseByTag g.owner cert-manager g.repository cert-manager tag v1.9.1
>> client.Repositories.DownloadReleaseAsset g.owner cert-manager g.repository cert-manager *assetID 72738262
Waiting for cert-manager to be available...
Installing Provider="cluster-api" Version="v1.2.2" TargetNamespace="capi-system"
Installing Provider="bootstrap-kubeadm" Version="v1.2.2" TargetNamespace="capi-kubeadm-bootstrap-system"
Installing Provider="control-plane-kubeadm" Version="v1.2.2" TargetNamespace="capi-kubeadm-control-plane-system"
Installing Provider="infrastructure-aws" Version="v1.5.0" TargetNamespace="capa-system"

3 calls to client.Repositories.ListReleases
- 1 for each provider (core/aws): gets replaced by goproxy request
- 1 for cert-manager: gets removed because its not required
3 calls to client.Repositories.GetReleaseByTag
- 1 for each provider (core / aws)
- 1 for cert-manager
7 calls to client.Repositories.DownloadReleaseAsset
- 4 for core provider currently
- 2 for aws provider
- 1 for cert-manager

In sum: 13 API Calls to github.

With this PR (if the providers are go based and supported via goproxy) we reduce the amount of API calls by 3 (to 10) in this example:

❯ AWS_B64ENCODED_CREDENTIALS=$(echo foo | base64) go run ./cmd/clusterctl init --infrastructure aws
Fetching providers
>> !!goproxy!! versionClient.List g.owner kubernetes-sigs g.repository cluster-api
>> client.Repositories.GetReleaseByTag g.owner kubernetes-sigs g.repository cluster-api tag v1.2.2
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826385
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826397
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826387
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api *assetID 77826389
>> !!goproxy!! versionClient.List g.owner kubernetes-sigs g.repository cluster-api-provider-aws
>> client.Repositories.GetReleaseByTag g.owner kubernetes-sigs g.repository cluster-api-provider-aws tag v1.5.0
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api-provider-aws *assetID 74081055
>> client.Repositories.DownloadReleaseAsset g.owner kubernetes-sigs g.repository cluster-api-provider-aws *assetID 74081043
Installing cert-manager Version="v1.9.1"
>> client.Repositories.GetReleaseByTag g.owner cert-manager g.repository cert-manager tag v1.9.1
>> client.Repositories.DownloadReleaseAsset g.owner cert-manager g.repository cert-manager *assetID 72738262
Waiting for cert-manager to be available...
Installing Provider="cluster-api" Version="v1.2.2" TargetNamespace="capi-system"
Installing Provider="bootstrap-kubeadm" Version="v1.2.2" TargetNamespace="capi-kubeadm-bootstrap-system"
Installing Provider="control-plane-kubeadm" Version="v1.2.2" TargetNamespace="capi-kubeadm-control-plane-system"
Installing Provider="infrastructure-aws" Version="v1.5.0" TargetNamespace="capa-system"

I will go ahead and implement the fallback mechanism.

chrischdi · 2022-09-16T14:17:14Z

Refactored the code and moved the caching + goproxy call up by one func so the fallback mechanism seems to be cleaner.

chrischdi · 2022-09-19T09:11:28Z

/test help

k8s-ci-robot · 2022-09-19T09:11:30Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-ipv6-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-test-mink8s-main
pull-cluster-api-verify-main

In response to this:

/test help

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.

chrischdi · 2022-09-19T09:11:54Z

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

chrischdi · 2022-09-19T12:18:17Z

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

fabriziopandini

This looks great!
I will handle GOPROXY=off or directly by silently falling back to GitHub API; also, I will document we are relying on goproxy for reducing API calls + we are respecting GOPROXY env var in https://cluster-api.sigs.k8s.io/clusterctl/overview.html?highlight=github#avoiding-github-rate-limiting and reference the same paragraph from https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html?highlight=github#creating-a-provider-repository-on-github.

cmd/clusterctl/client/cluster/cert_manager.go

cmd/clusterctl/client/repository/goproxy.go

cmd/clusterctl/client/repository/repository_github.go

fabriziopandini · 2022-09-22T15:52:49Z

/retitle ✨ Reduce github api requests in clusterctl by querying go modules

cmd/clusterctl/client/repository/goproxy.go

cmd/clusterctl/client/repository/repository_github.go

chrischdi · 2022-09-23T12:08:06Z

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

chrischdi · 2022-09-23T12:17:52Z

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

cmd/clusterctl/client/repository/goproxy.go

chrischdi · 2022-11-09T07:45:25Z

@fabriziopandini / @sbueringer : friendly reminder, this one is still around :-) It may be a nice small improvement, although it may already be too late for v1.3.

sbueringer · 2022-11-15T04:24:09Z

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-25-latest-main

sbueringer

A few nits

cmd/clusterctl/client/cluster/cert_manager.go

cmd/clusterctl/client/repository/goproxy.go

fabriziopandini · 2022-11-15T10:27:56Z

Only one nit from my side.
Can we document in https://cluster-api.sigs.k8s.io/clusterctl/commands/init.html#provider-repositories that clusterctl uses go proxy to detect available versions without calling GitHub API and that users can control/disable this behavior by setting the go proxy variable?

A similar note should go into https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#creating-a-provider-repository-on-github and probably in https://cluster-api.sigs.k8s.io/clusterctl/overview.html#avoiding-github-rate-limiting

* Removes additional cert-manager latest version detection because it always gets overwritten. * Uses goproxy instead of github api for listing repository versions.

k8s-ci-robot · 2022-11-15T11:25:03Z

@chrischdi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`f7db0c7`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sbueringer · 2022-11-15T11:29:04Z

Thx!!

/lgtm

sbueringer · 2022-11-15T11:29:10Z

/assign @fabriziopandini

fabriziopandini · 2022-11-15T12:39:52Z

Great work!
/lgtm
/approve

k8s-ci-robot · 2022-11-15T12:40:00Z

[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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 8, 2022

k8s-ci-robot requested review from sbueringer and ykakarap September 8, 2022 11:45

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2022

chrischdi commented Sep 8, 2022

View reviewed changes

cmd/clusterctl/client/repository/repository_github.go Outdated Show resolved Hide resolved

chrischdi force-pushed the pr-optimize-gh-requests branch 3 times, most recently from 5fd4b37 to cc65b30 Compare September 9, 2022 08:36

chrischdi commented Sep 9, 2022

View reviewed changes

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

chrischdi changed the title ~~🌱 [WIP] Reduce github api requests in clusterctl~~ 🌱 Reduce github api requests in clusterctl Sep 9, 2022

killianmuldoon reviewed Sep 15, 2022

View reviewed changes

ykakarap reviewed Sep 16, 2022

View reviewed changes

fabriziopandini reviewed Sep 22, 2022

View reviewed changes

k8s-ci-robot changed the title ~~🌱 Reduce github api requests in clusterctl~~ ✨ Reduce github api requests in clusterctl by querying go modules Sep 22, 2022

ykakarap reviewed Sep 23, 2022

View reviewed changes

chrischdi force-pushed the pr-optimize-gh-requests branch 2 times, most recently from 0101c0b to c88740e Compare September 23, 2022 12:06

chrischdi force-pushed the pr-optimize-gh-requests branch 2 times, most recently from 0210a05 to 982bb18 Compare September 23, 2022 12:15

chrischdi force-pushed the pr-optimize-gh-requests branch from 982bb18 to 87bcb54 Compare September 26, 2022 06:18

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2022

vincepri reviewed Sep 26, 2022

View reviewed changes

cmd/clusterctl/client/repository/goproxy.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/repository/goproxy.go Outdated Show resolved Hide resolved

chrischdi force-pushed the pr-optimize-gh-requests branch from 87bcb54 to ea2e4e2 Compare September 27, 2022 06:02

sbueringer reviewed Nov 15, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2022

chrischdi force-pushed the pr-optimize-gh-requests branch from 1fe0b50 to f54300e Compare November 15, 2022 08:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2022

chrischdi force-pushed the pr-optimize-gh-requests branch from f54300e to 7c1df3a Compare November 15, 2022 08:26

sbueringer reviewed Nov 15, 2022

View reviewed changes

cmd/clusterctl/client/repository/goproxy.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/repository/goproxy.go Outdated Show resolved Hide resolved

Reduce github api requests in clusterctl

f7db0c7

* Removes additional cert-manager latest version detection because it always gets overwritten. * Uses goproxy instead of github api for listing repository versions.

chrischdi force-pushed the pr-optimize-gh-requests branch from 7c1df3a to f7db0c7 Compare November 15, 2022 11:22

k8s-ci-robot assigned sbueringer Nov 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2022

k8s-ci-robot assigned fabriziopandini Nov 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2022

k8s-ci-robot merged commit 5ff76fb into kubernetes-sigs:main Nov 15, 2022

k8s-ci-robot added this to the v1.3 milestone Nov 15, 2022

This was referenced Dec 7, 2022

goproxyClient doesn't list version >=v2 #7700

Closed

[release-1.2] 🐛 releaselink: fix goproxy to also return versions for major > 1 #7729

Merged

killianmuldoon mentioned this pull request Jul 6, 2023

clusterctl upgrades fail if a component is branched but not released #8966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Reduce github api requests in clusterctl by querying go modules #7192

✨ Reduce github api requests in clusterctl by querying go modules #7192

chrischdi commented Sep 8, 2022

sbueringer commented Sep 9, 2022

killianmuldoon left a comment

ykakarap left a comment

chrischdi commented Sep 16, 2022

chrischdi commented Sep 16, 2022 •

edited

Loading

chrischdi commented Sep 16, 2022

chrischdi commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

chrischdi commented Sep 19, 2022

chrischdi commented Sep 19, 2022

fabriziopandini left a comment

fabriziopandini commented Sep 22, 2022

chrischdi commented Sep 23, 2022

chrischdi commented Sep 23, 2022

chrischdi commented Nov 9, 2022

sbueringer commented Nov 15, 2022

sbueringer left a comment

fabriziopandini commented Nov 15, 2022

k8s-ci-robot commented Nov 15, 2022

sbueringer commented Nov 15, 2022

sbueringer commented Nov 15, 2022

fabriziopandini commented Nov 15, 2022

k8s-ci-robot commented Nov 15, 2022

✨ Reduce github api requests in clusterctl by querying go modules #7192

✨ Reduce github api requests in clusterctl by querying go modules #7192

Conversation

chrischdi commented Sep 8, 2022

sbueringer commented Sep 9, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

ykakarap left a comment

Choose a reason for hiding this comment

chrischdi commented Sep 16, 2022

chrischdi commented Sep 16, 2022 • edited Loading

chrischdi commented Sep 16, 2022

chrischdi commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

chrischdi commented Sep 19, 2022

chrischdi commented Sep 19, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini commented Sep 22, 2022

chrischdi commented Sep 23, 2022

chrischdi commented Sep 23, 2022

chrischdi commented Nov 9, 2022

sbueringer commented Nov 15, 2022

sbueringer left a comment

Choose a reason for hiding this comment

fabriziopandini commented Nov 15, 2022

k8s-ci-robot commented Nov 15, 2022

sbueringer commented Nov 15, 2022

sbueringer commented Nov 15, 2022

fabriziopandini commented Nov 15, 2022

k8s-ci-robot commented Nov 15, 2022

chrischdi commented Sep 16, 2022 •

edited

Loading