Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update kubekins / krte to support go version selection using .go-version from repo under test #28310

Closed
liggitt opened this issue Dec 22, 2022 · 19 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing.

Comments

@liggitt
Copy link
Member

liggitt commented Dec 22, 2022

What would you like to be added:

Update kubekins / krte images to include multiple go versions, and select the version to use based on a .go-version file (if present) in the repo / PR under test

I would suggest including the following go versions:

  • last patch release for supported go minor versions (maaaybe last two if it doesn't make the image too big)
  • most recent available release candidate for any upcoming go minor version

At the time this issue was opened, that would look like this:

  • go 1.18.9
  • go 1.19.4
  • go 1.20rc1

The go version could be fetched on demand (e.g. with gimme, as the kind repo does), but my preference would be to have CI and build images be as hermetic as possible.

Why is this needed:

Required presubmits sometimes fail when they shouldn't: jobs like like verify, dependencies sometimes fail when bumping go minor versions in ways that involve formatting or dependency changes (xref gofmt changes in go1.19, etc). We have to build go-canary kubekins images and run optional presubmits to make sure those jobs are green before switching the go version used in blocking CI

Required presubmits sometimes pass when they shouldn't: jobs like unit and integration run using the old go version, which means they don't identify any failures related to the new go version and allow merges of PRs updating to new go minor versions when tests don't actually pass on that version. Again, we have to remember to build go-canary kubekins images and run optional presubmits to make sure tests pass on the new go version.

The go version used to build/test should have a single source of truth: switching the go version used by CI currently has to be closely coordinated between test-infra and kubernetes/kubernetes PRs... the authoritative source of truth for what go version to use to test/build should live solely in the kubernetes/kubernetes repo, so it can take effect in presubmits on an unmerged PR, take effect immediately at HEAD when a PR changing the go version merges, and be rolled back easily with a revert in the kubernetes/kubernetes repo without changing test-infra configs.

cc @dims @BenTheElder @aojea

/sig testing release

@liggitt liggitt added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 22, 2022
@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Dec 22, 2022
@liggitt
Copy link
Member Author

liggitt commented Dec 22, 2022

Added .go-version to kubernetes/kubernetes in kubernetes/kubernetes#114660 proactively to lay the groundwork for this

@liggitt
Copy link
Member Author

liggitt commented Dec 22, 2022

cc @cpanato @xmudrii @justaugustus
as release folks who have had to do the test-infra / k/k CI go version dance before

@aojea
Copy link
Member

aojea commented Dec 22, 2022

+1

@BenTheElder
Copy link
Member

The version in

is also used in

It's wonderful.

I think we can compromise on hermetic by pre-fetching multiple versions of go with gimme in the CI image so when we upgrade we don't actually need to live fetch. But we can also test upgrades / downgrades at-whim by sending a test PR that just changes .go-version and lets it live-fetch.

The KIND scripts (replicated in the other two repos) also use the host's system go toolchain if it's already the desired version, and have an override env to force always using the existing host toolchain (which might be relevant for downstream builds).

@liggitt
Copy link
Member Author

liggitt commented Jan 4, 2023

would you expect gimme to be provided by the host environment or cloned into k/k?

@BenTheElder
Copy link
Member

would you expect gimme to be provided by the host environment or cloned into k/k?

In kind etc. we cloned it into the repo, I think that was a good move.

It's usable as a single MIT licensed shell script under 1k lines with minimal host dependencies (need bash, curl/wget).1 It rarely needs patching, most commits to it are just adding new known go versions to it (which isn't strictly necessary, it can fetch versions it doesn't statically know about).

Cloning it into the repo gives us tighter control and avoids requiring developers to install something else if make / scripts etc. start invoking it to ensure go is setup.

We'd probably also need to put it into krte/kubekins though so we can pre-fetch the versions we want to be available without live fetching.

Given how stable it is, it's not a big deal either way though, I think mostly it improves usability to exec the repo-local copy

Footnotes

  1. Technically there's a second small python script if curl and wget are not available but I think curl or wget are reasonable requirements. We skipped this in the other repos.

@liggitt
Copy link
Member Author

liggitt commented Jan 28, 2023

WIP experiment with that approach at kubernetes/kubernetes#115377. I'm on the fence about whether this should be opt-in or opt-out

@aojea
Copy link
Member

aojea commented Jan 28, 2023

I'm on the fence about whether this should be opt-in or opt-out

can you elaborate?

@liggitt
Copy link
Member Author

liggitt commented Feb 2, 2023

kubernetes/kubernetes#115377 is now ready for review. It turns out that integrating it into the go version setup step in hack/lib/golang.sh made it effective for all our presubmits without any test-infra changes... which was surprising and sort of awesome

@aojea
Copy link
Member

aojea commented Feb 2, 2023

that sounds like impressive

@BenTheElder
Copy link
Member

I was thinking more about this, I think we can start to use .go-version from k/k to dedupe the work to update variants.yaml

Possible options:

  • Periodically rebuild and fetch .go-version from k/k for each branch instead of encoding them in variants.yaml
  • .go-version triggered k/k postsubmit to file a k/test-infra PR to variants.yaml

The latter is more work but might be the right approach. Though I dislike giving yet more code access to the robot account(s).

Either way, updating go version in the image is now only:

  • An optimization for k/k (so we don't always have to live fetch the go binaries)
  • A way to ensure other repos re-using these images keep up with the go version in the variant they've selected

So e.g. the go version in KRTE is now largely irrelevant ...

@liggitt
Copy link
Member Author

liggitt commented Feb 6, 2023

.go-version triggered k/k postsubmit to file a k/test-infra PR to variants.yaml

I would personally prefer something that leaves a record of the baked-in go version in source history over "whatever existed at HEAD of release branches at build-time" behavior. I'm not sure why that requires giving more code access... the PRs don't have to be auto-approved, right?

@BenTheElder
Copy link
Member

I'm not sure why that requires giving more code access... the PRs don't have to be auto-approved, right?

They need access to some github account token in order to file a PR, even if it's an untrusted account we need an account and creating more accounts is a pain so we typically wind up using one of the existing accounts.

The existing automatic PRs in this repo use an account that gets auto-merged PRs.

@BenTheElder
Copy link
Member

I would personally prefer something that leaves a record of the baked-in go version in source history over "whatever existed at HEAD of release branches at build-time" behavior.

I'm not sure why this is relevant though, seeing as kubernetes is going to bypass the preinstalled version unless it matches .go-version anyhow?

Having the matching go version is just an optimization for kubernetes, but one that will work better if it stays up to date automatically.

The go version in the image will directly affect other repos, but we already upgrade go in these images on the basis of Kubernetes and do not roll back due to issues in other repos (they can switch to an older kubernetes branch tagged image, or their own image, or update to be compatible with the go version kubernetes is using), so as-is having it in test-infra git doesn't help them as we're not going to accept a revert.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2023
@BenTheElder
Copy link
Member

We seem to be getting by Ok with just fetching go if the desired version isn't already in the image

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 9, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Projects
None yet
Development

No branches or pull requests

5 participants