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 dependencies to k8s.io v0.27.2 #619

Closed

Conversation

SimonTheLeg
Copy link
Contributor

@SimonTheLeg SimonTheLeg commented Apr 25, 2023

This PR updates the dependencies to match the v0.27.2 version of the k8s.io components.
Go mod also automatically updates kyaml to v0.14.1 to keep compatibility.

With v0.27, DryRunVerifier has been deprecated (kubernetes/kubernetes#114294) without any replacement. Therefore this PR also removes any references.

We also need to update controller-runtime to v0.15.0 to work with v0.27.2. This brings with it another breaking change for the NewDynamicRESTMapper, which is adopted as well.

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

Welcome @SimonTheLeg!

It looks like this is your first PR to kubernetes-sigs/cli-utils 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cli-utils has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@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 Apr 25, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @SimonTheLeg. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 25, 2023
@ash2k
Copy link
Member

ash2k commented Apr 26, 2023

/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 Apr 26, 2023
@SimonTheLeg
Copy link
Contributor Author

Failures are due to controller runtime not yet supporting v0.27.1, which will be achieved once this is merged: kubernetes-sigs/controller-runtime#2189

side-note from me: the controller-runtime PR is dependent on a new release in api-machinery, which has to include this commit kubernetes/apimachinery@c2d59b0

So I guess the only option is to wait for now..

@ash2k
Copy link
Member

ash2k commented May 30, 2023

This should be unblocked now.

@SimonTheLeg SimonTheLeg changed the title update dependencies to k8s.io v0.27.1 update dependencies to k8s.io v0.27.2 May 30, 2023
@k8s-ci-robot 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 May 30, 2023
@SimonTheLeg
Copy link
Contributor Author

yes! I have updated the PR. One thing: with controller-runtime v0.15.0 there is a breaking change in NewDynamicRESTMapper, which now requires an http.Client to passed in. From reading the associated PR kubernetes-sigs/controller-runtime#2122, I understand that it is mainly done to re-use clients. Since we don't really need to do this I just went with the defaultClient for now. @ash2k I would appreciate if you could take a look, if I understood this correctly. Thanks! :)

@ash2k
Copy link
Member

ash2k commented May 30, 2023

@SimonTheLeg Thanks for updating the PR! I think controller-runtime is often a source of issues. There is no reason not to use vanilla client-go/etc. I think we should drop this dependency for everybody's benefit.

I think default client is fine because it's only used in tests.

/lgtm

Not sure I can lgtm.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023
@ash2k
Copy link
Member

ash2k commented Jun 8, 2023

/retest

@SimonTheLeg
Copy link
Contributor Author

CI complains about a deprecation notice on the NewExponentialBackoffManager() func. I have tried to apply what is suggested in the deprecation notice, but it leads to compiler errors. For now I have opened an issue kubernetes/kubernetes#118638

@ash2k
Copy link
Member

ash2k commented Jun 15, 2023

@SimonTheLeg Is it possible to silence the linter and use the deprecated API for now until the upstream issue is fixed? I hope we don't need to wait for it to be resolved.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2023
@SimonTheLeg
Copy link
Contributor Author

/retest

@SimonTheLeg
Copy link
Contributor Author

I have added the nolint, and cli-utils-presubmit-master is now working. The other two CI jobs though seam to fail during or shortly after cluster creation. And from a first look, I don't think this is related to any change in this PR. Could someone have a look?

@mortent
Copy link
Member

mortent commented Jun 19, 2023

@SimonTheLeg I suspect this might be related to kubernetes/test-infra#29742, but I will try to take a look.

@SimonTheLeg
Copy link
Contributor Author

/retest

1 similar comment
@ash2k
Copy link
Member

ash2k commented Jun 27, 2023

/retest

@mortent
Copy link
Member

mortent commented Jun 27, 2023

I think there is a problem with the stress tests that is not related to the changes here. It looks like the prow pod just gets killed, so my guess is that it is using more resources than is allowed.

@ash2k
Copy link
Member

ash2k commented Jul 18, 2023

/retest

@ash2k
Copy link
Member

ash2k commented Jul 18, 2023

@mortent I think something else might be going on. Here are the final lines from the logs of the latest run:

  STEP: Verify inventory deleted @ 07/18/23 06:01:51.823
  STEP: Verify 1000 CronTabs deleted @ 07/18/23 06:01:51.825
  STEP: Verify 1000 ConfigMaps deleted @ 07/18/23 06:01:51.826
  STEP: Verify 1000 Namespaces deleted @ 07/18/23 06:01:51.828
  STEP: Verify CRD deleted @ 07/18/23 06:01:51.829
  STEP: Cleanup ConfigMaps @ 07/18/23 06:01:51.832
  STEP: Cleanup Namespaces @ 07/18/23 06:01:51.833
• [175.712 seconds]
------------------------------
[AfterSuite] 
/home/prow/go/src/sigs.k8s.io/cli-utils/test/stress/stress_test.go:75
[AfterSuite] PASSED [0.003 seconds]
------------------------------
Ran 3 of 3 Specs in 1378.344 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
You're using deprecated Ginkgo functionality:
=============================================
  --ginkgo.slow-spec-threshold is deprecated --slow-spec-threshold has been deprecated and will be removed in a future version of Ginkgo.  This feature has proved to be more noisy than useful.  You can use --poll-progress-after, instead, to get more actionable feedback about potentially slow specs and understand where they might be getting stuck.
To silence deprecations that can be silenced set the following environment variable:
  ACK_GINKGO_DEPRECATIONS=2.9.5
Ginkgo ran 1 suite in 23m53.528413139s
Test Suite Passed
+ EXIT_VALUE=0
+ set +o xtrace
Cleaning up after docker in docker.
================================================================================
Cleaning up after docker
Error response from daemon: Could not kill running container 753db20ac63aada47a06c08bb560c748ea2731e1bb9bced39ef24bf419db351c, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 87203d58796e4141d56ec8031e1e074439b96029a30b3f307f0512aa5dd739a5, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container d26abce62bc8b8da5c4a97104faeaaed98cc05b3501b47f4949858fe8be460d6, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 5b32a2aaa7b874ef9fdb3b06ab26d8f7555537a46618b9a68cee5d2f68a8991a, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 39d9f5c32cd93f004f1e14c5180ffb382d565233b7a6a5d5a5dd49d5bdd45a68, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 08442bb9e92de8cebce642578c5033f75ad6b9e4d8e50da663e0bb6b4184b0d0, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container b978bb1ea3f972975491ca5d878754203d319f358c7f3d229263177687360ce1, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 2da012b6c0b20b7b2b4317efc484a8d09d2c9a2acb146024a96cc26b629fce12, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container b690f3bd9889787cc4ac09c9f5095f6f15fc862315198a386671d2c1b60e8eb3, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container 2289e514f0e68497b027ceee89137ecdd27fc22a111ea3ce0c080ebbb6e49fe3, cannot remove - tried to kill container, but did not receive an exit event
Error response from daemon: Could not kill running container ba9918f48b4412e1ec8e30c911fd2a9f0f37b1a9894bb4cb7de5e16fe2052dff, cannot remove - tried to kill container, but did not receive an exit event
Stopping Docker: dockerProgram process in pidfile '/var/run/docker-ssd.pid', 1 process(es), refused to die.
================================================================================
Done cleaning up after docker in docker. 

That page actually says passed:

Test started today at 3:36 PM passed after 30m7s.

Job execution failed: Pod got deleted unexpectedly

I wonder if this is related to migration to a different cluster in kubernetes/test-infra#29742.

@ash2k
Copy link
Member

ash2k commented Jul 18, 2023

If we run this job for master, will it succeed?

@ash2k
Copy link
Member

ash2k commented Jul 18, 2023

@ash2k
Copy link
Member

ash2k commented Jul 24, 2023

/retest

Copy link
Member

@ash2k ash2k left a comment

Choose a reason for hiding this comment

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

/lgtm

@mortent This has finally passed CI, ready for merge. Cheers.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2023
@ash2k
Copy link
Member

ash2k commented Aug 3, 2023

@mortent @karlkfi Could you merge this please? Any blockers?

Without going into too much detail, there is a CVE in docker, which is a transitive dependency in my project, and I cannot update it because that also requires Kubernetes 1.27 libraries, which I cannot use because cli-utils wouldn't compile (see the diff in this MR for breaking changes). I could use the branch sha, but it'd be nicer to just get this merged.

@seans3
Copy link
Contributor

seans3 commented Aug 7, 2023

looks good to me. I will approve shortly, unless someone has an objection.

OAN: DryRunVerifier is no longer needed

Since the API Server has understood server-side apply since v1.13, we can assume all current servers will correctly handle the server-side dry-run parameter.

@mortent
Copy link
Member

mortent commented Aug 7, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, mortent, SimonTheLeg

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2023
@seans3
Copy link
Contributor

seans3 commented Aug 7, 2023

/retest

2 similar comments
@ash2k
Copy link
Member

ash2k commented Aug 7, 2023

/retest

@ash2k
Copy link
Member

ash2k commented Aug 7, 2023

/retest

@ash2k
Copy link
Member

ash2k commented Aug 8, 2023

"/home/prow/go/bin/golangci-lint" run ./...
make: *** [Makefile:86: lint] Killed

Hm....

@ash2k
Copy link
Member

ash2k commented Aug 8, 2023

/retest

@k8s-ci-robot
Copy link
Contributor

@SimonTheLeg: 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
cli-utils-presubmit-master 852a880 link true /test cli-utils-presubmit-master

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.

@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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 added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2023
@ash2k
Copy link
Member

ash2k commented Aug 9, 2023

@SimonTheLeg I've incorporated these changes into #629, which got merged. Thank you!

/close

@k8s-ci-robot
Copy link
Contributor

@ash2k: Closed this PR.

In response to this:

@SimonTheLeg I've incorporated these changes into #629, which got merged. Thank you!

/close

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.

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants