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

feat: migrate elasticquota controller to ctrl runtime #527

Merged
merged 1 commit into from
Mar 30, 2023
Merged

feat: migrate elasticquota controller to ctrl runtime #527

merged 1 commit into from
Mar 30, 2023

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Mar 1, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #
#485

Special notes for your reviewer:

Does this PR introduce a user-facing change?

migrate ElasticQuota to use controller-runtime controllers

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 1, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @czybjtu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 1, 2023
@Huang-Wei
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2023
@Gekko0114 Gekko0114 mentioned this pull request Mar 2, 2023
Closed
@Huang-Wei
Copy link
Contributor

/assign @zwpaper

@k8s-ci-robot
Copy link
Contributor

@Huang-Wei: GitHub didn't allow me to assign the following users: zwpaper.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @zwpaper

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.

@nayihz
Copy link
Contributor Author

nayihz commented Mar 3, 2023

cc @zwpaper PTAL

@zwpaper
Copy link
Member

zwpaper commented Mar 3, 2023

hi @czybjtu, sorry for the later response, I will look into this PR in the following days.

btw, scheduler-plugins is cutting off for the 0.25.x release, the WIP migrations of ctl-runtime will be planned to the next releases.

@nayihz
Copy link
Contributor Author

nayihz commented Mar 3, 2023

hi @czybjtu, sorry for the later response, I will look into this PR in the following days.

btw, scheduler-plugins is cutting off for the 0.25.x release, the WIP migrations of ctl-runtime will be planned to the next releases.

Got it. Thanks for your reminding.

@zwpaper
Copy link
Member

zwpaper commented Mar 3, 2023

/assign

@zwpaper zwpaper mentioned this pull request Mar 4, 2023
Closed
17 tasks
zwpaper
zwpaper suggested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

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

hi @czybjtu, please revert the unnecessary indentation changes in yaml files, and added some other comments

pkg/controllers/elasticquota_controller.go
Comment on lines +111 to +107
if err := r.List(ctx, podList, client.InNamespace(namespace)); err != nil {
return nil, err
}

for _, p := range podList.Items {
if p.Status.Phase == v1.PodRunning {
used = quota.Add(used, computePodResourceRequest(&p))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

how about using a field selector

@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 9, 2023
@zwpaper
Copy link
Member

zwpaper commented Mar 9, 2023

/milestone v1.26

@Huang-Wei Huang-Wei added this to the v1.26 milestone Mar 10, 2023
@nayihz nayihz force-pushed the feat_elasticquota_ctrl branch from cd54d9d to 2f5cf9b Compare March 11, 2023 11:20
@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 11, 2023
@nayihz
Copy link
Contributor Author

nayihz commented Mar 12, 2023

/test pull-scheduler-plugins-integration-test-master

zwpaper
zwpaper suggested changes Mar 13, 2023
View reviewed changes
Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

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

hi @czybjtu, I have one more question, add review

@nayihz nayihz force-pushed the feat_elasticquota_ctrl branch from 2f5cf9b to 1cce271 Compare March 14, 2023 07:11
@nayihz
Copy link
Contributor Author

nayihz commented Mar 14, 2023

/test pull-scheduler-plugins-integration-test-master

@zwpaper
Copy link
Member

zwpaper commented Mar 14, 2023

/lgtm

please have a look at this PR when you have some cycle @Huang-Wei @denkensk

/assign @Huang-Wei @denkensk

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2023
@nayihz nayihz force-pushed the feat_elasticquota_ctrl branch from 1cce271 to af59d2e Compare March 20, 2023 01:29
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2023
@nayihz nayihz requested review from zwpaper and removed request for cwdsuzhou and yuanchen8911 March 24, 2023 01:45
@zwpaper
Copy link
Member

zwpaper commented Mar 24, 2023

/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 24, 2023
@ffromani
Copy link
Contributor

/cc

@k8s-ci-robot k8s-ci-robot requested a review from ffromani March 28, 2023 06:01
@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 28, 2023
@nayihz nayihz force-pushed the feat_elasticquota_ctrl branch from af59d2e to 7861560 Compare March 28, 2023 12:52
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 28, 2023
@nayihz
Copy link
Contributor Author

nayihz commented Mar 28, 2023

PR rebased.
kindly ping @zwpaper for lgtm, and @Huang-Wei @denkensk for review and approval.

Huang-Wei
Huang-Wei reviewed Mar 28, 2023
View reviewed changes
pkg/controllers/elasticquota_controller.go Outdated
Comment on lines 178 to 181
podMetaType := &metav1.PartialObjectMetadata{}
podMetaType.SetGroupVersionKind(schema.GroupVersionKind{Kind: "Pod", Group: "", Version: "v1"})
return ctrl.NewControllerManagedBy(mgr).
Watches(&source.Kind{Type: podMetaType}, &handler.EnqueueRequestForObject{}).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the usage of PartialObjectMetadata intentional? otherwise, this can achieve the same goal:

Suggested change
podMetaType := &metav1.PartialObjectMetadata{}
podMetaType.SetGroupVersionKind(schema.GroupVersionKind{Kind: "Pod", Group: "", Version: "v1"})
return ctrl.NewControllerManagedBy(mgr).
Watches(&source.Kind{Type: podMetaType}, &handler.EnqueueRequestForObject{}).
return ctrl.NewControllerManagedBy(mgr).
Watches(&source.Kind{Type: &v1.Pod{}}, &handler.EnqueueRequestForObject{}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both ways are OK. Adopt your suggestion to keep consistency with podgroup controller.

@nayihz nayihz force-pushed the feat_elasticquota_ctrl branch from 7861560 to c5f2698 Compare March 29, 2023 05:53
Huang-Wei
Huang-Wei reviewed Mar 30, 2023
View reviewed changes
Copy link
Contributor

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

/approve
(leave lgtm to other reviewers)

Thanks @czybjtu!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czybjtu, Huang-Wei

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2023
@zwpaper
Copy link
Member

zwpaper commented Mar 30, 2023

/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 30, 2023
@k8s-ci-robot k8s-ci-robot merged commit 549c61d into kubernetes-sigs:master Mar 30, 2023
@nayihz nayihz deleted the feat_elasticquota_ctrl branch March 30, 2023 01:39
@nayihz nayihz mentioned this pull request Apr 10, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Huang-Wei Huang-Wei Huang-Wei left review comments

@Gekko0114 Gekko0114 Gekko0114 left review comments

@zwpaper zwpaper Awaiting requested review from zwpaper

@ffromani ffromani Awaiting requested review from ffromani

Assignees

@Huang-Wei Huang-Wei

@zwpaper zwpaper

@denkensk denkensk

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/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.
7 participants
@nayihz @k8s-ci-robot @Huang-Wei @zwpaper @ffromani @Gekko0114 @denkensk