feature: UpdateContainerResources of CRI Manager #1511

starnop · 2018-06-12T03:23:06Z

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

add feature: UpdateContainerResources of CRI Manager

Ⅱ. Does this pull request fix one issue?

None

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

# pouch run -d -m 20m --name cri-update-test registry.hub.docker.com/library/nginx:latest
WARNING: Current Kernel does not support memory swap, discard --memory-swap 
bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5

# cat /sys/fs/cgroup/memory/default/bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5/memory.limit_in_bytes 
20971520

# crictl update --memory 31457280 bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5
DEBU[0000] UpdateContainerResourcesRequest: &UpdateContainerResourcesRequest{ContainerId:bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5,Linux:&LinuxContainerResources{CpuPeriod:0,CpuQuota:0,CpuShares:0,MemoryLimitInBytes:31457280,OomScoreAdj:0,CpusetCpus:,CpusetMems:,},} 
DEBU[0000] UpdateContainerResourcesResponse: &UpdateContainerResourcesResponse{} 
bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5

# cat /sys/fs/cgroup/memory/default/bd6c5b9de4874f6703961ed543a9056a9762279433c6cad0fd8b63f42f636ff5/memory.limit_in_bytes 
31457280

Ⅴ. Special notes for reviews

codecov-io · 2018-06-12T03:48:38Z

Codecov Report

Merging #1511 into master will increase coverage by 23.79%.
The diff coverage is 2.22%.

@@             Coverage Diff             @@
##           master    #1511       +/-   ##
===========================================
+ Coverage   16.88%   40.68%   +23.79%     
===========================================
  Files         211      254       +43     
  Lines       14020    16647     +2627     
===========================================
+ Hits         2367     6772     +4405     
+ Misses      11490     9006     -2484     
- Partials      163      869      +706

Impacted Files	Coverage Δ
daemon/mgr/container_state.go	`82.08% <0%> (+82.08%)`	⬆️
cri/v1alpha2/cri.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`48.8% <100%> (+48.8%)`	⬆️
storage/volume/driver/proxy.go	`53.42% <0%> (ø)`
storage/quota/grpquota.go	`0% <0%> (ø)`
main.go	`62.5% <0%> (ø)`
storage/volume/driver/remote.go	`68.96% <0%> (ø)`
pkg/debug/debug.go	`22.22% <0%> (ø)`
pkg/serializer/serialize.go	`9.52% <0%> (ø)`
... and 117 more

allencloud · 2018-06-12T07:17:53Z

cri/v1alpha1/cri.go

+	containerID := r.GetContainerId()
+	container, err := c.ContainerMgr.Get(ctx, containerID)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get container of %q: %v", containerID, err)


We do not need the work of. 😄

allencloud · 2018-06-12T07:19:00Z

cri/v1alpha1/cri.go

+
+	// Do not update the container when there is a removal in progress.
+	if container.State.Status == apitypes.StatusRemoving {
+		return nil, fmt.Errorf("container %q is in removing state", containerID)


This error is not so readable. How about cannot update container resource when it is in removing state?

allencloud · 2018-06-12T07:21:06Z

cri/v1alpha1/cri.go

+		Resources: resources,
+	}
+	c.ContainerMgr.Update(ctx, containerID, updateConfig)
+	if err != nil {


Which err this line code is using? From resources, err := resourceToCriResource(r.GetLinux()) or c.ContainerMgr.Update(ctx, containerID, updateConfig)?

If it is the latter one, please add more to be err = c.ContainerMgr.Update(ctx, containerID, updateConfig), and in the next line of fmt.Errorf() to construct this with the specific error. Thanks a lot.

allencloud · 2018-06-12T07:24:40Z

cri/v1alpha1/cri_utils.go

@@ -770,3 +770,29 @@ func parseUserFromImageUser(id string) string {
 	// no group, just return the id
 	return id
 }
+
+// resourceToCriResource converts OCILinuxResource to apitypes.Resources Object.
+func resourceToCriResource(new *runtime.LinuxContainerResources) (apitypes.Resources, error) {


Why is the input parameter's naming is new? Quite strange, how about changing another new name, like resource or something like that.

allencloud · 2018-06-12T09:47:16Z

cri/v1alpha1/cri_utils.go

@@ -770,3 +770,27 @@ func parseUserFromImageUser(id string) string {
 	// no group, just return the id
 	return id
 }
+
+// resourceToCriResource converts OCILinuxResource to apitypes.Resources Object.
+func resourceToCriResource(newresource *runtime.LinuxContainerResources) apitypes.Resources {


Please use the Camel-Case naming. s/newresource/newResource/g.

starnop · 2018-06-12T12:33:33Z

@allencloud Thank you for your patient guidance. Everything mentioned above has been resolved.

allencloud · 2018-06-13T02:25:19Z

cri/v1alpha2/cri.go

@@ -686,7 +686,27 @@ func (c *CriManager) ListContainerStats(ctx context.Context, r *runtime.ListCont

 // UpdateContainerResources updates ContainerConfig of the container.
 func (c *CriManager) UpdateContainerResources(ctx context.Context, r *runtime.UpdateContainerResourcesRequest) (*runtime.UpdateContainerResourcesResponse, error) {
-	return nil, fmt.Errorf("UpdateContainerResources Not Implemented Yet")
+	containerID := r.GetContainerId()
+	container, err := c.ContainerMgr.Get(ctx, containerID)


I am wondering if we need to add lock here for container.

allencloud · 2018-06-13T02:26:03Z

cri/v1alpha2/cri.go

+	}
+
+	// cannot update container resource when it is in removing state
+	if container.State.Status == apitypes.StatusRemoving {


Please check here if we have already support StatusRemoving in container manager. @HusterWan

allencloud · 2018-06-13T06:49:13Z

cri/v1alpha1/cri.go

+	}
+	err = c.ContainerMgr.Update(ctx, containerID, updateConfig)
+	if err != nil {
+		return nil, fmt.Errorf("failed to update resource for container %q with error: %v", containerID, err)


please remove with error

allencloud · 2018-06-13T06:54:38Z

cri/v1alpha1/cri.go

+	defer container.Unlock()
+
+	// cannot update container resource when it is in removing state
+	if container.State.Status == apitypes.StatusRemoving {


Please move this part into container_state.go

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

allencloud · 2018-06-14T04:28:28Z

LGTM

pouchrobot added kind/feature size/M labels Jun 12, 2018

allencloud reviewed Jun 12, 2018

View reviewed changes

allencloud assigned YaoZengzeng Jun 12, 2018

starnop force-pushed the cri-update branch 4 times, most recently from 77cbd41 to cd3450e Compare June 12, 2018 09:09

allencloud reviewed Jun 12, 2018

View reviewed changes

starnop force-pushed the cri-update branch from cd3450e to af4c92c Compare June 12, 2018 12:26

pouchrobot added size/L and removed size/M labels Jun 12, 2018

starnop force-pushed the cri-update branch from af4c92c to 8ab7bfc Compare June 12, 2018 12:28

starnop changed the title ~~[WIP] feature: UpdateContainerResources of CRI Manager~~ feature: UpdateContainerResources of CRI Manager Jun 12, 2018

allencloud reviewed Jun 13, 2018

View reviewed changes

starnop force-pushed the cri-update branch from 8ab7bfc to e9f6205 Compare June 13, 2018 06:25

allencloud reviewed Jun 13, 2018

View reviewed changes

starnop force-pushed the cri-update branch 3 times, most recently from a807c6e to 390d95e Compare June 13, 2018 08:24

pouchrobot added size/M and removed size/L labels Jun 13, 2018

starnop force-pushed the cri-update branch from 390d95e to 6f2d94e Compare June 13, 2018 08:58

starnop force-pushed the cri-update branch from 6f2d94e to e5230f4 Compare June 13, 2018 09:50

feature: UpdateContainerResources of CRI Manager

985234c

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

starnop force-pushed the cri-update branch from e5230f4 to 985234c Compare June 14, 2018 01:23

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jun 14, 2018

allencloud merged commit 1b35c2a into AliyunContainerService:master Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: UpdateContainerResources of CRI Manager #1511

feature: UpdateContainerResources of CRI Manager #1511

starnop commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 12, 2018 •

edited

Loading

allencloud Jun 12, 2018

allencloud Jun 12, 2018

allencloud Jun 12, 2018

allencloud Jun 12, 2018

allencloud Jun 12, 2018

starnop commented Jun 12, 2018

allencloud Jun 13, 2018

allencloud Jun 13, 2018

allencloud Jun 13, 2018

allencloud Jun 13, 2018

allencloud commented Jun 14, 2018

feature: UpdateContainerResources of CRI Manager #1511

feature: UpdateContainerResources of CRI Manager #1511

Conversation

starnop commented Jun 12, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Jun 12, 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

starnop commented Jun 12, 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

allencloud commented Jun 14, 2018

starnop commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 12, 2018 •

edited

Loading