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.

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

[scheduler cleanup phase 2]: Rename pkg/scheduler/util to pkg/scheduler/internal/podinfo #71863

Closed
misterikkit opened this issue Dec 7, 2018 · 26 comments · Fixed by #82465 or #83233
Closed
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@misterikkit
Copy link

PLEASE COORDINATE WITH SIG-SCHEDULING BEFORE WORKING ON THIS ISSUE. We want to avoid the situation where two people accidentally start work on the same fix.

Main tracking issue: #68951

If you contributed in phase 1, please give others a chance before claiming this issue. You can also help out by reviewing the phase 2 PRs.

This package contains only functions related to pods. Having “util” as a name is generally discouraged, since it doesn’t tell readers anything about what is in that package. This change will also mirror the change to create pkg/scheduler/nodeinfo which will have a bunch of node-related types and functions.

GetPodPriority from this package is used by pkg/kubelet/eviction. In order to make our package internal, we should do one of the following. (1 is my preference)

  1. Copy the function into kubelet and remove the dependency
  2. Move the function into a different area like apimachinery or client-go
  3. Make the funcion an official part of the scheduler’s public go libraries

/sig scheduling
/kind cleanup
/help
/good-first-issue

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Dec 7, 2018
@misterikkit misterikkit mentioned this issue Dec 7, 2018
25 tasks
@mysunshine92
Copy link
Contributor

I will work on it

@mysunshine92
Copy link
Contributor

/remove-help

@k8s-ci-robot k8s-ci-robot removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Dec 8, 2018
@misterikkit
Copy link
Author

/assign @mysunshine92

@k8s-ci-robot
Copy link
Contributor

@misterikkit: GitHub didn't allow me to assign the following users: mysunshine92.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @mysunshine92

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@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 the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 10, 2019
@shinytang6
Copy link
Contributor

Hey @misterikkit @mysunshine92 , what's the status of this issue? Can l take this up as a beginning contribution to k8s?

@hprateek43
Copy link
Contributor

hprateek43 commented Apr 19, 2019

@misterikkit I am picking up this issue. I almost have a patch ready and will be opening a PR shortly. Do let me know if someone else is working on this.

@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 May 19, 2019
@misterikkit
Copy link
Author

/remove-lifecycle rotten
The PR for this is nearly merged.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 22, 2019
@alculquicondor
Copy link
Member

I understand that no one is working on this now?

@hprateek43
Copy link
Contributor

I understand that no one is working on this now?

I have the PR almost merged.

@alculquicondor
Copy link
Member

@hprateek43 wasn't there more work pending than just that PR?

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Sep 25, 2019
@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Reopened this issue.

In response to this:

@hprateek43 wasn't there more work pending than just that PR?

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@hprateek43
Copy link
Contributor

@alculquicondor I am working on structuring the other PRs corresponding to this

@alculquicondor
Copy link
Member

Thanks @hprateek43. Just remember to not put the word "Fixes" in your PR description unless it's the last PR.

@alculquicondor
Copy link
Member

/reopen

Please @hprateek43, remember to not use the word "Fixes" in your PRs if they are not the last one.

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Reopened this issue.

In response to this:

/reopen

Please @hprateek43, remember to not use the word "Fixes" in your PRs if they are not the last one.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Oct 7, 2019
@alculquicondor
Copy link
Member

Hi @hprateek43, could you please do a survey of the remaining structs and functions in the util package and their usages?

From there, we can decide what is the best location for each of them.

@hprateek43
Copy link
Contributor

@alculquicondor I will review the scheduler today and identify redundant components. Will open a cleanup PR

@hprateek43
Copy link
Contributor

@alculquicondor I reviewed the scheduler and think we can safely move error_channel.go and its test cases into an internal package. I just made this change on a local branch and seems to work. Will open a issue for it and send the PR for merge.

@hprateek43
Copy link
Contributor

@alculquicondor I am opening a new issue around channels that links to this one, the PR will fix that instead of this umbrella issue

@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 the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2020
@haosdent
Copy link
Member

@ahg-g do we still need this?

@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 Feb 22, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
8 participants