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

Upgrade K8s dependencies to v1.29 #633

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Upgrade K8s dependencies to v1.29 #633

merged 1 commit into from
Apr 17, 2024

Conversation

tenzen-y
Copy link
Member

I upgraded the K8s dependencies to v1.29. Also, I upgraded the scheduler-plugins version to the latest (v1.28.9).

@tenzen-y
Copy link
Member Author

/hold
for #631 (comment)

@tenzen-y tenzen-y mentioned this pull request Apr 17, 2024
Merged
@alculquicondor
Copy link
Collaborator

I think it's the easiest path if you include the golang upgrade here.

@tenzen-y
Copy link
Member Author

I think it's the easiest path if you include the golang upgrade here.

I'll try to do it.

@tenzen-y tenzen-y force-pushed the bump-k8s-dependencies branch 2 times, most recently from af91a0f to f8cfc3e Compare April 17, 2024 13:02
@tenzen-y
Copy link
Member Author

tenzen-y commented Apr 17, 2024 •
edited
Loading

@alculquicondor The Bumping Go version seems to depend on controller-gen version: https://github.com/kubeflow/mpi-operator/actions/runs/8722386744/job/23928137672?pr=633

So, I'd like to bump the go version in your PR.

@tenzen-y tenzen-y force-pushed the bump-k8s-dependencies branch 4 times, most recently from 6ad6438 to 15bb02b Compare April 17, 2024 14:39
@google-oss-prow google-oss-prow bot added size/XXL and removed size/L labels Apr 17, 2024
@alculquicondor
Copy link
Collaborator

can you rebase?

@tenzen-y
Copy link
Member Author

can you rebase?

I have rebased, but I faced other issues. So, I'm trying to investigate the issue.

tenzen-y
tenzen-y commented Apr 17, 2024
View reviewed changes
Makefile
Comment on lines -200 to +201
-@GOPATH=/tmp go install volcano.sh/volcano/cmd/scheduler@$(VOLCANO_SCHEDULER_VERSION)
rm -rf /tmp/volcano.sh/volcano
git clone -b $(VOLCANO_SCHEDULER_VERSION) --depth 1 https://github.com/volcano-sh/volcano /tmp/volcano.sh/volcano
Copy link
Member Author

Choose a reason for hiding this comment

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

Due to golang/go#44840, go install doesn't work fine.

alculquicondor
alculquicondor reviewed Apr 17, 2024
View reviewed changes
Makefile Outdated
@@ -118,11 +118,11 @@ test_images:

.PHONY: tidy
tidy:
go mod tidy -go 1.20
go mod tidy -go 1.22
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
go mod tidy -go 1.22
go mod tidy

It's already in go.mod

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense.
Done.

Makefile Outdated

.PHONY: lint
lint: bin/golangci-lint ## Run golangci-lint linter
$(GOLANGCI_LINT) run --new-from-rev=origin/master --go 1.20
$(GOLANGCI_LINT) run --new-from-rev=origin/master --go 1.22
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we also remove this flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM
Done.

alculquicondor
alculquicondor reviewed Apr 17, 2024
View reviewed changes
test/e2e/e2e_suite_test.go Outdated
@@ -172,7 +172,7 @@ func bootstrapKindCluster() error {
}

func installOperator() error {
err := runCommand(kubectlPath, "apply", "-k", operatorManifestsPath)
err := runCommand(kubectlPath, "apply", "--server-side=true", "-k", operatorManifestsPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting.... This will have to go to the documentation as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now, update the README. Just --server-side should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting.... This will have to go to the documentation as well.

Based on this error, I added the server-side flag.
For sire, we should mention it in the document.

For now, update the README. Just --server-side should be enough.

That makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@tenzen-y
Copy link
Member Author

tenzen-y commented Apr 17, 2024 •
edited
Loading

The eventChecker seems to be broken...

@tenzen-y
Copy link
Member Author

The eventChecker seems to be broken...

NVM. I found another reason.

@tenzen-y tenzen-y force-pushed the bump-k8s-dependencies branch 2 times, most recently from f01b67b to 1582628 Compare April 17, 2024 16:42
tenzen-y
tenzen-y commented Apr 17, 2024
View reviewed changes
test/integration/mpi_job_controller_test.go
@@ -1176,6 +1176,7 @@ func newInt32(v int32) *int32 {
func eventForJob(event corev1.Event, job *kubeflow.MPIJob) corev1.Event {
event.Namespace = job.Namespace
event.Source.Component = "mpi-job-controller"
event.ReportingController = "mpi-job-controller"
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that dedicated ReportingController is returned.

I0418 01:30:35.591211   23178 event.go:364] Event(v1.ObjectReference{Kind:"MPIJob", Namespace:"test-2mcds", Name:"job", UID:"ef9b9814-9220-4d4c-bbdd-e93b23486d6d", APIVersion:"kubeflow.org/v2beta1", ResourceVersion:"223", FieldPath:""}): type: 'Normal' reason: 'MPIJobCreated' MPIJob test-2mcds/job is created.
+++++++++++++++++++++++++++++++++++:   &v1.Event{
        ... // 5 ignored and 7 identical fields
        Action:              "",
        Related:             nil,
-       ReportingController: "",
+       ReportingController: "mpi-job-controller",
        ReportingInstance:   "",
  }

@tenzen-y
Copy link
Member Author

@alculquicondor Could you take another look?

@tenzen-y
Copy link
Member Author

/hold cancel

alculquicondor
alculquicondor reviewed Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

just one nit

.github/workflows/golangci-lint.yml Outdated
version: v1.53.3
args: --timeout=5m --go=1.20
version: v1.57.2
args: --timeout=5m --go=1.22
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove --go=1.22?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!
Done.

@alculquicondor
Copy link
Collaborator

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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

@google-oss-prow google-oss-prow bot merged commit ae7c738 into kubeflow:master Apr 17, 2024
10 checks passed
@tenzen-y tenzen-y deleted the bump-k8s-dependencies branch April 17, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@carmark carmark Awaiting requested review from carmark

@gaocegege gaocegege Awaiting requested review from gaocegege

Assignees

@alculquicondor alculquicondor

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

Successfully merging this pull request may close these issues.
2 participants
@tenzen-y @alculquicondor