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

Feature/add replicaset gc pg #2589

Closed
wants to merge 1 commit into from
Closed

Feature/add replicaset gc pg #2589

wants to merge 1 commit into from

Conversation

RamezesDong
Copy link

The implementation for #2585. To fix the bug #2143

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn in a comment when ready.

The full list of commands accepted by this bot can be found 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

@volcano-sh-bot volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 2, 2022
@RamezesDong
Copy link
Author

/assign @k82cn

@RamezesDong RamezesDong force-pushed the feature/add-replicaset-gc-pg branch 2 times, most recently from d15debb to 12acde8 Compare December 2, 2022 08:17
@volcano-sh-bot volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 2, 2022
@volcano-sh-bot volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 2, 2022
wangyang0616
wangyang0616 reviewed Dec 2, 2022
View reviewed changes
pkg/controllers/podgroup/pg_controller.go
@@ -96,6 +98,12 @@ func (pg *pgcontroller) Initialize(opt *framework.ControllerOption) error {
pg.pgLister = pg.pgInformer.Lister()
pg.pgSynced = pg.pgInformer.Informer().HasSynced

pg.rsInformer = opt.SharedInformerFactory.Apps().V1().ReplicaSets()
pg.rsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
Copy link
Member

Choose a reason for hiding this comment

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

Whether to wait for rsInformer sync？

Copy link
Author

@RamezesDong RamezesDong Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's necessary to wait for rsInformer sync. However, I think the reason for bug is in line 101, that rsInformer must be from pg.informerFactory.

feature: add replicaset gc for podgroup
5d781f9 
Signed-off-by: Gaizhi <donghouze@minimac.com>
@stale
Copy link

stale bot commented Feb 2, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 2, 2023
@waiterQ
Copy link
Contributor

waiterQ commented Feb 6, 2023

@RamezesDong can you merge the conflict？

@waiterQ waiterQ mentioned this pull request Feb 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangyang0616 wangyang0616 wangyang0616 left review comments

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu

@k82cn k82cn Awaiting requested review from k82cn

Assignees

@k82cn k82cn

Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RamezesDong @volcano-sh-bot @waiterQ @wangyang0616 @k82cn