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

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

Merged
merged 1 commit into from
Oct 11, 2018
Merged

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

merged 1 commit into from
Oct 11, 2018

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
@@ -76,3 +79,114 @@ func createImageExistenceMap(nodes []*v1.Node) map[string]sets.String {
}
return imageExistenceMap
}

// ProtocolPort represents a protocol port pair, e.g. tcp:80.
type ProtocolPort struct {
Copy link

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

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
@@ -0,0 +1,132 @@
/*
Copy link
Member

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

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
Member

@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
@@ -21,6 +21,9 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
)

// DefaultBindAllHostIP defines the default ip address used to bind to all host.
Copy link
Member

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

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
Member

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
Member

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

[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

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

@misterikkit misterikkit misterikkit left review comments

@bsalamat bsalamat bsalamat left review comments

@k82cn k82cn k82cn left review comments

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