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

upgrade base image and tooling dependencies #3816

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

BenTheElder
Copy link
Member

@BenTheElder BenTheElder commented Dec 11, 2024

I've been reading through the release notes for these one by one ...

  • go 1.23 should be fine for kind, and appears to be supported (or even required) by the upgraded dependencies
  • Containerd 1.7.x should be safe (reviewed release notes)
  • runc makes me nervous but should be safe ... (kind does weird stuff ...)
  • crictl should be safe
  • containerd fuse-overlayfs doesn't mention any breaking changes
  • cni-plugins should be safe

I think we should also do local-path-provisioner but I haven't finished reviewing that and I want to let some of the CI go ahead and test this much. NOTE: that doesn't yet include the github actions which won't build a node image.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 11, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 11, 2024
@BenTheElder
Copy link
Member Author

looks like we'll have to upgrade golangci-lint

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 11, 2024
@BenTheElder
Copy link
Member Author

ok, go updates are going to be a slightly bigger can of worms ...

@BenTheElder
Copy link
Member Author

will revisit sometime tomorrow

@BenTheElder
Copy link
Member Author

https://github.com/opencontainers/runc/releases/tag/v1.2.3 dropped since I started this PR :-)

@aojea
Copy link
Contributor

aojea commented Dec 11, 2024

runc looks important, several fixes for regressions

@BenTheElder
Copy link
Member Author

The good news is all the prow e2e CI passed flawlessly.

The bad news is https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kind/3816/pull-kind-verify/1866663406874923008

go mod tidy is starting to mutate hack/tools/go.mod with:

$ git diff
diff --git a/hack/tools/go.mod b/hack/tools/go.mod
index ee468e10..ce7cba29 100644
--- a/hack/tools/go.mod
+++ b/hack/tools/go.mod
@@ -1,6 +1,8 @@
 module sigs.k8s.io/kind/hack/tools
 
-go 1.17
+go 1.22.1
+
+toolchain go1.23.4
 
 require (
        github.com/golangci/golangci-lint v1.62.2

@stmcginnis
Copy link
Contributor

go mod tidy is starting to mutate hack/tools/go.mod

Do we need to be as concerned with hack/tools? As long as the top level go.mod isn't being modified, it seems mostly safe to allow internal tooling to be different.

@BenTheElder
Copy link
Member Author

Do we need to be as concerned with hack/tools? As long as the top level go.mod isn't being modified, it seems mostly safe to allow internal tooling to be different.

I think this is OK, but I haven't root-caused yet, and it means we'll have to start duplicating the toolchain version

@BenTheElder
Copy link
Member Author

I'm looking at make generate issues with deepcopy-gen changes.

@BenTheElder
Copy link
Member Author

Just updating the tools module language version to 1.23 works as expected, and I think that makes the most sense.

At some point we'll want to bump the main module up, but we don't have a strong use case for any of the new-fangled features for the CLI binary and we will break people doing go get with distro provided go packages until toolchain is more widely available in those versions. At that point it will mostly be about the go version for tools embedding the packages.

@BenTheElder
Copy link
Member Author

fixed make generate for gengo changes, re-bumping runc now ...

images/base/Dockerfile Outdated Show resolved Hide resolved
@BenTheElder
Copy link
Member Author

BenTheElder commented Dec 11, 2024

aside: I don't generally recommend "big update all the dependencies" PRs, but these are high-touch and the last incoming PR was unresponsive about reviewing release notes. At this point a number of them are inter-related (like go1.23 and some of the base image bumps), and we don't risk disrupting 1.32 release anymore, and I have some time to review the downstream changes and test things out now ...

still WIP, will need new base image and other follow-ups (given the scope of this change I'd rather test the new base image before merge than use post-merge automation and a follow-up PR)

@BenTheElder
Copy link
Member Author

[I'm intending that we roll this + the kind build node-image fixes into a v0.26.0 that defaults to v1.32 kubernetes O(soon) but instead of blocking on that pushed a v1.32 image w/ v0.25.0 for now]

@BenTheElder BenTheElder added this to the v0.26.0 milestone Dec 11, 2024
@BenTheElder
Copy link
Member Author

very happy to see actions (with assorted host runtimes etc) working with updated runc in particular via WIP commit node image (309db31)

@BenTheElder
Copy link
Member Author

BenTheElder commented Dec 11, 2024

@BenTheElder
Copy link
Member Author

golangci-lint timeout is not a flake. seems likely to be related to 8f97ed0 given it was passing last night and the dockerfile changes should be unrelated

@BenTheElder
Copy link
Member Author

looks like it's while linting kindnetd

@BenTheElder
Copy link
Member Author

we did increase the size of that binary recently, maybe just bump the linter timeout. through 2m feels somewhat generous given the amount of in-repo source code.

@BenTheElder
Copy link
Member Author

@BenTheElder
Copy link
Member Author

BenTheElder commented Dec 12, 2024

OK, now everything is working.

I think we should also upgrade kindnetd and local-path-provisioner but this is already pretty huge.

I'm going to drop the temp image bump commits and we'll get new images built normally in postsubmit / follow-ups, I'm fairly satisfied that we've exercised runc+containerd in CI in particular. (1265c24 and 309db31)

@BenTheElder BenTheElder changed the title [WIP] upgrade dependencies upgrade base image and tooling dependencies Dec 12, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2024
@BenTheElder
Copy link
Member Author

I think let's split the rest into further PRs

@BenTheElder
Copy link
Member Author

/retest
flake on 1.31 https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kind/3816/pull-kind-e2e-kubernetes-1-31/1866999826344841216

NOTE: I dropped the image bump PRs, so that's not caused by this PR, just a flaky test

@aojea
Copy link
Contributor

aojea commented Dec 12, 2024

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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

@k8s-ci-robot k8s-ci-robot merged commit 9f82dd4 into kubernetes-sigs:main Dec 12, 2024
30 checks passed
@BenTheElder BenTheElder deleted the upgrades branch December 16, 2024 21:02
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. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants