Skip to content

[scheduler cleanup phase 1]: Move NodeInfo utils into pkg/scheduler/cache #69495

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
Merged
k8s-ci-robot merged 1 commit into from
Oct 11, 2018
Merged

[scheduler cleanup phase 1]: Move NodeInfo utils into pkg/scheduler/cache #69495

k8s-ci-robot merged 1 commit into from
Oct 11, 2018
+393 −356

Conversation

@wgliang
Copy link
Contributor

@wgliang wgliang commented Oct 6, 2018 •
edited by k82cn
Loading

What this PR does / why we need it:
Move NodeInfo utils into pkg/scheduler/cache

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 #68958

Special notes for your reviewer:
/sig scheduler
/ping @misterikkit @k82cn @bsalamat

Release note:

Move NodeInfo utils into pkg/scheduler/cache.

@k8s-ci-robot k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 6, 2018
@wgliang wgliang force-pushed the feature/movenodoinfofunctions branch 3 times, most recently from 8191248 to a1aff3f Compare October 6, 2018 23:03
@wgliang
Copy link
Contributor Author

wgliang commented Oct 7, 2018

/retest

1 similar comment
@wgliang
Copy link
Contributor Author

wgliang commented Oct 7, 2018

/retest

@wgliang
Copy link
Contributor Author

wgliang commented Oct 7, 2018

/ping @misterikkit @k82cn @bsalamat

misterikkit
misterikkit reviewed Oct 8, 2018
View reviewed changes
pkg/scheduler/cache/util.go Outdated
Copy link

@misterikkit misterikkit Oct 8, 2018

Choose a reason for hiding this comment

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

rather than adding this to utils.go, what do you think about creating a file like ports.go (and ports_test.go)?

Copy link
Contributor Author

@wgliang wgliang Oct 8, 2018

Choose a reason for hiding this comment

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

DONE.

@wgliang wgliang force-pushed the feature/movenodoinfofunctions branch 2 times, most recently from ad9ad0c to be6a825 Compare October 8, 2018 23:52
misterikkit
misterikkit reviewed Oct 10, 2018
View reviewed changes
Copy link

@misterikkit misterikkit left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2018
bsalamat
bsalamat reviewed Oct 10, 2018
View reviewed changes
pkg/scheduler/cache/ports.go Outdated
Copy link
Member

@bsalamat bsalamat Oct 10, 2018

Choose a reason for hiding this comment

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

Isn't it better to call this file host_ports.go to distinguish it from container ports?

Copy link
Contributor Author

@wgliang wgliang Oct 10, 2018

Choose a reason for hiding this comment

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

But there also has ProtocolPort code, so ports.go maybe more accurate.

Copy link
Contributor

@k82cn k82cn Oct 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

+1 to host_ports, we only care about host port in scheduler including its protocol.

k82cn
k82cn reviewed Oct 11, 2018
View reviewed changes
pkg/scheduler/cache/util.go Outdated
Copy link
Contributor

@k82cn k82cn Oct 11, 2018

Choose a reason for hiding this comment

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

move this const to ports.go

Copy link
Contributor Author

@wgliang wgliang Oct 11, 2018

Choose a reason for hiding this comment

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

Ok, I'm fine with that.

@k82cn k82cn changed the title [scheduler cleanup phase 1]: Move NodeInfo utils into pkg/scheduler/c… [scheduler cleanup phase 1]: Move NodeInfo utils into pkg/scheduler/cache Oct 11, 2018
@k82cn
Copy link
Contributor

k82cn commented Oct 11, 2018

LGTM overall

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 11, 2018
@wgliang wgliang force-pushed the feature/movenodoinfofunctions branch from be6a825 to 0f924f3 Compare October 11, 2018 02:51
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2018
@wgliang wgliang force-pushed the feature/movenodoinfofunctions branch from 0f924f3 to a50404d Compare October 11, 2018 03:04
@wgliang
Copy link
Contributor Author

wgliang commented Oct 11, 2018

/ping @k82cn
All done.

@k82cn
Copy link
Contributor

k82cn commented Oct 11, 2018

/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 Oct 11, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, wgliang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Oct 11, 2018
@k8s-ci-robot k8s-ci-robot merged commit 539bdbc into kubernetes:master Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@misterikkit misterikkit misterikkit left review comments

@bsalamat bsalamat bsalamat left review comments

@k82cn k82cn k82cn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@misterikkit misterikkit

@k82cn k82cn

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/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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[scheduler cleanup phase 1]: Move NodeInfo utils into pkg/scheduler/cache

5 participants

@wgliang @k82cn @k8s-ci-robot @misterikkit @bsalamat