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

Graduate metrics/v1alpha1 to v1beta1 #51653

Merged
merged 3 commits into from Sep 6, 2017

Conversation

@DirectXMan12
Contributor

DirectXMan12 commented Aug 30, 2017

This introduces v1beta1 of the resource metrics API, previously in alpha.
The v1alpha1 version remains for compatibility with the Heapster legacy version
of the resource metrics API, which is compatible with the v1alpha1 version. It also
renames the v1beta1 version to resource-metrics.metrics.k8s.io.

The HPA controller's REST clients (but not the legacy client) have been migrated as well.

Part of kubernetes/features#118.

Migrate the metrics/v1alpha1 API to metrics/v1beta1.  The HorizontalPodAutoscaler
controller REST client now uses that version.  For v1beta1, the API is now known as
resource-metrics.metrics.k8s.io.
@DirectXMan12

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Aug 30, 2017

Contributor

Is there a feature issue to attach to this?

Contributor

smarterclayton commented Aug 30, 2017

Is there a feature issue to attach to this?

@DirectXMan12

This comment has been minimized.

Show comment
Hide comment
@DirectXMan12

DirectXMan12 Aug 31, 2017

Contributor

Whoops, yep, will do -- it's part of kubernetes/features#118 ("the master metrics API... needs to be stabilized").

Contributor

DirectXMan12 commented Aug 31, 2017

Whoops, yep, will do -- it's part of kubernetes/features#118 ("the master metrics API... needs to be stabilized").

@DirectXMan12

This comment has been minimized.

Show comment
Hide comment
@DirectXMan12

DirectXMan12 Aug 31, 2017

Contributor

/retest

Contributor

DirectXMan12 commented Aug 31, 2017

/retest

@DirectXMan12

This comment has been minimized.

Show comment
Hide comment
@DirectXMan12

DirectXMan12 Aug 31, 2017

Contributor

let's see if that actually kicks the tests off -- it's been sitting here for a day, not doing anything

Contributor

DirectXMan12 commented Aug 31, 2017

let's see if that actually kicks the tests off -- it's been sitting here for a day, not doing anything

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Aug 31, 2017

Contributor

So other than the name of the group I don't see any issues here with API. Will need sig approval and LGTM though.

Contributor

smarterclayton commented Aug 31, 2017

So other than the name of the group I don't see any issues here with API. Will need sig approval and LGTM though.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Sep 1, 2017

Contributor

Yeah those groups sound fine to me.

Contributor

smarterclayton commented Sep 1, 2017

Yeah those groups sound fine to me.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Sep 1, 2017

Contributor

We can get a release exception if that goes past code freeze since this would be new APIs, so if we generally agree on the shorter names we should do that for 1.8

Contributor

smarterclayton commented Sep 1, 2017

We can get a release exception if that goes past code freeze since this would be new APIs, so if we generally agree on the shorter names we should do that for 1.8

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 1, 2017

Member

@smarterclayton which group names sound fine for you?

Member

piosz commented Sep 1, 2017

@smarterclayton which group names sound fine for you?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Sep 1, 2017

Contributor

metrics.k8s.io for resource metrics
custom.metrics.k8s.io for custom metrics?

SGTM

Contributor

smarterclayton commented Sep 1, 2017

metrics.k8s.io for resource metrics
custom.metrics.k8s.io for custom metrics?

SGTM

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz
Member

piosz commented Sep 1, 2017

@piosz piosz self-assigned this Sep 1, 2017

