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

Eclass task 1: clean up old code #71399

Open
wants to merge 2 commits into
base: master
from

Conversation

@resouer
Member

resouer commented Nov 25, 2018

Long live equivalence class!

Since new design of eclass is out , it's now safe to remove the old eclass related code. Then PRs of new design can follow up.

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind cleanup

What this PR does / why we need it:

Task 1 of #68720

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #71013

Special notes for your reviewer:

cc @denkensk @Huang-Wei @bsalamat

Marked as WIP since we need to make sure the clean up is complete and tests are happy. Will be ready for review after that.

Does this PR introduce a user-facing change?:

Clean up old eclass code
@resouer

This comment has been minimized.

Member

resouer commented Nov 25, 2018

/assign @Huang-Wei

@resouer

This comment has been minimized.

Member

resouer commented Nov 25, 2018

/priority important-soon

@resouer resouer force-pushed the resouer:eclass-task-1 branch from a3bab97 to 63bbe9c Nov 25, 2018

@resouer resouer changed the title from [WIP] Eclass task 1: clean up old code to Eclass task 1: clean up old code Nov 25, 2018

@k82cn

This comment has been minimized.

Member

k82cn commented Nov 26, 2018

/approve

lgtm overall :)

@Huang-Wei

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 26, 2018

@resouer

This comment has been minimized.

Member

resouer commented Nov 27, 2018

Thanks! We still need someone from sig-testing to review. Kindly ping @jberkus, any bandwidth?

The deleted e2e tests are not enabled by default, so I think it's quite safe. :-)

@bsalamat bsalamat self-assigned this Nov 30, 2018

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Nov 30, 2018

@msau42 FYI

@bsalamat

/lgtm
/approve

Thanks, @resouer! There is a small conflict that should be resolved.

@msau42

This comment has been minimized.

Member

msau42 commented Nov 30, 2018

lgtm!

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 5, 2018

New changes are detected. LGTM label has been removed.

@resouer resouer force-pushed the resouer:eclass-task-1 branch 2 times, most recently from 85baef5 to fb2bbd5 Dec 5, 2018

@misterikkit

Change looks good. Are we planning to keep the config field to enable/disable ecache?

@@ -49,13 +49,6 @@ func init() {
registerAlgorithmProvider(defaultPredicates(), defaultPriorities())
// IMPORTANT NOTES for predicate developers:

This comment has been minimized.

@misterikkit

misterikkit Dec 5, 2018

Contributor

I think this line should be removed also.

This comment has been minimized.

@resouer

resouer Dec 7, 2018

Member

Emm ... this line looks like a valid warning, for the below lines.

// TestCacheInvalidationRace tests that equivalence cache invalidation is correctly
// handled when an invalidation event happens early in a scheduling cycle. Specifically, the event
// occurs after schedulercache is snapshotted and before equivalence cache lock is acquired.
func TestCacheInvalidationRace(t *testing.T) {

This comment has been minimized.

@misterikkit

misterikkit Dec 5, 2018

Contributor

I enjoyed writing this test. Can you also delete syncingMockCache?

This comment has been minimized.

@resouer

resouer Dec 6, 2018

Member

Haha, will do

@@ -365,14 +353,12 @@ func NewConfigFactory(args *ConfigFactoryArgs) Configurator {
},
)
// On add and delete of PVs, it will affect equivalence cache items
// related to persistent volume
// On add and delete of PVs.

This comment has been minimized.

@misterikkit

misterikkit Dec 5, 2018

Contributor

I think the remaining comment doesn't say anything. Let's just remove it.

@@ -884,24 +691,7 @@ func (c *configFactory) deletePodFromSchedulingQueue(obj interface{}) {
}
func (c *configFactory) invalidateCachedPredicatesOnUpdatePod(newPod *v1.Pod, oldPod *v1.Pod) {

This comment has been minimized.

@misterikkit

misterikkit Dec 5, 2018

Contributor

Is there a reason this function isn't deleted?

This comment has been minimized.

@resouer

resouer Dec 6, 2018

Member

Emm, I will blame the latest rebase :-)

@resouer

This comment has been minimized.

Member

resouer commented Dec 6, 2018

@misterikkit Thanks for review! My plan is to remove feature gate for the current implementation as well, as the new design of eclass is not a "cache" any more. Will amend commits.

@resouer resouer force-pushed the resouer:eclass-task-1 branch from fb2bbd5 to abf1086 Dec 7, 2018

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Dec 7, 2018

Thanks, Harry. Please take a look at the "govet" failure. It needs a small fix.

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 7, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsalamat, k82cn, resouer
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: timothysc

If they are not already assigned, you can assign the PR to them by writing /assign @timothysc in a comment when ready.

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

@resouer resouer force-pushed the resouer:eclass-task-1 branch from abf1086 to 2e9508b Dec 10, 2018

@resouer resouer force-pushed the resouer:eclass-task-1 branch from 2e9508b to 16457b1 Dec 12, 2018

Eclass Task 1: clean up old equiv class code
Co-authored-by: Harry Zhang <resouer@gmail.com>
Co-authored-by: Wang Qingcan <wangqingcan@baidu.com>

@resouer resouer force-pushed the resouer:eclass-task-1 branch from 16457b1 to a534cda Dec 12, 2018

@resouer resouer force-pushed the resouer:eclass-task-1 branch from a534cda to af44761 Dec 12, 2018

@resouer

This comment has been minimized.

Member

resouer commented Dec 12, 2018

@bsalamat The PR is rebased frequently these days, maybe you wanna do a final check.

Also, we need someone to approve the change in test, kindly ping @jberkus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment