Migrate container configuration in portlayer in memory by emlin · Pull Request #4146 · vmware/vic

emlin · 2017-03-06T16:05:39Z

Migrate container configuration in memory if there is migration plugin registered
In commit method, drop extraconfig change if data is migrated
If data migration failed, keep partial migrated value, and keep migration error in container cache
In docker inspect and docker ps show container status as "broken" if data migration failed
If container status is broken
- allow container stop
- force delete even for "docker rm"

emlin · 2017-03-06T16:07:16Z

@matthewavery added you as reviewer cause you ever worked on this issue.

matthewavery · 2017-03-06T17:12:06Z

lib/apiservers/engine/backends/container_proxy.go


 	Stop(vc *viccontainer.VicContainer, name string, seconds *int, unbound bool) error
 	IsRunning(vc *viccontainer.VicContainer) (bool, error)
+	IsBroken(vc *viccontainer.VicContainer) (bool, error)


Are we sure this will only be needed in the container endpoints? I am wondering if we will need this information for networks or volumes at some point.

Right now, guess this is fine.
This information is needed by container stop/start/rm, cause we still allow user to delete the broken container. But for all the other functions, if the configuration is available after half migration, those piece of function will looks good, it otherwise, that will not. The function is consistent.
We don't that care about the function of broken containers atm, as long as that's deletable.

matthewavery · 2017-03-06T17:21:19Z

lib/config/executor/container_vm.go

 	ExecutionEnvironment string

-	// Unambiguous ID with meaning in the context of its hosting execution environment
+	// Unambiguous ID with meaning in the context of its hosting execution environment. Change this definition will cause container backward compatibility issue


please add "Please don't change this." I am not sure, you might add a "without talking to emma lin" to that. That way if someone does want to change it for whatever reason they can talk to you about why they likely shouldn't.

matthewavery · 2017-03-06T17:26:46Z

lgtm 😄 , other than the comment on the container ID field. Though that is simply a suggestion.

emlin · 2017-03-06T17:37:50Z

For easy to understand, here I posted the output of docker commands if there is one container broken for data migration error(the error if faked, I cannot have a real failure for that now):

CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
fc67c420adb3        busybox             "sh"                10 minutes ago      broken                                  wonderful_mestorf```
```docker -H 10.160.58.173:2376 --tls inspect fc
[
    {
        "Id": "fc67c420adb38c166c3402fe925c9f28b47c036ae43ff07d43338aa6f3933e1e",
        "Created": "2017-03-06T17:22:04Z",
        "Path": "sh",
        "Args": [],
        "State": {
            "Status": "broken",
            "Running": false,
            "Paused": false,
            "Restarting": false,
            "OOMKilled": false,
            "Dead": false,
            "Pid": 0,
            "ExitCode": 0,
            "Error": "Migration failed: Faked migration error",
            "StartedAt": "2017-03-06T17:22:28Z",
            "FinishedAt": "2017-03-06T17:22:52Z"
        },
...

vburenin · 2017-03-06T17:59:59Z

lib/apiservers/engine/backends/container_proxy.go

 	}
-
-	return inspectJSON.State.Running, nil
+	return inspectJSON.State.Status == "broken", nil


Are there any statuses defined as constants? Using strings is not the best solution especially if it used more than 2 times across all project.
I also don't like IsBroken method. It's just awkward. There should be a method that returns status only it would be way more useful for other possible cases too.

caglar10ur · 2017-03-06T20:41:06Z

lib/apiservers/engine/backends/container_proxy.go

 	}
-	if !running {
+	// attempt to stop container if status is running or broken
+	if !inspectJSON.State.Running && inspectJSON.State.Status != "broken" {


can we make this string a const?

caglar10ur · 2017-03-06T20:41:52Z

lib/apiservers/portlayer/restapi/handlers/containers_handlers.go

-	info.ContainerConfig.State = s
+	var state string
+	if container.MigrationError != nil {
+		state = "broken"


caglar10ur · 2017-03-06T20:42:56Z

lib/config/executor/container_vm.go

 	ExecutionEnvironment string

-	// Unambiguous ID with meaning in the context of its hosting execution environment
+	// Unambiguous ID with meaning in the context of its hosting execution environment. Change this definition will cause container backward compatibility issue. Please don't change this.


Change/Changing

Also the comment might be little bit more than this so that reader can understand why he/she shouldn't be changing this

caglar10ur · 2017-03-06T20:45:41Z

lib/portlayer/exec/commit.go

 			}
+			// nilify ExtraConfig if container configuration is migrated
+			// in this case, VCH and container are in different version. Migrated configuration cannot be written back to old container, to avoid data loss in old version's container
+			if h.Migrated {


We are setting ExtraConfig to nil if we are powered on so why do we need this?

This is because configuration data is migrated. If write back to container vmx file, the old binary cannot read back new data. We ever talked about that part in one meeting, remember?

This value is used to skip new features on old containers

sflxn · 2017-03-07T22:24:29Z

lib/apiservers/engine/backends/container.go

+			return err
+		}
+		// force stop if container is broken to make sure container is deletable later
+		if state.Status == ContainerError {


This is checking for status "error". Is this what you're using for "broken"?

So, I see where you're setting it on the portlayer side. Since this is our state string, why not use a more explicit string, such as "migrating"?

This is one error state caused by migration, not a working state as migrating. I think we might have other incorrect scenarios other than migration error, so I didn't use migration error as the state.

Ok, I just got broken was an actual status. It's actually a description, meaning the status is error.

Can you change the comment to say "if container is in an error state"? Or is broken the best description?

I'll update the comment.

sflxn · 2017-03-07T22:27:58Z

lib/apiservers/engine/backends/container_proxy.go


 	if c.client == nil {
-		return false, InternalServerError("ContainerProxy.IsRunning failed to get a portlayer client")
+		return nil, InternalServerError("ContainerProxy.dockerInfo failed to get a portlayer client")


.State and not .dockerInfo

sflxn · 2017-03-07T22:42:56Z

small nitpicks, but LGTM

vmwclabot added the cla-not-required label Mar 6, 2017

emlin requested a review from matthewavery March 6, 2017 16:06

emlin force-pushed the 3610 branch from e0a0b71 to 6952ecf Compare March 6, 2017 16:08

matthewavery reviewed Mar 6, 2017

View reviewed changes

matthewavery approved these changes Mar 6, 2017

View reviewed changes

vburenin reviewed Mar 6, 2017

View reviewed changes

emlin changed the title ~~[full ci]Migrate container configuration in portlayer in memory~~ [specific ci=Group0-Bugs]Migrate container configuration in portlayer in memory Mar 6, 2017

caglar10ur reviewed Mar 6, 2017

View reviewed changes

emlin changed the title ~~[specific ci=Group0-Bugs]Migrate container configuration in portlayer in memory~~ [specific ci=Group1-Docker-Commands]Migrate container configuration in portlayer in memory Mar 6, 2017

emlin force-pushed the 3610 branch from 512297e to 8c2bc0f Compare March 6, 2017 23:08

emlin changed the title ~~[specific ci=Group1-Docker-Commands]Migrate container configuration in portlayer in memory~~ [full ci]Migrate container configuration in portlayer in memory Mar 7, 2017

emlin force-pushed the 3610 branch 3 times, most recently from 970a3e6 to ccaf115 Compare March 7, 2017 16:53

emlin changed the title ~~[full ci]Migrate container configuration in portlayer in memory~~ [specific ci=Group1-Docker-Commands]Migrate container configuration in portlayer in memory Mar 7, 2017

emlin force-pushed the 3610 branch 2 times, most recently from 6798498 to 554b448 Compare March 7, 2017 19:55

caglar10ur approved these changes Mar 7, 2017

View reviewed changes

emlin force-pushed the 3610 branch from 554b448 to 6b34bfb Compare March 7, 2017 22:16

emlin added 7 commits March 7, 2017 16:18

Add container data migration

31bb11e

fix abnormal behavior caused by migration error status

570077c

fix UT

e05fd55

update comments

0793813

define constants

3565d3b

fix docker rm error message

c7376af

update interface to return docker types.ContainerState

bedcfd2

Fix container original version issue

6b34bfb

This value is used to skip new features on old containers

sflxn reviewed Mar 7, 2017

View reviewed changes

emlin changed the title ~~[specific ci=Group1-Docker-Commands]Migrate container configuration in portlayer in memory~~ Migrate container configuration in portlayer in memory Mar 7, 2017

update message

6b8ec42

emlin merged commit f04b2a7 into vmware:master Mar 8, 2017

emlin deleted the 3610 branch March 8, 2017 00:14

chengwang86 mentioned this pull request Mar 8, 2017

[specific ci=Group11-Upgrade] Reconfigure VM display name #4134

Merged

Conversation

emlin commented Mar 6, 2017

Uh oh!

emlin commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewavery commented Mar 6, 2017

Uh oh!

emlin commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sflxn commented Mar 7, 2017 • edited by frapposelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sflxn commented Mar 7, 2017 •

edited by frapposelli

Loading