Show outdated Hide outdated pkg/generated/openapi/BUILD Outdated
@@ -98,6 +98,7 @@ func New() *Generator {
`k8s.io/api/networking/v1`,
`k8s.io/kubernetes/federation/apis/federation/v1beta1`,
`k8s.io/metrics/pkg/apis/metrics/v1alpha1`,

This comment has been minimized.

@piosz

piosz Sep 1, 2017

Member

same here

@piosz

piosz Sep 1, 2017

Member

same here

@@ -36,7 +36,7 @@ PREFIX=k8s.io/metrics/pkg/apis
INPUT_BASE="--input-base ${PREFIX}"
CLIENTSET_PATH="--clientset-path k8s.io/metrics/pkg/client/clientset_generated"
${CLIENTGEN} --clientset-name="clientset" ${INPUT_BASE} --input metrics/v1alpha1 ${CLIENTSET_PATH} --output-base ${SCRIPT_BASE}
${CLIENTGEN} --clientset-name="clientset" ${INPUT_BASE} --input metrics/v1alpha1 --input metrics/v1beta1 ${CLIENTSET_PATH} --output-base ${SCRIPT_BASE}

This comment has been minimized.

@piosz

piosz Sep 1, 2017

Member

remove metrics/v1alpha1?

@piosz

piosz Sep 1, 2017

Member

remove metrics/v1alpha1?

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 1, 2017

Member

Let's go with metrics.k8s.io and custom.metrics.k8s.io then. Otherwise LGTM. Ping me once you update the code, so I'll apply the label.

FYI, we have to preserve v1alpha1 until we remove the legacy metrics client from the HPA

Oh I see. Please disregard my comments about v1alpha1.

Member

piosz commented Sep 1, 2017

Let's go with metrics.k8s.io and custom.metrics.k8s.io then. Otherwise LGTM. Ping me once you update the code, so I'll apply the label.

FYI, we have to preserve v1alpha1 until we remove the legacy metrics client from the HPA

Oh I see. Please disregard my comments about v1alpha1.

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 1, 2017

Member

I still can see modified pkg/apis/batch/v1beta1/zz_generated.conversion.go

Member

piosz commented Sep 1, 2017

I still can see modified pkg/apis/batch/v1beta1/zz_generated.conversion.go

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Sep 1, 2017

Contributor

/approve

Names custom.metrics.k8s.io and metrics.k8s.io. The other APIs you are promoting look fine.

Contributor

smarterclayton commented Sep 1, 2017

/approve

Names custom.metrics.k8s.io and metrics.k8s.io. The other APIs you are promoting look fine.

@sjenning

This comment has been minimized.

Show comment
Hide comment
@sjenning

sjenning Sep 2, 2017

Contributor

/retest

Contributor

sjenning commented Sep 2, 2017

/retest

@sjenning

This comment has been minimized.

Show comment
Hide comment
@sjenning

sjenning Sep 2, 2017

Contributor

Wait... when did kops drop off the required list?! I'm not complaining; it was blocking the world. Just didn't notice before I started the retest.

Contributor

sjenning commented Sep 2, 2017

Wait... when did kops drop off the required list?! I'm not complaining; it was blocking the world. Just didn't notice before I started the retest.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Sep 2, 2017

Member

It is non-blocking while they work out why it wasn't running tests using the test code from the PR being tested

Member

liggitt commented Sep 2, 2017

It is non-blocking while they work out why it wasn't running tests using the test code from the PR being tested

k8s-merge-robot added a commit that referenced this pull request Sep 4, 2017

Merge pull request #51792 from piosz/metrics-server
Automatic merge from submit-queue (batch tested with PRs 49727, 51792)

Introducing metrics-server

ref kubernetes/features#271

There is still some work blocked on problems with repo synchronization:
- migrate to `v1beta1` introduced in #51653 
- bump deps to HEAD
Will do it in a follow up PRs once the issue is resolved.

```release-note
Introduced Metrics Server
```

DirectXMan12 added some commits Aug 30, 2017

Graduate metrics/v1alpha1 to v1beta1
This commit graduates them resource metrics API from v1alpha1
to v1beta1.
Update HPA REST metrics client to metrics/v1beta1
This commit updates the REST metrics client to use metrics/v1beta1.
The legacy client still uses metrics/v1alpha1.
Rename metrics to metrics.k8s.io
This commit renames metrics to metrics.k8s.io
for the v1beta1 version, to give it a properly namespaced name which
mirrors custom.metrics.k8s.io.
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 5, 2017

Contributor

@DirectXMan12: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws e1a22e8 link /test pull-kubernetes-e2e-kops-aws

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.

Contributor

k8s-ci-robot commented Sep 5, 2017

@DirectXMan12: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws e1a22e8 link /test pull-kubernetes-e2e-kops-aws

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.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Sep 5, 2017

Contributor

@piosz is there an active release exception covering this yet?

This is API approved

Contributor

smarterclayton commented Sep 5, 2017

@piosz is there an active release exception covering this yet?

This is API approved

@piosz piosz added this to the v1.8 milestone Sep 6, 2017

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 6, 2017

Member

@smarterclayton I don't know how the exception process works but my feeling is that it was reviewed and approved before the code freeze.

Member

piosz commented Sep 6, 2017

@smarterclayton I don't know how the exception process works but my feeling is that it was reviewed and approved before the code freeze.

@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 6, 2017

Member

/lgtm

Member

piosz commented Sep 6, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 6, 2017

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Sep 6, 2017

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, piosz, smarterclayton

Associated issue: 118

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

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

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

Contributor

k8s-merge-robot commented Sep 6, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, piosz, smarterclayton

Associated issue: 118

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

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

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Sep 6, 2017

Contributor

Automatic merge from submit-queue (batch tested with PRs 51603, 51653)

Contributor

k8s-merge-robot commented Sep 6, 2017

Automatic merge from submit-queue (batch tested with PRs 51603, 51653)

@k8s-merge-robot k8s-merge-robot merged commit 0076f02 into kubernetes:master Sep 6, 2017

12 of 13 checks passed

pull-kubernetes-e2e-kops-aws Jenkins job failed.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation DirectXMan12 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-e2e-gce-bazel Job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-gce-gpu Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
@piosz

This comment has been minimized.

Show comment
Hide comment
@piosz

piosz Sep 6, 2017

Member

Ups. I unintentionally triggered merge. We can consider reverting this if people disagree with my #51653 (comment).

Member

piosz commented Sep 6, 2017

Ups. I unintentionally triggered merge. We can consider reverting this if people disagree with my #51653 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment