Skip to content
This repository was archived by the owner on Apr 17, 2025. It is now read-only.

Update to Go 1.22 #408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pjonsson
Copy link
Contributor

@pjonsson pjonsson commented Mar 2, 2025

Bump the Go versions in various
files and run go mod tidy to clean
up things, Then update to
controller-tools 0.15 to fix
the segmentation fault when running
controller-gen.

Being able to run controller-gen
gives a number of new compilation
errors, so update the K8S packages
to 0.27.16, controller-runtime to
0.15.3 which supports Kubernetes 1.27,
and open-policy-agent/cert-controller
to 0.8.0 which also supports
Kubernetes 1.27.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 2, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @pjonsson. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 2, 2025
@k8s-ci-robot k8s-ci-robot requested a review from tashimi March 2, 2025 14:13
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pjonsson
Once this PR has been reviewed and has the lgtm label, please assign adrianludwin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 2, 2025
c, err := newCachingClient(cache, config, options, uncachedObjects...)
func NewClient(readOnly bool) client.NewClientFunc {
return func(config *rest.Config, options client.Options) (client.Client, error) {
// FIXME: mgr.GetCache() gives us the cache, but this is called when creating
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need help with this one. To me it looks like we need the cache that we get from the manager, while we are in the process of creating the manager.

I'm fairly sure the idea conveyed with my code changes in newCachingClient() is correct though, it's now possible to create a caching client by passing the right options to client.New().

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need the managers cache, we just need a client that will cache unstructured objects. In the previous version of controller-runtime you could only do this with the DelegatedClient. With its removal we can shift the configuration to NewClient. The changes to newCachingClient lgmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed the constant nil parameter and the FIXME. The uncachedObjects argument will never be passed, but I guess that is ok.

@@ -227,7 +228,9 @@ func (a ancestorObjects) add(onm string, ns *forest.Namespace) {
}

func (v *Validator) InjectConfig(cf *rest.Config) error {
mapper, err := apimeta.NewGroupKindMapper(cf)
// FIXME: Where do we get the client from?
client := &http.Client{Timeout: 180}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing we can't change the signature of injection functions, so I have no idea how to get the http.Client at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous comment doesn't matter, because the inject functions are no longer used according to the release notes (https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0).

My impression is that the inject stuff should be moved to the construction of the validator, but this turned into a skill issue. Where are the validators constructed?

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 2, 2025

@rjbez17 I tried to make the minimal change for getting Go 1.22 going (calling +82k-61k lines minimal doesn't feel right). This builds in a Go 1.22 container locally for me, but there are things I don't know how to change so I've made comments on them in this PR that I was hoping you could help me out with.

To get the post-submit tests to not crash require a more recent controller-tools (or controller-runtime, I don't remember), but that brings in a slew of other dependencies that also need to be updated along with some API changes that requires changes in the HNC code. I have some of those updates in a local branch (that doesn't build), but I'm trying to not pile on everything at once.

The bot told me the other PR I was using for testing in the CI needed to be rebased; not sure how I broke that because it worked yesterday, but it might be related to the Go 1.21 update being merged to the main branch. Could you please give this PR the ok-to-test so I could use this PR for the rest?

I also prepared the test-infra bump for later: kubernetes/test-infra#34442

@rjbez17
Copy link
Contributor

rjbez17 commented Mar 3, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 3, 2025
@pjonsson pjonsson force-pushed the update-golang-122 branch from 40df97d to b701e50 Compare March 3, 2025 14:34
Bump the Go versions in various
files and run go mod tidy to clean
up things, Then update to
controller-tools 0.15 to fix
the segmentation fault when running
controller-gen.

Being able to run controller-gen
gives a number of new compilation
errors, so update the K8S packages
to 0.27.16, controller-runtime to
0.15.3 which supports Kubernetes 1.27,
and open-policy-agent/cert-controller
to 0.8.0 which also supports
Kubernetes 1.27.
@pjonsson pjonsson force-pushed the update-golang-122 branch from b701e50 to 2d7e1db Compare March 3, 2025 14:47
@k8s-ci-robot
Copy link
Contributor

@pjonsson: 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-hnc-test 2d7e1db link true /test pull-hnc-test

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

@rjbez17
Copy link
Contributor

rjbez17 commented Mar 5, 2025

Thank you so much for this PR and all the work you've put in. I think I've covered all of your questions so far. I will set aside some time either later tonight or no later than tomorrow night to do a deeper dive and test this out locally. Once we get this merged in and tested I'll cut a maintenance release.

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 5, 2025

@rjbez17 sorry, I wasn't very clear, but #408 (comment) still confuses me. I realize the (long deprecated) injection API is gone so I removed the injection functions, but I don't know how to restore the functionality that removed.

I'm guessing the missing injections are the cause for the current simple test failures in my local container. I think the crash on main for the post-submit-job (E2E-tests?) require the controller-tools update that #374 was meant to solve before I ran into the Go 1.22 requirement.

I suspect fixing this stuff and everything else is a 30 minute thing for someone who knows Go, Kubernetes, and this code base, so you're more than welcome to take this across the finish line.

If you decide to update dependencies further than this PR, the "big" thing I had in my non-building branch is changing *admission.Decoder into admission.Decoder, because that was changed to an interface (or something like that) in some later release of one of the dependencies.

@pjonsson
Copy link
Contributor Author

@rjbez17 I realize you're busy so I didn't want to disturb you the same week, but it's been a couple of weeks now, did other things get in the way of your plans?

@rjbez17
Copy link
Contributor

rjbez17 commented Mar 18, 2025

Hey sorry about this. I tried reaching out in k8s slack. We had a family emergency and I've been out last week. I'll take a look before end of week. Again, so sorry to keep you hanging, I appreciate all the help.

@pjonsson
Copy link
Contributor Author

@rjbez17 I'm sorry to hear that. A few more weeks here or there won't make a big difference, take care of family first and then return to this when the important stuff is taken care of.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

3 participants