Moved qos to api.helpers. #44906

k82cn · 2017-04-25T06:56:49Z

What this PR does / why we need it:
The GetPodQoS is also used by other components, e.g. kube-scheduler and it's not bound to kubelet; moved it to api helpers so client-go.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #N/A

Release note:

k8s-reviewable · 2017-04-25T06:56:56Z

This change is

k82cn · 2017-04-25T07:19:01Z

xref: #44851

k82cn · 2017-04-25T08:20:35Z

@k8s-bot unit test this

k82cn · 2017-04-26T02:09:20Z

ping :).

k82cn · 2017-04-27T00:19:57Z

ping @jayunit100 / @dchen1107 :).

k82cn · 2017-04-27T01:35:51Z

/cc @lavalamp , @deads2k , @caesarxuchao

Another example that, the util func GetPodQOS is used by several components. I moved it to api.helper; as it's more reasonable to be global helper func instead of kubelet's helper.

k82cn · 2017-04-28T06:47:26Z

ping for this PR :). I'd like to move it to api.helper firstly, and then we can handler it with other help funcs together.

deads2k · 2017-04-28T13:12:56Z

pkg/api/v1/helper/helpers_test.go

@@ -499,3 +500,155 @@ func TestGetAffinityFromPodAnnotations(t *testing.T) {
 		}
 	}
 }
+
+func TestGetPodQOS(t *testing.T) {


I'd like to have a test (I think this one), that does a conversion to internal and runs the internal version of the function to make sure that it agrees with the external version of the function.

sure; I'll add a test for internal one.

sure; I'll add a test for internal one.

The key piece bit is that its the same test, not a copy/paste. That way a change in one will cause the test fail. A separate test won't achieve the same goal.

deads2k · 2017-04-28T13:13:20Z

I'd like a test to make sure that the implementations stay in sync, but after that, this lgtm.

k82cn · 2017-04-30T02:40:18Z

@deads2k , added a conversion within one test case.

deads2k · 2017-05-01T15:05:39Z

/lgtm

caesarxuchao · 2017-05-01T16:50:14Z

@k82cn are you going to use the helper functions in client-go/pkg/api? They are going to disappear after we move the external types to k8s.io/api.

caesarxuchao · 2017-05-01T16:51:41Z

Can you add a deprecation notice in client-go saying that pkg/api is going to disappear?

k82cn · 2017-05-02T00:49:04Z

@caesarxuchao , this PRs just focus on moving common funcs to helpers. I'd like to follow the way how k8s.io/api handle api.helpers, including this GetPodQOS func.

caesarxuchao · 2017-05-02T03:12:24Z

@k82cn k8s/api only have external types (i.e., excluding pkg/api/types.go), so it won't include any helper functions that in pkg/api/.

Also, client-go will import types from k8s/api and stop copying the entire pkg/api and pkg/apis folder, so client-go won't have the helper functions either.

k82cn · 2017-05-02T03:21:16Z

@caesarxuchao , for where to place those helper funcs, I'd suggest to continue the discuss at #44851

In this PRs, it try to avoid introducing package dependency between kubelet & others; after moving to api.helper, we just need to discuss how to handle api.helpers.

k82cn · 2017-05-16T04:32:43Z

ping @caesarxuchao for lgtm :).

caesarxuchao · 2017-05-18T00:34:51Z

staging/src/k8s.io/client-go/pkg/api/helper/qos/BUILD

+go_library(
+    name = "go_default_library",
+    srcs = ["qos.go"],
+    tags = ["automanaged"],


Can you add the visibility rule like this: #46006

caesarxuchao · 2017-05-18T00:35:09Z

staging/src/k8s.io/client-go/pkg/api/install/BUILD

@@ -11,7 +11,6 @@ go_library(
    name = "go_default_library",
    srcs = ["install.go"],
    tags = ["automanaged"],
-    visibility = ["//visibility:private"],


Why removing the visibility rule?

Is it a rebase issue? You removed all the rules.

caesarxuchao · 2017-05-18T00:36:38Z

staging/src/k8s.io/client-go/pkg/api/helper/BUILD

 go_library(
    name = "go_default_library",
    srcs = ["helpers.go"],
    tags = ["automanaged"],
-    visibility = ["//visibility:private"],


Please keep the visibility rule.

hm, just run update-bazel scripts :). let me check it.

caesarxuchao · 2017-05-18T00:39:24Z

pkg/api/v1/helper/qos/qos_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2016 The Kubernetes Authors.
+Copyright 2017 The Kubernetes Authors.


Please don't change the year.

ok, got it.

caesarxuchao · 2017-05-18T00:41:39Z

pkg/api/helper/helpers.go

@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

+// NOTE: DO NOT use those helper functions through client-go, the


We don't need to add the NOTE here now, we restrict the visibility of the package in the BUILD file. (sorry)

k82cn · 2017-05-19T03:25:17Z

@k8s-bot kops aws e2e test this

k82cn · 2017-05-21T13:23:23Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

caesarxuchao · 2017-05-22T17:49:17Z

/lgtm
/approve

k8s-github-robot · 2017-05-22T17:49:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, k82cn, lavalamp

Needs approval from an approver in each of these OWNERS Files:

~~pkg/api/OWNERS~~ [lavalamp]
~~pkg/kubelet/OWNERS~~ [lavalamp]
~~pkg/printers/OWNERS~~ [deads2k,lavalamp]
~~pkg/quota/OWNERS~~ [lavalamp]
~~pkg/registry/OWNERS~~ [deads2k,lavalamp]
~~plugin/pkg/scheduler/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/pkg/api/OWNERS~~ [caesarxuchao,deads2k,lavalamp]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-05-22T17:49:42Z

@k82cn Thanks for bearing with me :)

k8s-ci-robot · 2017-05-22T19:42:31Z

@k82cn: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`fd0190f`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-github-robot · 2017-05-22T20:30:06Z

Automatic merge from submit-queue

k82cn · 2017-05-23T00:49:48Z

my pleasure :).

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 25, 2017

k8s-github-robot assigned jayunit100 and dchen1107 Apr 25, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 25, 2017

caesarxuchao assigned lavalamp and caesarxuchao and unassigned jayunit100 and dchen1107 Apr 27, 2017

deads2k reviewed Apr 28, 2017

View reviewed changes

k82cn force-pushed the moved_qos_to_v1helper branch 2 times, most recently from 733ae2e to b51e41b Compare April 30, 2017 01:50

k82cn force-pushed the moved_qos_to_v1helper branch from b51e41b to d3281c3 Compare April 30, 2017 15:47

k8s-ci-robot assigned deads2k May 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2017

k82cn force-pushed the moved_qos_to_v1helper branch from d43cbf0 to e9c3643 Compare May 16, 2017 03:20

caesarxuchao reviewed May 18, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2017

k82cn force-pushed the moved_qos_to_v1helper branch from e9c3643 to aa7912a Compare May 19, 2017 02:18

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

Klaus Ma added 2 commits May 20, 2017 07:17

Moved qos to api.helpers.

83b7f77

generated client-go.

fd0190f

k82cn force-pushed the moved_qos_to_v1helper branch from fcf3a9f to fd0190f Compare May 20, 2017 11:59

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017

k8s-github-robot merged commit 91adb3e into kubernetes:master May 22, 2017

k82cn deleted the moved_qos_to_v1helper branch May 23, 2017 00:49

Moved qos to api.helpers. #44906

Moved qos to api.helpers. #44906

Conversation

k82cn commented Apr 25, 2017

k8s-reviewable commented Apr 25, 2017

k82cn commented Apr 25, 2017

k82cn commented Apr 25, 2017

k82cn commented Apr 26, 2017

k82cn commented Apr 27, 2017

k82cn commented Apr 27, 2017

k82cn commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Apr 28, 2017

k82cn commented Apr 30, 2017 • edited Loading

deads2k commented May 1, 2017

caesarxuchao commented May 1, 2017

caesarxuchao commented May 1, 2017

k82cn commented May 2, 2017

caesarxuchao commented May 2, 2017

k82cn commented May 2, 2017

k82cn commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented May 19, 2017

k82cn commented May 21, 2017

caesarxuchao commented May 22, 2017

k8s-github-robot commented May 22, 2017

caesarxuchao commented May 22, 2017

k8s-ci-robot commented May 22, 2017 • edited Loading

k8s-github-robot commented May 22, 2017

k82cn commented May 23, 2017

k82cn commented Apr 30, 2017 •

edited

Loading

caesarxuchao May 18, 2017 •

edited

Loading

k8s-ci-robot commented May 22, 2017 •

edited

Loading