scheduler: remove direct import to pkg/master/ports #90000
Conversation
There was a problem hiding this comment.
Thanks, this is exactly the kind of cleanup we're looking for, but I think it can go a little further.
Looking at usages of the ports.InsecureSchedulerPort const, there are only 3 places it's used (outside of the scheduler where you've already updated it):
Can we update these places to use the new const location from within the scheduler, and remove it from pkg/master/ports?
| // May be overridden by a flag at startup. | ||
| // Copied from pkg/master/ports/ports.go | ||
| // TODO: Import this constant from a consts only package, that does not pull any further dependencies. | ||
| KubeSchedulerPort = 10259 |
There was a problem hiding this comment.
Adding to #90000 (review), this is the only location that KubeSchedulerPort is used, so it could be removed from pkg/master/ports. Though I think this may be better declared in one of the APIs than cmd/kube-scheduler, but I'll let someone else comment on that. @Huang-Wei wdyt?
There was a problem hiding this comment.
IMO we should move this along with InsecureSchedulerPort to pkg/scheduler/apis/config/types.go.
| // Copied from pkg/master/ports/ports.go | ||
| // TODO: Import this constant from a consts only package, that does not pull any further dependencies. | ||
| // Deprecated: use the secure KubeSchedulerPort instead. | ||
| InsecureSchedulerPort = 10251 |
There was a problem hiding this comment.
Maybe better to move this to pkg/scheduler/apis/config/types.go.
6948929 to
0f47f3c
Compare
|
/retest @liggitt To not duplicate defining |
pkg/scheduler/apis/config/types.go
Outdated
| // InsecureSchedulerPort is the default port for the scheduler status server. | ||
| // May be overridden by a flag at startup. | ||
| // Deprecated: use the secure KubeSchedulerPort instead. | ||
| InsecureSchedulerPort = 10251 |
There was a problem hiding this comment.
| InsecureSchedulerPort = 10251 | |
| DefaultInsecureSchedulerPort = 10251 |
pkg/scheduler/apis/config/types.go
Outdated
| // Deprecated: use the secure KubeSchedulerPort instead. | ||
| InsecureSchedulerPort = 10251 | ||
|
|
||
| // KubeSchedulerPort is the default port for the scheduler status server. |
There was a problem hiding this comment.
| // KubeSchedulerPort is the default port for the scheduler status server. | |
| // DefaultKubeSchedulerPort is the default port for the scheduler status server. |
| @@ -340,7 +341,7 @@ type componentStatusStorage struct { | |||
| func (s componentStatusStorage) serversToValidate() map[string]*componentstatus.Server { | |||
| serversToValidate := map[string]*componentstatus.Server{ | |||
| "controller-manager": {Addr: "127.0.0.1", Port: ports.InsecureKubeControllerManagerPort, Path: "/healthz"}, | |||
| "scheduler": {Addr: "127.0.0.1", Port: ports.InsecureSchedulerPort, Path: "/healthz"}, | |||
| "scheduler": {Addr: "127.0.0.1", Port: kubeschedulerconfig.InsecureSchedulerPort, Path: "/healthz"}, | |||
There was a problem hiding this comment.
add a comment indicating this is fragile and assumes the default port is being used
|
a couple comments, lgtm otherwise |
Signed-off-by: SataQiu <1527062125@qq.com>
0f47f3c to
41d3e44
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei, liggitt, SataQiu 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
scheduler: remove direct import to pkg/master/ports
Which issue(s) this PR fixes:
Ref #89930
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: