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

Set 0 sync period in scheduler integration test #82222

Merged
merged 2 commits into from Nov 16, 2020
Merged

Set 0 sync period in scheduler integration test #82222

merged 2 commits into from Nov 16, 2020

Conversation

Huang-Wei
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The resync period reconciles between the in-memory cache and the business logic. It's a misunderstanding that it's used to sync with apiserver. A common pattern is set it to 0 and widely used across the codebase.

This PR sets the resync period to 0.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

/sig scheduling
/priority important-longterm

@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 1, 2019
keerthigd
keerthigd reviewed Sep 1, 2019
View reviewed changes
test/integration/scheduler/framework_test.go Outdated
@@ -448,7 +448,7 @@ func TestPreFilterPlugin(t *testing.T) {
// Create the master and the scheduler with the test plugin set.
context := initTestSchedulerWithOptions(t,
initTestMaster(t, "prefilter-plugin", nil),
false, nil, registry, plugins, emptyPluginConfig, false, time.Second)
false, nil, registry, plugins, emptyPluginConfig, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a good idea if you specify the sync period as 0 explicitly? rather than assuming the default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting to 0 is a common pattern and I believe it's the default value. And again, the "sync period" is NOT syncing with apiserver. See:

Copy link
Member Author

Choose a reason for hiding this comment

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

And setting to 0 doesn’t mean never resync, my memory is that it underneath sets it to a random 10ish minute.

@Huang-Wei
Copy link
Member Author

/retest

1 similar comment
@Huang-Wei
Copy link
Member Author

/retest

hex108
hex108 approved these changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@hex108 hex108 left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

The detail for resync period could be found at https://groups.google.com/forum/#!msg/kubernetes-sig-api-machinery/PbSCXdLDno0/hEG1YykvDQAJ

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2019
@liggitt
Copy link
Member

liggitt commented Sep 11, 2019

/hold

batches including this PR are flaking badly on scheduler integration tests:

please review closely to ensure this is not introducing a regression

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2019
This was referenced Sep 11, 2019
Merged
Merged
@liggitt
Copy link
Member

liggitt commented Sep 11, 2019

all scheduler-related PRs in the failing batches: #82187 #82209 #82222

@Huang-Wei
Copy link
Member Author

This PR needs further work. I will take a close look.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 10, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 9, 2020
@Huang-Wei
Copy link
Member Author

/remove-lifecycle rotten

@Huang-Wei Huang-Wei mentioned this pull request Oct 30, 2020
Closed
@Huang-Wei
Copy link
Member Author

Still holding this PR, I will resolve comments after #96062 and #96071 get merged.

@alculquicondor alculquicondor mentioned this pull request Nov 2, 2020
Merged
@Huang-Wei
Copy link
Member Author

/retest

@liggitt
Copy link
Member

liggitt commented Nov 9, 2020

/hold

is this making the integration tests run in a way that does not match the real kube-scheduler environment? The analysis in #96071 (comment) seems to indicate the event handlers the scheduler is attaching to the informer do not handle update events correctly. Is this PR masking that bug?

@Huang-Wei
Copy link
Member Author

is this making the integration tests run in a way that does not match the real kube-scheduler environment?

Nope, kube-scheduler always adopts the 0 resync period for a while:

kubernetes/cmd/kube-scheduler/app/options/options.go

Line 289 in a439bc5

c.InformerFactory = scheduler.NewInformerFactory(client, 0)

It's just the integration tests that were written in a "1-second-resync" style.

@liggitt
Copy link
Member

liggitt commented Nov 9, 2020

ok, thanks

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2020
@Huang-Wei Huang-Wei mentioned this pull request Nov 11, 2020
Closed
@Huang-Wei
Copy link
Member Author

/retest

@Huang-Wei
Copy link
Member Author

Huang-Wei commented Nov 11, 2020 •
edited

Hmm.. it seems this PR falls into a weird CI bug.

Updated:

The failure got worked around by rebasing master and force push again.

@Huang-Wei
Copy link
Member Author

/retest

@Huang-Wei
Copy link
Member Author

@alculquicondor @adtac comments have been addressed. PTAL.

@alculquicondor
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2020
@Huang-Wei
Copy link
Member Author

@liggitt would you mind /approve the changes in test/integration/util/util.go? In terms of scheduler, we don't need a non-zero syncPeriod option in integrate tests.

@Huang-Wei
Copy link
Member Author

Kindly ping @liggitt for /approve.

@liggitt
Copy link
Member

liggitt commented Nov 16, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, hex108, Huang-Wei, liggitt

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 Nov 16, 2020
@Huang-Wei
Copy link
Member Author

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit 147a120 into kubernetes:master Nov 16, 2020
@Huang-Wei Huang-Wei deleted the 0-informer-sync-period branch November 16, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hex108 hex108 hex108 approved these changes

@alculquicondor alculquicondor alculquicondor left review comments

@adtac adtac adtac left review comments

@keerthigd keerthigd keerthigd left review comments

@ahg-g ahg-g Awaiting requested review from ahg-g

@resouer resouer Awaiting requested review from resouer

Assignees

@hex108 hex108

@alculquicondor alculquicondor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Huang-Wei @liggitt @fejta-bot @hex108 @alculquicondor @k8s-ci-robot @adtac @keerthigd