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

WIP: Bump k8s module versions to v1.27 #880

Closed
wants to merge 1 commit into from

Conversation

tenzen-y
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I upgraded k8s module versions to v1.27 because I prefer to use v1.27 module in kueue v0.4.
But if @trasc has already worked in progress, I will close this PR.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 20, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tenzen-y
Once this PR has been reviewed and has the lgtm label, please assign denkensk for approval. For more information see the Kubernetes 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

@netlify
Copy link

netlify bot commented Jun 20, 2023

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 70a7bb0
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64920160e49e6c0008e54562

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 20, 2023
k8s.io/klog/v2 v2.90.1
k8s.io/utils v0.0.0-20230313181309-38a27ef9d749
sigs.k8s.io/controller-runtime v0.14.6
sigs.k8s.io/controller-runtime v0.15.0
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to upgrade k8s modules with controller-runtime to avoid the following errors:

# sigs.k8s.io/controller-runtime/pkg/cache
../../../pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/cache/multi_namespace_cache.go:308:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced)
../../../pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/cache/multi_namespace_cache.go:321:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced)
../../../pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/cache/multi_namespace_cache.go:326:17: impossible type assertion: h.(map[string]toolscache.ResourceEventHandlerRegistration)
        map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced)

@@ -9,19 +9,20 @@ require (
github.com/kubeflow/mpi-operator v0.4.0
github.com/onsi/ginkgo/v2 v2.11.0
github.com/onsi/gomega v1.27.8
github.com/open-policy-agent/cert-controller v0.7.0
github.com/open-policy-agent/cert-controller v0.7.1-0.20230527042005-3b09cd39622f
Copy link
Member Author

Choose a reason for hiding this comment

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

The cert-controller v0.7.0 doesn't support controller-runtime v0.14.x.

@@ -27,7 +27,7 @@ import (
)

func NewFakeClient(objs ...client.Object) client.Client {
return NewClientBuilder().WithObjects(objs...).Build()
return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).Build()
Copy link
Member Author

Choose a reason for hiding this comment

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

Since controller-runtime v0.15.0, we need to register objects to fakeClient for StatusSubresource.

Testing: Fake client status handling (kubernetes-sigs/controller-runtime#2259)
Added a new WithStatusSubresource option and pre-populating it with
all in-tree resources that have a status subresource
Update and Patch methods now don't change the status for any such
resource anymore
The status clients Update and Patch methods now only change the status
for any such resource

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0

@tenzen-y
Copy link
Member Author

/test pull-kueue-test-e2e-main-1-25

@k8s-ci-robot
Copy link
Contributor

@tenzen-y: The following tests 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-kueue-verify-main 70a7bb0 link true /test pull-kueue-verify-main
pull-kueue-test-unit-main 70a7bb0 link true /test pull-kueue-test-unit-main
pull-kueue-test-e2e-main-1-26 70a7bb0 link true /test pull-kueue-test-e2e-main-1-26
pull-kueue-test-e2e-main-1-24 70a7bb0 link true /test pull-kueue-test-e2e-main-1-24
pull-kueue-test-e2e-main-1-27 70a7bb0 link true /test pull-kueue-test-e2e-main-1-27
pull-kueue-test-e2e-main-1-25 70a7bb0 link true /test pull-kueue-test-e2e-main-1-25

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.

@tenzen-y
Copy link
Member Author

Since it seems that the ComponentConfig doesn't work correctly, we need to work on #725, at first.

@tenzen-y
Copy link
Member Author

@trasc Are you trying to upgrade k8s module versions? If so, I can close this PR.

@trasc
Copy link
Contributor

trasc commented Jun 21, 2023

@trasc Are you trying to upgrade k8s module versions? If so, I can close this PR.

Yes I have something ongoing as part of #856, not very advanced , anyway I'll take #725 for now.

@tenzen-y
Copy link
Member Author

@trasc Are you trying to upgrade k8s module versions? If so, I can close this PR.

Yes I have something ongoing as part of #856, not very advanced , anyway I'll take #725 for now.

Do you have the commit on your local since I can not see the commit to upgrade modules in #856?
Also, can we merge commits into the main branch #725 -> this PR -> #856 in that order?

@trasc
Copy link
Contributor

trasc commented Jun 22, 2023

Also, can we merge commits into the main branch #725 -> this PR -> #856 in that order?

For sure #725 should be first. We should check if we need both (this and #856) and in which order after.

@tenzen-y
Copy link
Member Author

Also, can we merge commits into the main branch #725 -> this PR -> #856 in that order?

For sure #725 should be first. We should check if we need both (this and #856) and in which order after.

I have already checked that this PR will work fine if #726 is resolved.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@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.

@trasc
Copy link
Contributor

trasc commented Jun 23, 2023

Also, can we merge commits into the main branch #725 -> this PR -> #856 in that order?

For sure #725 should be first. We should check if we need both (this and #856) and in which order after.

I have already checked that this PR will work fine if #726 is resolved.

So did I for #856, also #856 contains some changes to the config that I needed vX.27.3, and is based on #889 already.
(the work on jobset is also on hold waiting for #856).

We should go:

#889
#856
#880

/hold

@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 Jun 23, 2023
@tenzen-y
Copy link
Member Author

tenzen-y commented Jun 23, 2023

In favor of #856.
/hold cancel
/close

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

@tenzen-y: Closed this PR.

In response to this:

In favor of #880.
/hold cancel
/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.

@tenzen-y tenzen-y deleted the upgrade-k8s-deps branch June 23, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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