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

[RFC] CRI: Add daemon configuration to StatusResponse as extra information #2589

Merged
merged 1 commit into from
Dec 20, 2018

Conversation

zjumoon01
Copy link
Contributor

StatusResponse in CRI returns runtime extra information which is in
the map of Info. Now there is only one key in this map. This patch
add "daemon-config" as another key and its value is the daemon
configuration in json format. With this patch, for example, we can
get pouch root dir from StatusResponse.

Signed-off-by: Wang Rui baijia.wr@antfin.com

Ⅰ. Describe what this PR did

User could get pouch root dir by StatusResponse in CRI with this patch.
This patch add a new key "daemon-config" to StatusResponse.Info . A lot of extra information can be found in info["daemon-config"].

Ⅱ. Does this pull request fix one issue?

It fix the proposal #2568

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

StatusResponse in CRI returns runtime extra information which is in
the map of Info. Now there is only one key in this map. This patch
add "daemon-config" as another key and its value is the daemon
configuration in json format. With this patch, for example, we can
get pouch root dir from StatusResponse.

Signed-off-by: Wang Rui <baijia.wr@antfin.com>
@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #2589 into master will decrease coverage by 0.14%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2589      +/-   ##
==========================================
- Coverage   69.18%   69.04%   -0.15%     
==========================================
  Files         278      278              
  Lines       18689    18694       +5     
==========================================
- Hits        12930    12907      -23     
- Misses       4285     4305      +20     
- Partials     1474     1482       +8
Flag Coverage Δ
#criv1alpha1test 31.39% <0%> (+0.16%) ⬆️
#criv1alpha2test 35.7% <20%> (-0.09%) ⬇️
#integrationtest 40.64% <0%> (-0.12%) ⬇️
#nodee2etest 32.65% <20%> (-0.11%) ⬇️
#unittest 26.73% <0%> (+0.06%) ⬆️
Impacted Files Coverage Δ
cri/v1alpha2/cri.go 68.81% <20%> (-0.81%) ⬇️
daemon/mgr/system.go 67.93% <0%> (-5.35%) ⬇️
ctrd/watch.go 80.28% <0%> (-4.23%) ⬇️
daemon/mgr/events.go 96.29% <0%> (-3.71%) ⬇️
cri/v1alpha2/cri_wrapper.go 63.2% <0%> (-2.4%) ⬇️
ctrd/client.go 68.45% <0%> (-2.02%) ⬇️
ctrd/container.go 58.1% <0%> (-0.8%) ⬇️
daemon/mgr/container_utils.go 83.33% <0%> (-0.6%) ⬇️
daemon/mgr/container.go 58.79% <0%> (-0.22%) ⬇️
... and 2 more

@allencloud
Copy link
Collaborator

Just at my first glance of this, I am wondering if we need any test to cover this part ? @zjumoon01 @zhuangqh @starnop

@zhuangqh
Copy link
Contributor

have a deep discussion of this proposal with @zjumoon01 offline.
LGTM

@zhuangqh
Copy link
Contributor

Just at my first glance of this, I am wondering if we need any test to cover this part ? @zjumoon01 @zhuangqh @starnop

we could update the runtime info testcase here https://github.com/alibaba/cri-tools/blob/release-1.12/pkg/validate/runtime_info.go#L40

@zjumoon01
Copy link
Contributor Author

Just at my first glance of this, I am wondering if we need any test to cover this part ? @zjumoon01 @zhuangqh @starnop

At my second glance, I didn't see where the test case for Status CRI is. Do you mean add test in alibaba/cri-tools project ?

@starnop
Copy link
Contributor

starnop commented Dec 20, 2018

LGTM.

@allencloud
Copy link
Collaborator

At my second glance, I didn't see where the test case for Status CRI is. Do you mean add test in alibaba/cri-tools project ?

Just to check, We can do this in follow-up if could be. also cc @starnop @zhuangqh

@allencloud allencloud merged commit 48e0733 into AliyunContainerService:master Dec 20, 2018
@zjumoon01 zjumoon01 deleted the info branch December 21, 2018 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants