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

Top-level MultiKueue cluster API #1631

Merged
merged 9 commits into from
Jan 29, 2024
Merged

Top-level MultiKueue cluster API #1631

merged 9 commits into from
Jan 29, 2024

Conversation

trasc
Copy link
Contributor

@trasc trasc commented Jan 22, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Makes MultiKueueCluster a top level API object and adds a reconciler for it.

Which issue(s) this PR fixes:

Relates to #693

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. 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 Jan 22, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 22, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 22, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 1643981
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65b744dd7f7d8e000807549e

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 22, 2024
@trasc trasc force-pushed the multikueue-ckuster-object branch from d1071e4 to 7a524df Compare January 22, 2024 14:17
@trasc
Copy link
Contributor Author

trasc commented Jan 22, 2024

/test all

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed 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 Jan 22, 2024
@trasc trasc marked this pull request as ready for review January 22, 2024 14:23
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2024
mimowo
mimowo reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

First pass, looks pretty good.

apis/kueue/v1alpha1/multikueueconfig_types.go Show resolved Hide resolved
apis/kueue/v1alpha1/multikueueconfig_types.go Outdated Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/admissioncheck.go Outdated Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/admissioncheck.go Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/admissioncheck.go Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/multikueuecluster.go Outdated Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/multikueuecluster.go Show resolved Hide resolved
pkg/controller/admissionchecks/multikueue/multikueuecluster.go Show resolved Hide resolved
test/integration/multikueue/multikueue_test.go Outdated Show resolved Hide resolved
test/e2e/multikueue/e2e_test.go Outdated Show resolved Hide resolved
@mimowo
Copy link
Contributor

mimowo commented Jan 23, 2024

/retitle Top-level MultiKueue cluster API

@k8s-ci-robot k8s-ci-robot changed the title Multikueue ckuster object Top-level MultiKueue cluster API Jan 23, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024
@trasc trasc force-pushed the multikueue-ckuster-object branch from 7a524df to 9c7fcd8 Compare January 23, 2024 15:33
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024
@trasc trasc force-pushed the multikueue-ckuster-object branch from 9c7fcd8 to a2a8538 Compare January 25, 2024 07:53
mimowo
mimowo reviewed Jan 25, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @tenzen-y @alculquicondor

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@trasc trasc mentioned this pull request Jan 25, 2024
Merged
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2024
@trasc trasc mentioned this pull request Jan 26, 2024
Merged
@alculquicondor
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2024
@alculquicondor
Copy link
Contributor

Maybe this solves the flakiness of #1658

/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29

@alculquicondor
Copy link
Contributor

Another pass:

/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29

@tenzen-y
Copy link
Member

@trasc Remaining comment only here: #1631 (comment)

@trasc
Copy link
Contributor Author

trasc commented Jan 26, 2024

@trasc Remaining comment only here: #1631 (comment)

Is that a blocker?

@tenzen-y
Copy link
Member

@trasc Remaining comment only here: #1631 (comment)

Is that a blocker?

I'm ok with follow-ups for implementations. However, I want to complete its discussion here.

@alculquicondor
Copy link
Contributor

Let's do 2 more passes:

/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29

tenzen-y
tenzen-y reviewed Jan 29, 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.

/lgtm

pkg/controller/admissionchecks/multikueue/multikueuecluster.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2024
@tenzen-y
Copy link
Member

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2024
@k8s-ci-robot k8s-ci-robot requested a review from tenzen-y January 29, 2024 05:05
@tenzen-y
Copy link
Member

The last pass:

/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29

tenzen-y
tenzen-y reviewed Jan 29, 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.

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 1d40942fa5d4562ce973ea6c296646731e079a3a

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tenzen-y, trasc

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:
  • OWNERS [alculquicondor,tenzen-y]

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 merged commit 248255d into kubernetes-sigs:main Jan 29, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.6 milestone Jan 29, 2024
@trasc trasc deleted the multikueue-ckuster-object branch February 8, 2024 07:00
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@trasc @kannon92
Top-level MultiKueue cluster API (kubernetes-sigs#1631)
cbbbfe1 
* [api/v1alpha1] Make MultiKueueCluster an API object.

* [multikueue] Make MultiKueueCluster an API object.

* Fix after rebase

* Review Remarks

* Review Remarks

* Review Remarks

* [API/multikueue] Drop KubeConfig.Name

* Review remarks

* Review remarks
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

@denkensk denkensk Awaiting requested review from denkensk

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@mimowo mimowo Awaiting requested review from mimowo

Assignees

@alculquicondor alculquicondor

@mimowo mimowo

@tenzen-y tenzen-y

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. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.6
Development

Successfully merging this pull request may close these issues.
5 participants
@trasc @k8s-ci-robot @mimowo @alculquicondor @tenzen-y