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

Add "only_cpu_and_memory" GET parameter to /stats/summary http handler in kubele #67829

Merged
merged 1 commit into from Sep 26, 2018

Conversation

@krzysztof-jastrzebski
Contributor

krzysztof-jastrzebski commented Aug 24, 2018

What this PR does / why we need it:
PR adds "only_cpu_and_memory" GET parameter to /stats/summary http handler in kubelet. If parameter is true then only cpu and memory will be present in response. The parameter will be used by Metric Server to avoid sending/decoding unneeded data. Metric server needs only cpu and memory.

Release note:

Add "only_cpu_and_memory" GET parameter to /stats/summary http handler in kubelet. If parameter is true then only cpu and memory will be present in response.
@dims

This comment has been minimized.

Show comment
Hide comment
@dims

dims Aug 24, 2018

Member

@mwielgus this does not fall under sig node?

Member

dims commented Aug 24, 2018

@mwielgus this does not fall under sig node?

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 25, 2018

Member

^ i have the same question. ;-)

@krzysztof-jastrzebski please add a release note explaining your change.
https://github.com/kubernetes/kubernetes/blob/master/.github/PULL_REQUEST_TEMPLATE.md

Member

neolit123 commented Aug 25, 2018

^ i have the same question. ;-)

@krzysztof-jastrzebski please add a release note explaining your change.
https://github.com/kubernetes/kubernetes/blob/master/.github/PULL_REQUEST_TEMPLATE.md

@krzysztof-jastrzebski krzysztof-jastrzebski changed the title from Add /stats/cpuandmemory http handller to kubelet. to Add "only_cpu_and_memory" GET parameter to /stats/summary http handler in kubele Aug 27, 2018

@dashpole

This comment has been minimized.

Show comment
Hide comment
@dashpole

dashpole Aug 27, 2018

Contributor

@krzysztof-jastrzebski This seems to be a way to implement core metrics in the kubelet. Back when I proposed the set of core metrics, sig-instrumentation wasn't ready to consume scoped-down metrics in heapster (which has now become the metrics server), and thus we punted on adding the new endpoint. IMO, if the metrics-server is in a position to consume core metrics, we should define the core metrics endpoint, rather than making this change. That way we can also begin the process of removing dependencies on the summary api.

Contributor

dashpole commented Aug 27, 2018

@krzysztof-jastrzebski This seems to be a way to implement core metrics in the kubelet. Back when I proposed the set of core metrics, sig-instrumentation wasn't ready to consume scoped-down metrics in heapster (which has now become the metrics server), and thus we punted on adding the new endpoint. IMO, if the metrics-server is in a position to consume core metrics, we should define the core metrics endpoint, rather than making this change. That way we can also begin the process of removing dependencies on the summary api.

@mwielgus mwielgus added this to the v1.12 milestone Aug 28, 2018

@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Aug 29, 2018

Contributor

Using new endpoint requires migrating all nodes in the cluster to the version with new endpoint. As we support 3 versions it can't be delivered before version 1.15. This could be a temporary solution before final solution is delivered.

Contributor

krzysztof-jastrzebski commented Aug 29, 2018

Using new endpoint requires migrating all nodes in the cluster to the version with new endpoint. As we support 3 versions it can't be delivered before version 1.15. This could be a temporary solution before final solution is delivered.

@dashpole

This comment has been minimized.

Show comment
Hide comment
@dashpole

dashpole Aug 29, 2018

Contributor

/ok-to-test

Contributor

dashpole commented Aug 29, 2018

/ok-to-test

@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Aug 29, 2018

Contributor

/test pull-kubernetes-e2e-kops-aws

Contributor

krzysztof-jastrzebski commented Aug 29, 2018

/test pull-kubernetes-e2e-kops-aws

@dashpole

This comment has been minimized.

Show comment
Hide comment
@dashpole

dashpole Aug 30, 2018

Contributor

I think this makes sense as a temporary solution. We can hopefully start discussions on the core metrics API for the 1.13 timeframe

Contributor

dashpole commented Aug 30, 2018

I think this makes sense as a temporary solution. We can hopefully start discussions on the core metrics API for the 1.13 timeframe

@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Aug 31, 2018

Contributor

Is there anything I need to fix?

Contributor

krzysztof-jastrzebski commented Aug 31, 2018

Is there anything I need to fix?

@dashpole

This comment has been minimized.

Show comment
Hide comment
@dashpole

dashpole Aug 31, 2018

Contributor

/lgtm

Contributor

dashpole commented Aug 31, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm and removed lgtm labels Aug 31, 2018

@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 6, 2018

Contributor

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws

Contributor

krzysztof-jastrzebski commented Sep 6, 2018

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Sep 6, 2018

Add "only_cpu_and_memory" GET parameter to /stats/summary http handle…
…r in kubelet. If parameter is true then only cpu and memory will be present in response. The parameter will be used by Metric Server to avoid sending/decoding unneeded data.
@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 6, 2018

Contributor

/test pull-kubernetes-e2e-kops-aws

Contributor

krzysztof-jastrzebski commented Sep 6, 2018

/test pull-kubernetes-e2e-kops-aws

@dashpole dashpole referenced this pull request Sep 11, 2018

Open

Reduce the set of metrics exposed by the kubelet #68522

0 of 7 tasks complete
@@ -87,3 +89,32 @@ func (sp *summaryProviderImpl) Get(updateStats bool) (*statsapi.Summary, error)
}
return &summary, nil
}
func (sp *summaryProviderImpl) GetCPUAndMemoryStats() (*statsapi.Summary, error) {
summary, err := sp.Get(false)

This comment has been minimized.

@yujuhong

yujuhong Sep 14, 2018

Contributor

Will it be hard to actually bypass getting the imageFS (and other unused) stats? Setting them to nil after getting all the data seems sub-optimal...

@yujuhong

yujuhong Sep 14, 2018

Contributor

Will it be hard to actually bypass getting the imageFS (and other unused) stats? Setting them to nil after getting all the data seems sub-optimal...

This comment has been minimized.

@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 17, 2018

Contributor

Do you want me to bypass getting all unneeded stats or only some of them? Do you want me to add functions to StatsProvider that returns only needed stats?

@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 17, 2018

Contributor

Do you want me to bypass getting all unneeded stats or only some of them? Do you want me to add functions to StatsProvider that returns only needed stats?

This comment has been minimized.

@yujuhong

yujuhong Sep 18, 2018

Contributor

Do you want me to bypass getting all unneeded stats or only some of them?

Bypassing all unneeded if possible.

Do you want me to add functions to StatsProvider that returns only needed stats?

If that's needed, then yes :-)

@yujuhong

yujuhong Sep 18, 2018

Contributor

Do you want me to bypass getting all unneeded stats or only some of them?

Bypassing all unneeded if possible.

Do you want me to add functions to StatsProvider that returns only needed stats?

If that's needed, then yes :-)

This comment has been minimized.

@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 19, 2018

Contributor

I have final #68841 but it is much bigger than this one.
Could we submit this PR and later submit final version? I would like to push this PR to 1.12 patch release and #68841 will be released with 1.13. It will be easier to release smaller PR in patch release.

@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 19, 2018

Contributor

I have final #68841 but it is much bigger than this one.
Could we submit this PR and later submit final version? I would like to push this PR to 1.12 patch release and #68841 will be released with 1.13. It will be easier to release smaller PR in patch release.

@guineveresaenger

This comment has been minimized.

Show comment
Hide comment
@guineveresaenger

guineveresaenger Sep 18, 2018

Contributor

As we are close to release, please continue this work in 1.13!
/milestone v1.13

Contributor

guineveresaenger commented Sep 18, 2018

As we are close to release, please continue this work in 1.13!
/milestone v1.13

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.12, v1.13 Sep 18, 2018

@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Sep 25, 2018

Contributor

/approve

Contributor

yujuhong commented Sep 25, 2018

/approve

@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Sep 26, 2018

Contributor

/lgtm

based on #67829 (comment)

Contributor

yujuhong commented Sep 26, 2018

/lgtm

based on #67829 (comment)

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 26, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 26, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, krzysztof-jastrzebski, yujuhong

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

Contributor

k8s-ci-robot commented Sep 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, krzysztof-jastrzebski, yujuhong

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

@krzysztof-jastrzebski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-jastrzebski

krzysztof-jastrzebski Sep 26, 2018

Contributor

/test pull-kubernetes-e2e-gce-100-performance

Contributor

krzysztof-jastrzebski commented Sep 26, 2018

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot k8s-ci-robot merged commit f263993 into kubernetes:master Sep 26, 2018

17 of 19 checks passed

Submit Queue PR does not have lgtm label.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
cla/linuxfoundation krzysztof-jastrzebski authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment