feature: extend cri apis for remove volume #2124

starnop · 2018-08-20T08:56:17Z

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

extend cri apis for remove volume

PouchContainer CRI Manager provides an interface for removing volume.
PouchContainer CRI Manager supports querying the name of volume by containerstatus.

Ⅱ. Does this pull request fix one issue?

None.

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

Ⅳ. Describe how to verify it

use cri-tools that has been modified：starnop/cri-tools

# pouch volume ls
DRIVER   VOLUME NAME
local    687a243f9aafb0264b491dc1128b2c842b45f420894d36671332d30862c76bc7
local    c2ad62ff78afc55efe1d43649b9e77a780875bb348f2def8903ffefa3709f5b1
local    d8d84f8709e4e0093feb72c78eb6921f7dc1c02d92e121b520fb697add44a6af

# crictl rmv 687a243f9aafb0264b491dc1128b2c842b45f420894d36671332d30862c76bc7
DEBU[0000] RemoveVolumeRequest: &RemoveVolumeRequest{VolumeName:687a243f9aafb0264b491dc1128b2c842b45f420894d36671332d30862c76bc7,} 
DEBU[0000] RemoveVolumeResponse: &RemoveVolumeResponse{} 

# pouch volume ls
DRIVER   VOLUME NAME
local    d8d84f8709e4e0093feb72c78eb6921f7dc1c02d92e121b520fb697add44a6af
local    c2ad62ff78afc55efe1d43649b9e77a780875bb348f2def8903ffefa3709f5b1

Ⅴ. Special notes for reviews

codecov-io · 2018-08-20T09:43:20Z

Codecov Report

Merging #2124 into master will decrease coverage by 0.11%.
The diff coverage is 40.9%.

@@            Coverage Diff             @@
##           master    #2124      +/-   ##
==========================================
- Coverage   64.38%   64.27%   -0.12%     
==========================================
  Files         209      209              
  Lines       16631    16655      +24     
==========================================
- Hits        10708    10705       -3     
- Misses       4595     4614      +19     
- Partials     1328     1336       +8

Flag	Coverage Δ
#criv1alpha1test	`32.78% <9.09%> (-0.18%)`	⬇️
#criv1alpha2test	`33.39% <40.9%> (-0.18%)`	⬇️
#integrationtest	`39.21% <9.09%> (-0.04%)`	⬇️
#unittest	`23.87% <0%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
cri/v1alpha2/cri_wrapper.go	`52.08% <0%> (-1.8%)`	⬇️
cri/v1alpha2/service/cri.go	`85.71% <100%> (+1.09%)`	⬆️
daemon/daemon.go	`57.69% <100%> (ø)`	⬆️
cri/criservice.go	`64.7% <100%> (ø)`	⬆️
cri/v1alpha2/cri.go	`63.95% <37.5%> (-0.81%)`	⬇️
ctrd/watch.go	`72.72% <0%> (-7.58%)`	⬇️
cri/stream/httpstream/spdy/upgrade.go	`54.28% <0%> (-5.72%)`	⬇️
ctrd/container.go	`41.76% <0%> (-1.44%)`	⬇️
daemon/mgr/container.go	`56.03% <0%> (-0.42%)`	⬇️
cri/v1alpha1/cri.go	`63.3% <0%> (-0.4%)`	⬇️
... and 5 more

HusterWan · 2018-08-20T13:05:44Z

cri/apis/v1alpha2/api.proto

@@ -181,6 +187,8 @@ message Mount {
 	bool selinux_relabel = 4;
 	// Requested propagation mode.
 	MountPropagation propagation = 5;
+	// Name of volume
+	string name = 100;


do we need add more param in Mount?

Yeah, that's enough for the moment. :)

HusterWan · 2018-08-20T13:07:12Z

cri/apis/v1alpha2/api.proto

+}
+
+message RemoveVolumeResponse {}


Is your Editor problem? :)

rudyfly · 2018-08-22T03:31:33Z

hack/protoc.sh

@@ -39,7 +39,6 @@ generateproto(){
 }

 main(){
-    API_ROOT="${DIR}/cri/apis/v1alpha1" YEAR_TIME=" $(date '+%Y')" generateproto


Why delete it？

Now we don't need to extend the API of CRI v1alpha1. :)

If we do not the extensive CRI advanced feature, should we find somewhere to record this change? @starnop

https://github.com/alibaba/pouch/blob/master/docs/api/GRPC_API_CHANGELOG.md

rudyfly · 2018-08-22T03:35:22Z

Can you add test for CRI remove volume?

starnop · 2018-08-22T04:04:55Z

@rudyfly The test for function of RemoveVolume is not necessary, as for functional testing, I've given the steps for validation.

rudyfly · 2018-08-23T01:54:17Z

LGTM

allencloud · 2018-08-23T02:17:05Z

cri/apis/v1alpha2/api.proto

+	string volume_name = 1;
+}
+
+message RemoveVolumeResponse {
 }


Please add a blank line here to make it happy. @starnop

allencloud · 2018-08-23T02:21:17Z

cri/apis/v1alpha2/api.proto

@@ -181,6 +187,8 @@ message Mount {
 	bool selinux_relabel = 4;
 	// Requested propagation mode.
 	MountPropagation propagation = 5;
+	// Name of volume


TODO:
classify the anonymous volume or not.

allencloud · 2018-08-23T02:22:21Z

cri/v1alpha2/cri_wrapper.go

+	logrus.Infof("RemoveVolume %q", r.GetVolumeName())
+	defer func() {
+		if err != nil {
+			logrus.Errorf("RemoveVolume %q failed, error: %v", r.GetVolumeName(), err)


failed to do something

I'll submit a single PR to format logs.

Signed-off-by: Starnop <[email protected]>

pouchrobot added areas/storage kind/feature size/XXL labels Aug 20, 2018

starnop force-pushed the cri-extend-removeVolume branch from 22f8823 to b33168b Compare August 20, 2018 09:00

HusterWan reviewed Aug 20, 2018

View reviewed changes

cri/apis/v1alpha2/api.proto Outdated

}

message RemoveVolumeResponse {}

Copy link

Contributor

HusterWan Aug 20, 2018

Choose a reason for hiding this comment

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

Is your Editor problem? :)

starnop force-pushed the cri-extend-removeVolume branch from b33168b to b4c0678 Compare August 21, 2018 01:51

rudyfly reviewed Aug 22, 2018

View reviewed changes

pouchrobot added LGTM one maintainer or community participant agrees to merge the pull reuqest. conflict/needs-rebase and removed LGTM one maintainer or community participant agrees to merge the pull reuqest. labels Aug 23, 2018

allencloud reviewed Aug 23, 2018

View reviewed changes

starnop force-pushed the cri-extend-removeVolume branch from b4c0678 to 5a7d173 Compare August 23, 2018 02:49

pouchrobot added size/M and removed size/XXL conflict/needs-rebase labels Aug 23, 2018

starnop force-pushed the cri-extend-removeVolume branch 2 times, most recently from 912c373 to d7bca5c Compare August 23, 2018 03:22

pouchrobot added size/XXL and removed size/M labels Aug 23, 2018

starnop force-pushed the cri-extend-removeVolume branch from d7bca5c to 32f5cc9 Compare August 23, 2018 03:24

extend cri apis for remove volume

845cff8

Signed-off-by: Starnop <[email protected]>

starnop force-pushed the cri-extend-removeVolume branch from 32f5cc9 to 845cff8 Compare August 23, 2018 03:25

rudyfly merged commit dabdb55 into AliyunContainerService:master Aug 23, 2018

pouchrobot mentioned this pull request Aug 25, 2018

Weekly Report of PouchContainer from 2018-08-18 to 2018-08-25 #2160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: extend cri apis for remove volume #2124

feature: extend cri apis for remove volume #2124

starnop commented Aug 20, 2018 •

edited

Loading

codecov-io commented Aug 20, 2018 •

edited

Loading

HusterWan Aug 20, 2018

starnop Aug 21, 2018

HusterWan Aug 20, 2018

rudyfly Aug 22, 2018

starnop Aug 22, 2018

allencloud Aug 23, 2018

allencloud Aug 23, 2018

rudyfly commented Aug 22, 2018

starnop commented Aug 22, 2018 •

edited

Loading

rudyfly commented Aug 23, 2018

allencloud Aug 23, 2018

allencloud Aug 23, 2018

allencloud Aug 23, 2018

starnop Aug 23, 2018

feature: extend cri apis for remove volume #2124

feature: extend cri apis for remove volume #2124

Conversation

starnop commented Aug 20, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Aug 20, 2018 • edited Loading

Codecov Report

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

rudyfly commented Aug 22, 2018

starnop commented Aug 22, 2018 • edited Loading

rudyfly commented Aug 23, 2018

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

starnop commented Aug 20, 2018 •

edited

Loading

codecov-io commented Aug 20, 2018 •

edited

Loading

starnop commented Aug 22, 2018 •

edited

Loading