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

feature: add stats api in daemon side #2112

Merged
merged 1 commit into from
Aug 22, 2018

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented Aug 17, 2018

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

This PR adds the stats API in daemon side. Here we defined the API:

  /containers/{id}/stats:
    get:
      summary: "Get container stats based on resource usage"
      produces: ["application/json"]
      responses:
        200:
          description: container stats
          schema:
            $ref: "#/definitions/ContainerStats"
        404:
          $ref: "#/responses/404ErrorResponse"
        500:
          $ref: "#/responses/500ErrorResponse"
      parameters:
        - name: "id"
          in: "path"
          required: true
          description: "ID or name of the container"
          type: "string"
        - name: "stream"
          in: "query"
          description: "Stream the output. If false, the stats will be output once and then it will disconnect."
          type: "boolean"
          default: true

Ⅱ. Does this pull request fix one issue?

this fixes the first part of issue #2086

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

let me add client side implementation and then add test case.

Ⅳ. Describe how to verify it

image

and we can also set the stream query as true to stream stats of a container.

Ⅴ. Special notes for reviews

none

@codecov-io
Copy link

codecov-io commented Aug 17, 2018

Codecov Report

Merging #2112 into master will increase coverage by 4.73%.
The diff coverage is 0.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
+ Coverage   59.63%   64.36%   +4.73%     
==========================================
  Files         208      209       +1     
  Lines       16482    16596     +114     
==========================================
+ Hits         9829    10682     +853     
+ Misses       5453     4584     -869     
- Partials     1200     1330     +130
Flag Coverage Δ
#criv1alpha1test 32.77% <0.78%> (-0.37%) ⬇️
#criv1alpha2test 33.55% <0.78%> (?)
#integrationtest 39.26% <0.78%> (-0.26%) ⬇️
#unittest 23.72% <0%> (-0.18%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container_types.go 79.54% <ø> (ø) ⬆️
daemon/mgr/container.go 56.44% <ø> (+0.57%) ⬆️
cri/v1alpha2/cri_utils.go 82.33% <0%> (+27.18%) ⬆️
cri/v1alpha1/cri_utils.go 83.64% <0%> (ø) ⬆️
apis/server/container_bridge.go 75.83% <0%> (-3.55%) ⬇️
ctrd/container.go 43.19% <0%> (-0.32%) ⬇️
daemon/mgr/container_stats.go 0% <0%> (ø)
apis/server/router.go 91.83% <100%> (+0.05%) ⬆️
cri/v1alpha1/cri.go 63.44% <0%> (-0.35%) ⬇️
... and 12 more

logrus.Infof("context is cancelled when streaming stats of container %s", c.ID)
return nil
default:
logrus.Debugf("Start to stream stats of container %s", c.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we don't have event listener for specific container right now, I think we should check the status of container before get stat. If the status has been changed from running to stopped, we should exit the loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the existing moby api and it doesn't exit and will wait for next restart.

ThrottledTime: metric.CPU.Throttling.ThrottledTime,
},
},
PrecpuStats: &types.CPUStats{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't support this right now, could we remove this part and add the TODO?

Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit adfc051 into AliyunContainerService:master Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants