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

kubekins/krte: Add main variant #25355

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Feb 21, 2022

Signed-off-by: Stephen Augustus foo@auggie.dev

/hold for kubernetes/kubernetes#107105
/assign @cpanato @puerco
cc: @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/images area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Feb 21, 2022
@justaugustus justaugustus changed the title kubekins/krte: Build master variant using go1.18rc1 and add main variant kubekins/krte: Build master using go1.18rc1 and add main variant Feb 21, 2022
@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 21, 2022
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, justaugustus

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:

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

@justaugustus
Copy link
Member Author

/test pull-test-infra-unit-test

@BenTheElder
Copy link
Member

I missed, have we tested on -experimental at all yet? 1.18 looks like it has some exciting / major changes :D

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2022
Signed-off-by: Stephen Augustus <foo@auggie.dev>
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 28, 2022
@justaugustus justaugustus changed the title kubekins/krte: Build master using go1.18rc1 and add main variant kubekins/krte: Add main variant Mar 28, 2022
@justaugustus
Copy link
Member Author

#25723 bumped the golang version to go1.18, so I've updated this to only include the new main variant.
@kubernetes/release-engineering -- Needs /lgtm!

I missed, have we tested on -experimental at all yet? 1.18 looks like it has some exciting / major changes :D

@BenTheElder -- We created the go-canary variant in #18328 to prevent disruption for other experimental jobs when changing Golang major versions.

The experimental variant is used in enough non-SIG Release/Testing, that I've never felt comfortable with subjecting that many jobs to the change.

So right now, if someone is feeling adventurous: use go-canary.
If not, they can use the master variant which doesn't get moved to "next" Golang until it lands in k/k.

That said, we don't really have a policy re: variants.
@kubernetes/sig-testing-leads -- WDYT?

@BenTheElder
Copy link
Member

The experimental variant is used in enough non-SIG Release/Testing, that I've never felt comfortable with subjecting that many jobs to the change.

Users opting into -experimental know what they're getting into :-), we've rolled go ahead of k/k in the past here many many times.

IMHO we should be running with new go versions pretty extensively in CI before rolling them to the main repo, that's why -experimental exists (though test-infra now manages go version in repo anyhow ...). Any project choosing to use -experimental is signing up to be experimented on, if that's not suitable for their usage, they need to use another image.

KRTE in particular is more explicitly not even supported for other purposes than testing kubernetes with kind, for this reason (to avoid other usage blocking changes), but "experimental" in the tag should be a pretty good hint + past behavior of regular go upgrades.

I don't think go-canary is used widely enough, and the point of -experimental vs "go-canary" is that it might not only be go that we need to ensure is safe to upgrade, the same CI jobs may also be used to trial e.g. docker upgrades, it's wasteful to have jobs running only intended to be used for go upgrade testing.

At different points in time we have different things to update in the CI environment that should receive the same level of soaking through some e2e canary jobs etc before rolling to main/master / ...

@BenTheElder
Copy link
Member

I'm not sure what happened where this go round, but the go1.18 update was extremely not smooth, it doesn't feel like it got soaked in CI ahead of updating.

@BenTheElder
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2022
@liggitt
Copy link
Member

liggitt commented Mar 28, 2022

if the intent of this is just to enable master/main kubekins tags to be identical, this seems harmless.

I agree we should stop bumping master (and now main) images to a new version of go before demonstrating green runs on the go-canary / experimental image, but that seems outside the scope of this PR.

@justaugustus, is there an issue tracking the fallout of the last CI go bump to 1.18 and how we'll change the rollout for go 1.19?

@justaugustus
Copy link
Member Author

I'm not sure what happened where this go round, but the go1.18 update was extremely not smooth, it doesn't feel like it got soaked in CI ahead of updating.

I agree we should stop bumping master (and now main) images to a new version of go before demonstrating green runs on the go-canary / experimental image, but that seems outside the scope of this PR.

@justaugustus, is there an issue tracking the fallout of the last CI go bump to 1.18 and how we'll change the rollout for go 1.19?

@BenTheElder @liggitt -- I've filed kubernetes/release#2487 for this.

The tl;dr is:

The timing/order of operations for much of this is documented in our golang update issue template, so I think what it came down to here was a few too many folks working on this without following up on the previous steps.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0178a71 into kubernetes:master Apr 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/images area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants