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.

Sign up for GitHub

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

Jump to bottom

Bump to k8s 1.31 #664

Merged
merged 5 commits into from
Oct 15, 2024
Merged

Bump to k8s 1.31 #664

merged 5 commits into from
Oct 15, 2024

Conversation

ArangoGutierrez
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez commented Oct 15, 2024 •
edited by tenzen-y
Loading

I upgraded the K8s library versions to v1.31, and controller-tools to v0.16.4.

Fixes: #652

@ArangoGutierrez
Copy link
Contributor Author

Supersedes #655

@ArangoGutierrez ArangoGutierrez marked this pull request as ready for review October 15, 2024 15:15
tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thanks!

Makefile Outdated
KUBECTL_VERSION=v1.30.4
ENVTEST_K8S_VERSION=1.30.0
KUBECTL_VERSION=v1.31.1
ENVTEST_K8S_VERSION=1.31.1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENVTEST_K8S_VERSION=1.31.1
ENVTEST_K8S_VERSION=1.31.0

It seems that the latest version is v1.30.0:

setup-envtest list
(installed)  v1.31.0  darwin/arm64
(installed)  v1.30.0  darwin/arm64
(installed)  v1.29.3  darwin/arm64
(installed)  v1.29.1  darwin/arm64
(installed)  v1.28.3  darwin/arm64
(installed)  v1.27.1  darwin/arm64
(available)  v1.30.0  darwin/arm64
(available)  v1.29.3  darwin/arm64
(available)  v1.29.1  darwin/arm64
(available)  v1.29.0  darwin/arm64
(available)  v1.28.3  darwin/arm64
(available)  v1.28.0  darwin/arm64
(available)  v1.27.1  darwin/arm64
(available)  v1.26.1  darwin/arm64
(available)  v1.26.0  darwin/arm64
(available)  v1.25.0  darwin/arm64
(available)  v1.24.2  darwin/arm64
(available)  v1.24.1  darwin/arm64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

go.mod Outdated
@@ -1,18 +1,18 @@
module github.com/kubeflow/mpi-operator

go 1.22.0
go 1.23.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
go 1.23.0
go 1.23

Could we eliminate the patch version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!
/lgtm
/approve

/hold for CI

@google-oss-prow google-oss-prow bot removed the lgtm label Oct 15, 2024
@tenzen-y
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 15, 2024
@tenzen-y tenzen-y mentioned this pull request Oct 15, 2024
Closed
7 tasks
@tenzen-y
Copy link
Member

tenzen-y commented Oct 15, 2024 •
edited
Loading

/lgtm cancel

We need to upgrade the golangci-lint version.

@google-oss-prow google-oss-prow bot removed the lgtm label Oct 15, 2024
@tenzen-y
Copy link
Member

/approve cancel

@tenzen-y
Copy link
Member

I chat with @ArangoGutierrez offline.
He is trying to resolve CI errors now.

tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
pkg/controller/mpi_job_controller.go
@@ -339,7 +339,7 @@ func NewMPIJobControllerWithClock(
priorityClassSynced: priorityClassSynced,
mpiJobLister: mpiJobInformer.Lister(),
mpiJobSynced: mpiJobInformer.Informer().HasSynced,
queue: workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{Name: "MPIJobs"}),
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),
Copy link
Member

@tenzen-y tenzen-y Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess that we need to do this:

Suggested change
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),

and then,

mpi-operator/pkg/controller/mpi_job_controller.go

Line 250 in dbcbfae

queue workqueue.RateLimitingInterface 

workqueue.TypedRateLimitingInterface[any]

Copy link
Member

Choose a reason for hiding this comment

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

Modified my comment related to types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I was about to comment that any is required.

Copy link
Member

Choose a reason for hiding this comment

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

@ArangoGutierrez Could you address the Line 250 as I mentioned above?

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 did

@ArangoGutierrez
Bump to k8s 1.31
a209ebb 
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@ArangoGutierrez
Bump sigs.k8s.io/controller-runtime to v0.19.0
46f385e 
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@ArangoGutierrez
Bump golangci-lint to v1.61.0
9d5efd8 
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@tenzen-y
Copy link
Member

tenzen-y commented Oct 15, 2024 •
edited
Loading

Additionally, I guess that we need to specify the 22.04:

@tenzen-y
Copy link
Member

It seems that the latest ubuntu runner has many issues: https://github.com/actions/runner-images/issues?q=is%3Aissue+Ubuntu+24.04+runner

@tenzen-y
Copy link
Member

Could you try to change the installation URL in the following?

mpi-operator/Makefile

Line 149 in 86d09e3

curl -L -o $(PROJECT_DIR)/bin/kubectl https://storage.googleapis.com/kubernetes-release/release/${KUBECTL_VERSION}/bin/$(GOOS)/$(GOARCH)/kubectl 

curl -L -o $(PROJECT_DIR)/bin/kubectl https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/$(GOOS)/$(GOARCH)/kubectl

tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
test/e2e/e2e_suite_test.go Outdated
@@ -61,7 +61,7 @@ const (
volcanoSchedulerManifestPath = rootPath + "/dep-manifests/volcano-scheduler/" // all in one yaml of volcano-development.yaml
envUseExistingSchedulerPlugins = "USE_EXISTING_SCHEDULER_PLUGINS"
envUseExistingVolcanoScheduler = "USE_EXISTING_VOLCANO_SCHEDULER"
defaultSchedulerPluginsVersion = "v0.29.8"
defaultSchedulerPluginsVersion = "v0.31.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defaultSchedulerPluginsVersion = "v0.31.1"
defaultSchedulerPluginsVersion = "v0.29.8"

This is the scheduler-plugins version, not k8s version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed back

@ArangoGutierrez
Update kubectl url
16d56d1 
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!
This was not trival efforts!

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Oct 15, 2024
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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

@ArangoGutierrez
Copy link
Contributor Author

Thank you!

/lgtm

/approve

/hold for CI

Should we remove the hold?

@tenzen-y
Copy link
Member

Yup
/hold cancel

@google-oss-prow google-oss-prow bot merged commit a869150 into kubeflow:master Oct 15, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenzen-y tenzen-y tenzen-y left review comments

@gaocegege gaocegege Awaiting requested review from gaocegege

@zw0610 zw0610 Awaiting requested review from zw0610

@carmark carmark Awaiting requested review from carmark

Assignees

@tenzen-y tenzen-y

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Next release date with updated k8s libraries for 1.31
2 participants
@ArangoGutierrez @tenzen-y