refactor: re-design upgrade feature #2211

HusterWan · 2018-09-06T13:36:07Z

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

Before i re-design the upgrade feature, i have thought about a question for a long time: what's for user to use upgrade ? If you want to change memory, cpu, env etc, i think you should use the update feature. But if you want to update the container's image, and inherit the volume and network from the old container, now you should think about the upgrade feature.

After figured out the purpose of the upgrade feature, i re-designed the api interface and the core logic of the upgrade.

About API

Now, upgrade api request body is below, we only support you to specify the Image which used to create a new container and the new Entrypoint and Cmd that the new container used to run.

type ContainerUpgradeConfig struct {

	// Execution commands and args
	// Min Items: 1
	Cmd []string `json:"Cmd"`

	// The entrypoint for the container as a string or an array of strings.
	// If the array consists of exactly one empty string (`[""]`) then the entry point is reset to system default.
	//
	Entrypoint []string `json:"Entrypoint"`

	// image
	// Required: true
	Image string `json:"Image"`
}

I think it's enough to upgrade a container from the old image to a new one.

About the core logic

I think we must solve problems below when implement the upgrade feature.

1. Which cmd should be used to create the new container when upgrade

There are four commands as mentioned below, when create a new container in upgrade, the order of using the command is:

We should first use CMD3;
We should use the CMD1, if CMD4 is not specified ;
We need to use CMD4, if CMD1 is also not specified ;
CMD2 should not be used.

cmd	named
old cmd of container config	CMD1
old image's cmd	CMD2
new cmd of container upgrade config	CMD3
new image's cmd	CMD4

2. Inherit the volumes and network from the old container when create new container

network: before create a new container, we should first stop the old container if it is running, then we can use the network config of the old container.
storage: new container inherit all the mounts information of the old one, so we can use the volumes used by old container, and we should also parse the volume information contained in new image.

3. How to prepare new rootfs for the new container

In containerd, snapshot represents the rootfs of container, so we should use the new image to create a new snapshot for the new container in upgrade. In case of snapshot name conflicted, we add a random suffix string to the container id as the name of the new snapshot.

4. Rollback

When occurred any error during upgrade, we should have the ability to rollback the upgrade and recover the old container just like it before.

Ⅱ. Does this pull request fix one issue?

none

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

Yes, add more test cases

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

codecov-io · 2018-09-06T17:37:21Z

Codecov Report

Merging #2211 into master will increase coverage by 0.03%.
The diff coverage is 67.54%.

@@            Coverage Diff             @@
##           master    #2211      +/-   ##
==========================================
+ Coverage   67.33%   67.37%   +0.03%     
==========================================
  Files         213      215       +2     
  Lines       17517    17593      +76     
==========================================
+ Hits        11795    11853      +58     
- Misses       4327     4340      +13     
- Partials     1395     1400       +5

Flag	Coverage Δ
#criv1alpha1test	`32.11% <11.25%> (-0.12%)`	⬇️
#criv1alpha2test	`36.14% <11.25%> (-0.18%)`	⬇️
#integrationtest	`40.3% <66.88%> (+0.15%)`	⬆️
#nodee2etest	`33.52% <11.92%> (-0.14%)`	⬇️
#unittest	`23.13% <4.63%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
cri/v1alpha1/cri_utils.go	`83.22% <0%> (ø)`	⬆️
cri/v1alpha2/cri_utils.go	`90.57% <100%> (ø)`	⬆️
daemon/mgr/image.go	`57.7% <50%> (-0.23%)`	⬇️
ctrd/container.go	`59.28% <50%> (+0.95%)`	⬆️
daemon/mgr/container_upgrade.go	`65.09% <65.09%> (ø)`
daemon/mgr/container_types.go	`83.33% <80%> (-0.31%)`	⬇️
pkg/utils/random_string.go	`81.81% <81.81%> (ø)`
daemon/mgr/container.go	`59% <83.33%> (+1.3%)`	⬆️
ctrd/watch.go	`75.75% <0%> (-4.55%)`	⬇️
daemon/mgr/snapshot.go	`89.85% <0%> (-4.35%)`	⬇️
... and 9 more

allencloud · 2018-09-06T17:45:22Z

ctrd/container_types.go

+	Labels      map[string]string
+	IO          *containerio.IO
+	Spec        *specs.Spec
+	SnapshotKey string


What is this used for?

SnapshotKey is the snapshot id used by container.

we used to use container id as snapshot id, but when we support upgrade a container, the snapshot id may be different with container id.

So, we must record the snapshot id that container used

allencloud · 2018-09-06T17:48:05Z

daemon/mgr/container.go

-// Upgrade upgrades a container with new image and args.
-func (mgr *ContainerManager) Upgrade(ctx context.Context, name string, config *types.ContainerUpgradeConfig) error {
-	c, err := mgr.container(name)
+func (mgr *ContainerManager) getImageRef(ctx context.Context, image string) (digest.Digest, string, error) {


I do not think this is function of ContainerManager. It is better to locate it in ImageManager. So we should use containerMgr.ImageMgr.GetImageRef. WDYT? @HusterWan

Good advice, i will put those function to ImageMgr

allencloud · 2018-09-06T17:48:34Z

daemon/mgr/container.go

+	return imageID, primaryRef.String(), nil
+}
+
+func (mgr *ContainerManager) getOCIImageConfig(ctx context.Context, image string) (ocispec.ImageConfig, error) {


Same issue, improper place to locate the function here.

allencloud · 2018-09-06T17:51:07Z

daemon/mgr/container.go

+	return ociImage.Config, nil
+}
+
+func (mgr *ContainerManager) prepareContainerEntrypointForUpgrade(ctx context.Context, c *Container, config *types.ContainerUpgradeConfig) error {


For all the related Upgrade functions. could we move all of them to a saperated file container_upgrade.go ?
Just a piece of advice.

allencloud · 2018-09-06T17:54:45Z

daemon/mgr/container.go

+	// Upgrade success, remove snapshot of old container
+	if err := mgr.Client.RemoveSnapshot(ctx, oldSnapID); err != nil {
+		// TODO(ziren): remove old snapshot failed, may cause dirty data
+		logrus.Errorf("failed to remove snapshot %s: %v", oldSnapID, err)


Do we need the log level of Error? Is Warn enough?
Just double check to see the right way to use error and warn.

allencloud · 2018-09-06T17:55:30Z

daemon/mgr/container_types.go

@@ -258,6 +258,9 @@ type Container struct {

 	// MountFS is used to mark the directory of mount overlayfs for pouch daemon to operate the image.
 	MountFS string `json:"-"`
+
+	// SnapshotKey specify id of the snapshot that container using.
+	SnapshotKey string


How about directly using SnapshotID?

allencloud · 2018-09-09T01:57:06Z

apis/swagger.yml

-        properties:
-          HostConfig:
-            $ref: "#/definitions/HostConfig"
+      ContainerUpgradeConfig is used for API "POST /containers/upgrade". when upgrade a container,


"POST /containers/{id}/upgrade"?

Are you missing the {id}/? @HusterWan

s/when upgrade/When upgrading/? Uppercase and add the ing?

allencloud · 2018-09-09T15:36:39Z

validate-swagger
./hack/validate_swagger.sh
2018/09/09 15:14:46 yaml: line 2457: did not find expected key
Makefile:121: recipe for target 'validate-swagger' failed
make: *** [validate-swagger] Error 1

HusterWan · 2018-09-09T15:56:49Z

validate-swagger
./hack/validate_swagger.sh
2018/09/09 15:14:46 yaml: line 2457: did not find expected key
Makefile:121: recipe for target 'validate-swagger' failed
make: *** [validate-swagger] Error 1

@allencloud fixed

allencloud · 2018-10-09T10:00:18Z

Could we make this move on ASAP? @HusterWan
I think it is always a fancy feature for end-users. ❤️

HusterWan · 2018-10-11T11:36:17Z

Could we make this move on ASAP? @HusterWan
I think it is always a fancy feature for end-users. ❤️

of course, i have updated this PR

allencloud · 2018-10-12T06:51:43Z

daemon/mgr/container.go

@@ -322,9 +321,9 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, errors.Wrapf(errtypes.ErrInvalidParam, "unknown runtime %s", config.HostConfig.Runtime)
 	}

-	config.Image = primaryRef.String()
+	snapID := id


Could you explain why to use generated container ID to assign to snapshot ID?

SnapshotID is used to equal with ContainerID, but after upgrade they are different.

So, i use param snapID here just to reminder that snapshot id and container id are different

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan · 2018-10-15T14:14:28Z

upgrade should consider things below:

Should support delete old volumes logic when upgrade a container.
CopyData only does once when the volume is created
the order of which cmd should be used need discuss again.

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan · 2018-10-17T06:41:52Z

Since we need more discussion to determine how to integrate upgrade feature to Sigma, we can first merge this pr now. @allencloud @fuweid

allencloud · 2018-10-17T08:13:17Z

LGTM

pouchrobot added kind/design kind/feature kind/refactor size/XL labels Sep 6, 2018

HusterWan force-pushed the zr/fix-upgrade branch from 22257ce to ab1b5f2 Compare September 6, 2018 16:58

pouchrobot added size/XXL and removed size/XL labels Sep 6, 2018

allencloud reviewed Sep 6, 2018

View reviewed changes

pouchrobot added the conflict/needs-rebase label Sep 7, 2018

HusterWan force-pushed the zr/fix-upgrade branch from ab1b5f2 to ebd117a Compare September 7, 2018 06:00

pouchrobot removed the conflict/needs-rebase label Sep 7, 2018

HusterWan force-pushed the zr/fix-upgrade branch 2 times, most recently from 21c3b71 to bffe170 Compare September 7, 2018 11:03

HusterWan changed the title ~~[wip]refactor: re-design upgrade feature~~ refactor: re-design upgrade feature Sep 7, 2018

pouchrobot added the conflict/needs-rebase label Sep 9, 2018

allencloud reviewed Sep 9, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Sep 9, 2018

HusterWan force-pushed the zr/fix-upgrade branch from bffe170 to 87917c0 Compare September 9, 2018 15:14

pouchrobot removed the conflict/needs-rebase label Sep 9, 2018

HusterWan force-pushed the zr/fix-upgrade branch from 87917c0 to 0ad0282 Compare September 9, 2018 15:56

HusterWan force-pushed the zr/fix-upgrade branch from 0ad0282 to 3d573a7 Compare September 9, 2018 16:02

HusterWan force-pushed the zr/fix-upgrade branch from c8443e6 to 270cb6d Compare September 13, 2018 03:31

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 8, 2018

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 10, 2018

HusterWan force-pushed the zr/fix-upgrade branch from 270cb6d to c04e44b Compare October 11, 2018 03:34

pouchrobot removed the conflict/needs-rebase label Oct 11, 2018

allencloud reviewed Oct 12, 2018

View reviewed changes

refactor: re-design upgrade feature

267b2c3

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan force-pushed the zr/fix-upgrade branch from c04e44b to cd51345 Compare October 14, 2018 14:31

HusterWan force-pushed the zr/fix-upgrade branch from cd51345 to f521a14 Compare October 16, 2018 11:59

vendor: remove unuse vendor package github.com/imdario/mergo

a89c125

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan force-pushed the zr/fix-upgrade branch from f521a14 to a89c125 Compare October 17, 2018 04:22

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

allencloud merged commit d53105e into AliyunContainerService:master Oct 17, 2018

pouchrobot mentioned this pull request Oct 20, 2018

Weekly Report of PouchContainer from 2018-10-13 to 2018-10-20 #2337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: re-design upgrade feature #2211

refactor: re-design upgrade feature #2211

HusterWan commented Sep 6, 2018 •

edited

Loading

codecov-io commented Sep 6, 2018 •

edited by codecov bot

Loading

allencloud Sep 6, 2018

HusterWan Sep 7, 2018

allencloud Sep 6, 2018

HusterWan Sep 7, 2018

allencloud Sep 6, 2018

allencloud Sep 6, 2018

allencloud Sep 6, 2018

allencloud Sep 6, 2018

allencloud Sep 9, 2018 •

edited

Loading

allencloud commented Sep 9, 2018

HusterWan commented Sep 9, 2018

allencloud commented Oct 9, 2018

HusterWan commented Oct 11, 2018

allencloud Oct 12, 2018

HusterWan Oct 12, 2018

HusterWan commented Oct 15, 2018

HusterWan commented Oct 17, 2018

allencloud commented Oct 17, 2018

refactor: re-design upgrade feature #2211

refactor: re-design upgrade feature #2211

Conversation

HusterWan commented Sep 6, 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 Sep 6, 2018 • edited by codecov bot 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

Choose a reason for hiding this comment

allencloud Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

allencloud commented Sep 9, 2018

HusterWan commented Sep 9, 2018

allencloud commented Oct 9, 2018

HusterWan commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented Oct 15, 2018

HusterWan commented Oct 17, 2018

allencloud commented Oct 17, 2018

HusterWan commented Sep 6, 2018 •

edited

Loading

codecov-io commented Sep 6, 2018 •

edited by codecov bot

Loading

allencloud Sep 9, 2018 •

edited

Loading