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

Introduce the ClusterAutoscaler APIs module #1872

Merged

Conversation

tenzen-y
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I replaced the k8s.io/autoscaler/cluster-autoscaler with the k8s.io/autoscaler/cluster-autoscaler/apis because kubernetes/autoscaler#6315 is finally merged.

Which issue(s) this PR fixes:

Fixes #1345

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 20, 2024
@k8s-ci-robot
Copy link
Contributor

[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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 20, 2024
Copy link

netlify bot commented Mar 20, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit cf651cf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6601869aba72fc0008a9b5b1

@trasc
Copy link
Contributor

trasc commented Mar 20, 2024

kueue/Makefile

Lines 400 to 404 in a751ffd

CAROOT = $(shell $(GO_CMD) list -m -f "{{.Dir}}" k8s.io/autoscaler/cluster-autoscaler)
.PHONY: cluster-autoscaler-crd
cluster-autoscaler-crd:
mkdir -p $(PROJECT_DIR)/dep-crds/cluster-autoscaler/
cp -f $(CAROOT)/config/crd/* $(PROJECT_DIR)/dep-crds/cluster-autoscaler/

needs an update

@tenzen-y
Copy link
Member Author

kueue/Makefile

Lines 400 to 404 in a751ffd

CAROOT = $(shell $(GO_CMD) list -m -f "{{.Dir}}" k8s.io/autoscaler/cluster-autoscaler)
.PHONY: cluster-autoscaler-crd
cluster-autoscaler-crd:
mkdir -p $(PROJECT_DIR)/dep-crds/cluster-autoscaler/
cp -f $(CAROOT)/config/crd/* $(PROJECT_DIR)/dep-crds/cluster-autoscaler/

needs an update

Yes, that's right.
Also, we need to upgrade Golang version to v1.22 due to kubernetes/autoscaler@eb5d875.

go: downloading k8s.io/autoscaler/cluster-autoscaler v0.0.0-20240320105552-09954b6741cb
go: k8s.io/autoscaler/cluster-autoscaler@v0.0.0-20240320105552-09954b6741cb requires go >= 1.22; switching to go1.22.1
go: downloading go1.22.1 (darwin/arm64)
go: downloading k8s.io/autoscaler/cluster-autoscaler v0.0.0-20240320105552-09954b6741cb

Or, putting https://github.com/kubernetes/autoscaler/tree/09954b6741cbb910971916c079f45f6e8878d192/cluster-autoscaler/config/crd to https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/apis/provisioningrequest would be better.

@alculquicondor @x13n WDYT?

@x13n
Copy link

x13n commented Mar 21, 2024

Putting the CRD in CA apis module makes sense to me, it has to be in sync with the code there anyway.

@alculquicondor
Copy link
Contributor

I'm also ok upgrading to golang 1.22

But all the APIs should be in the separate module

@tenzen-y
Copy link
Member Author

@x13n @alculquicondor Thank you for responding.
I can take both approaches.
Let me try them.

@tenzen-y
Copy link
Member Author

@x13n @alculquicondor Thank you for responding. I can take both approaches. Let me try them.

Raised at kubernetes/autoscaler#6651

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024
@mwielgus
Copy link
Contributor

kubernetes/autoscaler#6651 is merged.

@tenzen-y
Copy link
Member Author

kubernetes/autoscaler#6651 is merged.

Thank you!

@tenzen-y tenzen-y mentioned this pull request Mar 25, 2024
@tenzen-y tenzen-y force-pushed the use-cluster-autoscaler-api-lib branch from 305f2f9 to db5acf4 Compare March 25, 2024 13:57
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024
@tenzen-y tenzen-y force-pushed the use-cluster-autoscaler-api-lib branch from db5acf4 to 2fe331a Compare March 25, 2024 13:58
@tenzen-y tenzen-y assigned tenzen-y and unassigned tenzen-y Mar 25, 2024
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y tenzen-y force-pushed the use-cluster-autoscaler-api-lib branch from 2fe331a to cf651cf Compare March 25, 2024 14:13
@tenzen-y
Copy link
Member Author

@alculquicondor This PR is ready for review.

Copy link
Contributor

@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.

Thank you so much @tenzen-y!!!!

This was a long effort that simplifies the maintainability of our deps greatly!

@tenzen-y
Copy link
Member Author

Thank you so much @tenzen-y!!!!

This was a long effort that simplifies the maintainability of our deps greatly!

I appreciate to collaboration with SIG-Autoscaling (@x13n and @mwielgus)!

@alculquicondor btw maybe you forgot to add /lgtm label 🙃

@tenzen-y
Copy link
Member Author

cc: @B1F030 @village-way

@alculquicondor
Copy link
Contributor

I did indeed forget :)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2111d2abc5d15cc89d44ac302fa4c42e8b44d122

@tenzen-y
Copy link
Member Author

@alculquicondor How about cherry-picking this into the release-0.6 branch?

@alculquicondor
Copy link
Contributor

I'm not too sure about that. Why do you suggest it?

Also, I'm not sure what's the origin of the Job failure.

@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 25, 2024

I'm not too sure about that. Why do you suggest it?

Cherry-picking would allow cluster-admins to avoid #1345 error when cluster-admins implement a separate controller for the in-house job.

[MODIFIED COMMENT]

@tenzen-y
Copy link
Member Author

Also, I'm not sure what's the origin of the Job failure.

I'll try to investigate CI failure.

@tenzen-y
Copy link
Member Author

...
[FAILED] Test Report unavailable because a Ginkgo parallel process disappeared
    The aggregated report could not be fetched for a ReportAfterSuite node.  A
    Ginkgo parallel process disappeared before it could finish reporting.
...
This occurs if a parallel process exits before it reports its results to the
Ginkgo CLI.  The CLI will now print out all the stdout/stderr output it's
collected from the running processes.  However you may not see anything useful
in these logs because the individual test processes usually intercept output to
stdout/stderr in order to capture it in the spec reports.

Maybe this is the reason 🧐

@alculquicondor
Copy link
Contributor

One of my teammates is working in splitting the Multikueue E2E test into its own job. Let's decide if the limits are ok after that.

@tenzen-y
Copy link
Member Author

One of my teammates is working in splitting the Multikueue E2E test into its own job. Let's decide if the limits are ok after that.

That makes sense.
/test pull-kueue-test-integration-main

@tenzen-y
Copy link
Member Author

I'm not too sure about that. Why do you suggest it?

Cherry-picking would allow cluster-admins to avoid #1345 error when cluster-admins implement a separate controller for the in-house job.

[MODIFIED COMMENT]

@alculquicondor How about this?

@k8s-ci-robot k8s-ci-robot merged commit d665c9a into kubernetes-sigs:main Mar 25, 2024
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.7 milestone Mar 25, 2024
@tenzen-y tenzen-y deleted the use-cluster-autoscaler-api-lib branch March 25, 2024 22:24
@alculquicondor
Copy link
Contributor

/cherry-pick release-0.6

@k8s-infra-cherrypick-robot

@alculquicondor: #1872 failed to apply on top of branch "release-0.6":

Applying: Introduce the ClusterAutoscaler APIs module
Using index info to reconstruct a base tree...
M	Makefile
M	cmd/kueue/main.go
M	go.mod
M	go.sum
M	pkg/controller/admissionchecks/provisioning/controller.go
M	pkg/controller/admissionchecks/provisioning/controller_test.go
M	pkg/controller/admissionchecks/provisioning/indexer_test.go
M	test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/indexer_test.go
CONFLICT (content): Merge conflict in pkg/controller/admissionchecks/provisioning/indexer_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/controller_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/controller.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging cmd/kueue/main.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Introduce the ClusterAutoscaler APIs module
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.6

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
Copy link
Member Author

@alculquicondor: #1872 failed to apply on top of branch "release-0.6":

Applying: Introduce the ClusterAutoscaler APIs module
Using index info to reconstruct a base tree...
M	Makefile
M	cmd/kueue/main.go
M	go.mod
M	go.sum
M	pkg/controller/admissionchecks/provisioning/controller.go
M	pkg/controller/admissionchecks/provisioning/controller_test.go
M	pkg/controller/admissionchecks/provisioning/indexer_test.go
M	test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/indexer_test.go
CONFLICT (content): Merge conflict in pkg/controller/admissionchecks/provisioning/indexer_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/controller_test.go
Auto-merging pkg/controller/admissionchecks/provisioning/controller.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging cmd/kueue/main.go
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Introduce the ClusterAutoscaler APIs module
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

ACK

vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to perform "go list -modfile=./go.mod -m all"
7 participants