implement (re)store of proxy's state #107

dvoytik · 2017-08-18T16:45:01Z

Introduce the high availability feature of cc-proxy by implementing
store/restore of proxy's state to/from disk. This feature depends
on the ability of shim to reconnect to cc-proxy if connection is lost.

Fixes #4.

Signed-off-by: Dmitry Voytik dmitry.voytik@huawei.com

sboeuf · 2017-08-18T16:53:20Z

@dvoytik sorry but we are kind of rushing those days because we have some milestones to reach. And unfortunately, what you implemented is not in the scope of our P1 issues right now, that's why the slow reviews.
But don't think we don't appreciate your contribution, and I wanted to let you know that it will take some time to review both clearcontainers/shim#54 and #107 because they introduce core changes to the shim and proxy.
BTW, did you try some real/functional use cases where you have both the shim and the proxy patched with your patches, in order to make sure it works well ? Those tests should be added to the github.com/clearcontainers/tests repo, so that we run them on every PR.

dvoytik · 2017-08-21T09:25:57Z

Hi @sboeuf,
No problem, I understand this. Please take your time, I don't hurry.
I tested manually both patches. It works well except that hyperstart has it's own internal time-out (around 5-7 seconds) and if proxy doesn't recover quick enough then hyperstart can hang. I didn't invest much time in debugging hyperstart because it is being replaced with agent. My plan is to wait until agent is stabilised and check if the same problem happens.
I plan to submit a functional test case to tests repo later, but without merging these patches I'm not sure if it makes sense to do it before.

coveralls · 2017-08-21T11:15:56Z

Coverage decreased (-0.3%) to 73.933% when pulling 7a2f972 on dvoytik:store_restore_state into d7a4dd8 on clearcontainers:master.

jodh-intel · 2017-08-31T10:24:07Z

proxy.go

@@ -124,6 +125,33 @@ func newClient(proxy *proxy, conn net.Conn) *client {
 	}
 }

+func (proxy *proxy) restoreTokens(vm *vm, tokens []string) error {


Shouldn't this be tokens []Token? Also, what if vm == nil?

Unfortunately proxy.allocateTokens() returns []string. I'd like to avoid writing a conversion loop from []string to []Token in registerVM() and revert conversion loop in storeVMState().

jodh-intel · 2017-08-31T10:24:39Z

proxy.go

+	return storeStateDir + "vm_" + id + ".json"
+}
+
+func storeVMState(vm *vm, tokens []string) {


Again, tokens []Token?

jodh-intel · 2017-08-31T10:25:52Z

proxy.go

+	}
+}
+
+func delVMAndState(proxy *proxy, vm *vm) {


What if proxy and vm are nil?

I'll add checks.

jodh-intel · 2017-08-31T10:26:45Z

proxy.go

+	}
+}
+
+func restoreVMState(proxy *proxy, containerID string) {


Missing checks on params. Also, this is a very long function - I think it must be close the the cyclomatic complexity limit so I'd consider refactoring it into smaller chunks.

Ok, will do.

jodh-intel · 2017-08-31T10:29:34Z

proxy.go

@@ -188,6 +216,207 @@ func (proxy *proxy) releaseToken(token Token) (*tokenInfo, error) {
 	return info, nil
 }

+var storeStateDir = "/var/lib/clear-containers/proxy/"


Can you move this to the top of the file? Its value should also be passed in from Makefile so that at the top it's something like:

// XXX: set by build var storeStateDir string

Got it. Will do. Should it be set up to "/var/lib/clear-containers/proxy/" in Makefile?

Well, it should probably be set to $PKGLIBDIR/proxy/ (feel free to steal that chunk of code from the runtime's Makefile ;)

jodh-intel · 2017-08-31T10:39:33Z

proxy.go

+	}
+
+	var proxyState proxyStateOnDisk
+	err = json.Unmarshal(fdata, &proxyState)


Something to consider: what happens when we change the proxy state file format? The 3 main approaches are:

Don't handle it and just error if the on-disk state doesn't match what the proxy expects.

Add a version field to vmStateOnDisk to allow the proxy to determine how to deal with a different format state file.

Make the code "sniff" the state file data and see which fields it contains and their types to determine how to handle such state file differences (the "autoconf approach" ;)

Clearly, here, there is only 1 possible on-disk format, but I think it would be worth thinking about this and documenting the strategy for how we plan to handle any changes to vmStateOnDisk.

IMHO adding a version field to the format would be easiest way to handle this kind of issues.

jodh-intel · 2017-08-31T10:40:15Z

proxy.go

-	if proxy.socketPath, err = getSocketPath(); err != nil {
-		return fmt.Errorf("couldn't get a rigth socket path: %v", err)
+		if proxy.socketPath, err = getSocketPath(); err != nil {
+			return fmt.Errorf("couldn't get a rigth socket path: %v", err)


Typo that could be fixed now I've spotted it ;-) "rigth" => "right".

Thanks, will do.

jodh-intel · 2017-08-31T10:42:28Z

proxy.go

+	proxy.Lock()
+	delete(proxy.vms, vm.containerID)
+	proxy.Unlock()
+	proxy.storeState()


This take a new lock. I don't know if there is an issue here in that the state could change between the call to Unlock() and the call to storeState()?

Here proxy.vms hash-map is protected. I regularly run code with -race parameter to see if data races appear.

jodh-intel · 2017-08-31T10:46:36Z

proxy_test.go

+
+	// clean up a possible state
+	os.RemoveAll(storeStateDir)
+	assert.Equal(t, rig.proxy.restoreState(), false)


Not a major problem, but in the runtime for new code we've started creating a assert variable to avoid having to hass the testing.T each time:

assert := assert.New(t) : : assert.Equal(rig.proxy.restoreState(), false)

Got it. Thanks. Will do.

jodh-intel · 2017-08-31T10:47:41Z

proxy_test.go

@@ -900,3 +901,29 @@ func TestHyperstartResponse(t *testing.T) {

 	rig.Stop()
 }
+
+func TestStoreRestore(t *testing.T) {


Can you add some new tests where the restore fails due to an invalid state file (zero size, invalid contents (non-json, pure text (hello world or something ;-)))

Sure, will do.

jodh-intel · 2017-08-31T10:50:19Z

Hi @dvoytik - thanks for raising this! It looks good. I've done a first-pass review and given some suggestions. There are 2 other areas to mention:

Tests

Thanks for adding a new test. However, this is a relatively big change and although you've added a new test, the coverage level has dropped. Ideally, it would be going up ;-)
Locking code

I think the locking calls will need a more thorough review.

dvoytik · 2017-09-01T11:38:55Z

@jodh-intel, thank you for the review!
It seems like today I won't have enough time to address all comments thoroughly, and during next two weeks I'm on a vacation without access to a proper internet connection. If you don't mind, I'm going to respond to the comments and rework the PR after September, 18th.

jodh-intel · 2017-09-01T13:38:35Z

Hi @dvoytik - no problem! And enjoy your holiday! 😎

dvoytik · 2017-09-01T14:37:16Z

@jodh-intel, thanks! 😃

jodh-intel · 2017-09-29T16:34:25Z

Hi @dvoytik - hope you had a good holiday. Are you still planning to update this branch? Let me know if there is anything I can do to help.

dvoytik · 2017-10-02T12:32:57Z

@jodh-intel, thanks! Sorry, for delaying on the PR. I'll rework it and address the comments today after my main job. Unfortunately for the time being I can finish this only in my spare time :(

jodh-intel · 2017-10-02T12:49:50Z

Hi @dvoytik - understood. I'm happy to help with this if you are tied up with other things.

dvoytik · 2017-10-02T12:59:44Z

HI @jodh-intel, thank you! I feel responsibility to finish this anyway, so I'm going to work on the PR today evening. Sorry again for delaying this.

dvoytik · 2017-10-05T22:18:15Z

Hi @jodh-intel, I've update the PR. Could you please take a look at it again?

jodh-intel · 2017-10-05T14:47:47Z

proxy.go

@@ -188,6 +216,207 @@ func (proxy *proxy) releaseToken(token Token) (*tokenInfo, error) {
 	return info, nil
 }

+var storeStateDir = "/var/lib/clear-containers/proxy/"


Well, it should probably be set to $PKGLIBDIR/proxy/ (feel free to steal that chunk of code from the runtime's Makefile ;)

jodh-intel · 2017-10-05T14:49:25Z

proxy.go

+// returns false if it's a clean start (i.e. no state is stored) or restoring failed
+func (proxy *proxy) restoreState() bool {
+	proxyStateFilePath := storeStateDir + "proxy_state.json"
+	if _, err := os.Stat(storeStateDir); os.IsNotExist(err) {


Ah - sorry, yes I see now.

jodh-intel · 2017-10-06T08:34:10Z

proxy.go

@@ -84,6 +92,20 @@ type proxy struct {
 	wg sync.WaitGroup
 }

+// proxyStateOnDisk is used to re(store) proxy state on disk
+type proxyStateOnDisk struct {


This is a pretty significant feature which I think justifies moving all the state handling code into a state.go.

Ok, I'll do it.

jodh-intel · 2017-10-06T08:34:33Z

proxy.go

@@ -124,6 +125,33 @@ func newClient(proxy *proxy, conn net.Conn) *client {
 	}
 }

+func (proxy *proxy) restoreTokens(vm *vm, tokens []string) error {


jodh-intel · 2017-10-06T08:35:41Z

proxy.go

+	}
+
+	for _, token := range tokens {
+		token, err := vm.AllocateTokenAs(Token(token))


Do you need to assert that token != "" here? AllocateTokenAs() will handle it I see, but it's not an expected scenario I think?

Good catch! Thanks!

jodh-intel · 2017-10-06T08:47:08Z

proxy.go

+
+func restoreVMState(proxy *proxy, containerID string) {
+	if proxy == nil {
+		proxyLog.Errorf("proxy parameter must be not nil")


Can you add a check for containerID == "".

Can you add the containerID to the log message to aid debugging?

Will do, thanks.

jodh-intel · 2017-10-06T08:58:45Z

proxy.go

+	proxyLog.Warn("Recovering proxy state from: ", proxyStateFilePath)
+	if proxyState.Version != Version {
+		proxyLog.Warnf("Stored state version (%s) mismatches proxy"+
+			" version (%s). Aborting", proxyState.Version, Version)


We can't deal with the scenario where proxyState.Version > Version as that implies the state is from a newer version of the proxy than is running (forced downgrade), but I think we do need to handle the scenario where proxyState.Version <= Version as the running proxy should be able to handle the state of all previous versions.

I'm still not 100% convinced that a version field is the right approach: even if we have one, the code will still need to sanity-check all the state values read off disk to ensure:

they are of the correct type.

they have a "reasonable" value.

However, it doesn't hurt to include a version.

jodh-intel · 2017-10-06T09:07:36Z

proxy.go

+	proxy.vms[regVM.ContainerID] = vm
+	proxy.Unlock()
+
+	proxyLog.Infof("restoreVMState(containerId=%s,ctlSerial=%s,ioSerial=%s,console=%s)",


It would be great if you could convert all the new log messages on this PR to use the structured logging approach. For example, this one could become:

proxyLog.WithFields(logrus.Fields{ "container": regVM.ContainerID, "control-channel": regVM.CtlSerial, "io-channel": regVM.IoSerial, "console": regVM.Console, }).Info("restoring state")

Ok, will do.

jodh-intel · 2017-10-06T09:32:27Z

proxy.go

+	}
+
+	proxyState := &proxyStateOnDisk{
+		Version:         Version,


This is the storing the protocol version. We need a separate version to describe the state file version as that might need to change more frequently.

Ok. I'll rework this part.

jodh-intel · 2017-10-06T09:37:20Z

proxy_test.go

@@ -929,3 +930,93 @@ func TestGetSocketPath(t *testing.T) {
 	assert.NotNil(t, err, err)
 	assert.Equal(t, p, "")
 }
+
+func TestStoreRestore(t *testing.T) {


Can you add tests for all the functions added? (This might necessitate splitting them into smaller functions to make them more easily testable).

Ok, I'll try :)

jodh-intel · 2017-10-06T09:46:27Z

Hi @dvoytik - thank you again raising this PR. It's a very significant new feature and because of its sensitivity, I'm afraid my pedantic knob is up to 11 😄

A couple of general points not already mentioned in the review:

Can you space the code out a little so that there are blank lines between logical blocks / if tests, etc. That will make it a lot easier to read.
I like the simplicity of the current approach but I do feel it would be better to have all functions return a bool or an error and allow the higher-levels to determine whether the error should be fatal or not. That also makes writing tests much easier as there is something concrete to check.
I think we should document how the proxy handles full and partial failure to restore state. It may be that we want to add a --error-on-failure-to-restore-type option for example for some users. The default should probably be to log as much as possible and continue to startup, but that might not be desirable in some environments.

As I've said before, I'm more than happy to help out with any of this. This is a big feature so if we can find a way to logically sub-divide the work, just ping me so we can work together on this.

dvoytik · 2017-10-06T20:57:42Z

Hi @jodh-intel, thank you again for reviewing the PR! No problems with a pedantic attitude at all! Actually I value this a lot, especially when it's reasonable.

All your points are valid, I agree. I'll rework the PR with your comments in mind.

If you don't mind I'll try to finish this PR. It's just going to take a little longer because for the time being I can only work on this at evenings.

IMHO, --error-on-failure-to-restore make sense to implement as a separate PR.

jodh-intel · 2017-10-09T10:35:15Z

Thanks @dvoytik! ;)

dvoytik · 2017-10-09T22:31:16Z

The PR has been updated.

jodh-intel · 2017-10-10T13:44:21Z

Thanks @dvoytik - reviewing and testing with clearcontainers/shim#54 ...

jodh-intel · 2017-10-10T13:24:01Z

state.go

+}
+
+func logContID(containerID string) *logrus.Entry {
+	return proxyLog.WithField("container", containerID)


jodh-intel · 2017-10-10T13:25:00Z

state.go

+		}
+
+		proxy.Lock()
+		proxy.tokenToVM[token] = &tokenInfo{


I'm not wholly clear on this. Should the token be removed if findSessionByToken() fails below?

Practically if vm.findSessionByToken(token) fails here then the vm object, which owns tokens, is being released later by delVMAndState, thus tokens are being also released.
But maybe I should release them here with such code:

vm.Lock() for token := range vm.tokenToSession { _ = vm.freeTokenUnlocked(token) delete(vm.tokenToSession, token) } vm.Unlock()

Slightly more complicated code but more future-proof?

Yes, I think that looks good. What do you think @sboeuf?

Yes fine with me.

jodh-intel · 2017-10-10T16:42:56Z

This is looking very nice.

@sboeuf, @amshinde - could you take a look please?

sboeuf · 2017-10-10T16:49:11Z

@jodh-intel sure I'll take a look

jodh-intel · 2017-10-11T14:07:09Z

state.go

+
+// returns false if it's a clean start (i.e. no state is stored) or restoring
+// failed
+func (proxy *proxy) restoreState() bool {


I think for clarity this should be called something like restoreAllState() because it is not doing the opposite job to storeState():

storeState(): only stores proxy state.

restoreState(): restores both proxy state and state of all VMs.

(alternatively, you could split the VM restore code in restoreState() into a separate function I guess).

OK, I'll rename it.

jodh-intel · 2017-10-11T14:07:26Z

proxy.go

+		logContID(vm.containerID).Errorf(
+			"couldn't store a VM state: %v", err)
+	}
+	if err := proxy.storeState(); err != nil {


Missing blank line above the if (here and in various other places).

Oops :) I'll fix these, thanks!

jodh-intel · 2017-10-11T14:09:48Z

proxy.go

+			"couldn't store a VM state: %v", err)
+	}
+	if err := proxy.storeState(); err != nil {
+		proxyLog.Errorf("couldn't store proxy's state: %v",


I'd put the err on this line as it looks a bit odd down there on its own (same comment for a few other occurrences).

Thanks, I'll fix these.

jodh-intel · 2017-10-11T15:05:16Z

state.go

+		}
+
+		proxy.Lock()
+		proxy.tokenToVM[token] = &tokenInfo{


Yes, I think that looks good. What do you think @sboeuf?

sboeuf · 2017-10-12T06:55:26Z

Sorry I could not review that, but I'll do that tomorrow first thing! And I know I am gonna spend some time since that's a pretty huge PR

sboeuf

@dvoytik thanks for this huge PR. I have several comments, but I have also one concern (unless I am missing something). From my understanding, you basically STORE during RegisterVM() and RESTORE during UnregisterVM(). The issue is that you're not covering all cases here. You will miss all the tokens/sessionID created during a pod lifecycle. For instance, after the VM has been started and that we have stored all the info, if we create new containers/processes, new tokens/sessionIDs are gonna be generated by the proxy. Am I missing something or this is something expected ? Cause that means that we are not covering a lot of crashing cases otherwise.
@sameo could you please also take a look since this is a significant PR/feature.

sboeuf · 2017-10-12T20:23:01Z

Makefile

@@ -16,6 +16,7 @@ LOCALSTATEDIR := /var

 SOURCES := $(shell find . 2>&1 | grep -E '.*\.(c|h|go)$$')
 PROXY_SOCKET := $(LOCALSTATEDIR)/run/clear-containers/proxy.sock
+STORE_STATE_DIR := $(LOCALSTATEDIR)/lib/clear-containers/proxy/


$(LOCALSTATEDIR)/run/clear-containers/proxy/ would be more appropriate since we're gonna store some data generated at runtime.

Yep, I agree. I'll change this. Thanks.

sboeuf · 2017-10-12T20:26:16Z

Makefile

@@ -53,7 +54,7 @@ all: cc-proxy $(UNIT_FILES)

 cc-proxy: $(SOURCES) Makefile
 	$(QUIET_GOBUILD)go build -i -o $@ -ldflags \
-		"-X main.DefaultSocketPath=$(PROXY_SOCKET) -X main.Version=$(VERSION)"
+		"-X main.DefaultSocketPath=$(PROXY_SOCKET) -X main.Version=$(VERSION) -X main.storeStateDir=$(STORE_STATE_DIR)"


I guess you could fix the other variables as they don't need to be upper case since they don't need to be exported.

I'd like not to mix other refactoring in one patch if you don't mind. But if you insist, I could write a separate patch and make it as a part of this PR though.

sboeuf · 2017-10-12T20:43:58Z

state.go

+
+// Returns false if it's a clean start (i.e. no state is stored) or restoring
+// failed.
+func (proxy *proxy) restoreAllState() bool {


I think we should make the difference between the three cases: clean start / restoration succeeded / restoration failed. IMO, that's the caller responsibility to decide what to do with the different output combinations.

func (proxy *proxy) restoreAllState() (bool, error) {

Or we could have one function to detect if we are in a clean start case by checking if we have something in .../run/clear-containers/proxy/..., and another function for the actual restoration.

BTW, you should not define methods belonging to the proxy object outside of proxy.go.

I'll rework this function.

sboeuf · 2017-10-12T20:44:52Z

proxy.go

@@ -622,12 +632,14 @@ func (proxy *proxy) init() error {
 	// Force a coredump + full stacktrace on internal error
 	debug.SetTraceback("crash")

-	// flags
-	proxy.enableVMConsole = logrus.GetLevel() == logrus.DebugLevel
+	if !proxy.restoreAllState() {


Not clear enough. See my comment about restoreAllState() function.

OK, thanks. I'll rework this.

sboeuf · 2017-10-12T21:07:17Z

state.go

+}
+
+func vmStateFilePath(id string) string {
+	return storeStateDir + "vm_" + id + ".json"


Using path/filepath package is usually clearer/cleaner when creating/modifying a path.

filepath.Join(storeStateDir, "vm_" + id + ".json")

Thank you for the hint! I'll rework this function as you suggested.

sboeuf · 2017-10-12T21:26:20Z

state.go

+	Tokens     []string       `json:"tokens"`
+}
+
+func logContID(containerID string) *logrus.Entry {


This is very generic, it should go into log.go or utils.go, but I don't think it should stay here.

I'm OK to move it to a distinct file, but maybe it'd be better to do this as a separate refactoring PR? If you insist I can rework this as a part of my PR.

sboeuf · 2017-10-12T21:27:28Z

state.go

+}
+
+// On success returns nil, otherwise an error string message.
+func (proxy *proxy) restoreTokens(vm *vm, tokens []string) error {


You should not define methods belonging to the proxy object outside of proxy.go.

I'll change this.

sboeuf · 2017-10-12T21:33:43Z

state.go

+		}
+
+		proxy.Lock()
+		proxy.tokenToVM[token] = &tokenInfo{


Yes fine with me.

sboeuf · 2017-10-12T21:44:05Z

state.go

+
+	storeFile := vmStateFilePath(vm.containerID)
+
+	err = ioutil.WriteFile(storeFile, o, proxyStateFilesPerm)


Please use the format

if err := ...; err != nil {

No problem, as you wish ;)

sboeuf · 2017-10-12T21:47:57Z

state.go

+}
+
+// On success returns nil, otherwise an error string message.
+func storeVMState(vm *vm, tokens []string) error {


Actually, I think you should define a storeState() and fetchState() methods both for vm and proxy. That way, it would be more consistent with the overall naming of those new functions that you're introducing.

Originally the code was in proxy.go but @jodh-intel asked me to move it out to a separate file. If I define the methods on vm and proxy then I have to move them to proxy.go and vm.go back according to what you ask :)

dvoytik · 2017-10-13T19:16:29Z

Hi @sboeuf, thank you for the review!

From my understanding, you basically STORE during RegisterVM() and RESTORE during UnregisterVM().

No, restoring is done only when proxy starts and detects a stored state on disk. During unregisterVM() we call delVMAndState(), which in turn updates proxy's state and deletes the requested vm's state on disk.

The issue is that you're not covering all cases here. You will miss all the tokens/sessionID created during a pod lifecycle. For instance, after the VM has been started and that we have stored all the info, if we create new containers/processes, new tokens/sessionIDs are gonna be generated by the proxy. Am I missing something or this is something expected ? Cause that means that we are not covering a lot of crashing cases otherwise.

Tokens are stored in a state. Please see vmStateOnDisk.Tokens.

sboeuf · 2017-10-13T19:28:26Z

No, restoring is done only when proxy starts and detects a stored state on disk. During unregisterVM() we call delVMAndState(), which in turn updates proxy's state and deletes the requested vm's state on disk.

Yes sorry I used wrong explanation, but basically you're saving things only at the RegisterVM() time, and you restore when the proxy get restarted and it goes through init().

Tokens are stored in a state. Please see vmStateOnDisk.Tokens.

Yes sure, but you're doing the STORE only when the VM is registered. And according to a full pod lifecycle, you're gonna miss almost everything. I mean that you're only be able to RESTORE the very first configuration that you saved when the VM has been created... Lots of containers and process could be pending at the time the proxy had crashed, and you would miss all of that. I think that you need to store in lot more places in the code. Does that make sense ?

dvoytik · 2017-10-13T20:02:37Z

Yes sure, but you're doing the STORE only when the VM is registered. And according to a full pod lifecycle, you're gonna miss almost everything. I mean that you're only be able to RESTORE the very first configuration that you saved when the VM has been created... Lots of containers and process could be pending at the time the proxy had crashed, and you would miss all of that. I think that you need to store in lot more places in the code. Does that makes sense ?

Oops, actually you are right! I've just checked again all API methods and totally forgot about attachVM(). Originally in my previous versions it was there and somehow disappeared :) I remember I was testing attaching manually and it worked.
If you can find other places where I've missed storing/updating something please let me know. Thanks!

dvoytik · 2017-10-15T20:00:35Z

@sboeuf, thank you for the review again!
I've updated the PR.

jodh-intel · 2017-10-24T09:11:20Z

@sboeuf - ping.

devimc · 2017-10-24T16:27:53Z

Hi All
I think the best way to avoid restoring proxy's state is a redesign in the way we connect cc-shims and the container, we can hot plug virtio serial ports and associate them directly with cc-shim when a new container/workload is created, cc-proxy will be only used to negotiate with the agent (create, stop, delete containers, etc). That mean 1 serial port to control the container (CTL) and N serial ports to communicate cc-shim and workload (TTY)

Currently:

cc-shim <->  cc-proxy <-> TTY serial port <-> cc-agent <-> workload

With a redesign:

cc-shim <-> TTY N serial port <-> cc-agent <-> workload

in that way containers will not be affected if the cc-proxy dies since part of the logic of the cc-proxy will be moved to cc-agent

sboeuf · 2017-10-24T17:34:52Z

@devimc that could work, sure. But the drawback of such a solution is that we would get a larger attack surface because you would create as much serial ports as we have processes.

sboeuf · 2017-10-25T14:47:20Z

@jodh-intel @dvoytik I'll look into this today

sboeuf

I have noticed that you're missing to save all the container IDs (the one you can find into ioSession structure). We make a difference between the containerID in the vm structure which means the podID and the containerID in the ioSession structure which means a containerID. There is one session per container and one VM per pod.
I think that you need to figure out a way to save the containers related to each pod/vm, otherwise we could not be able to perform proper action like killing the container process after the proxy restarted.
Take a look here: https://github.com/clearcontainers/proxy/blob/master/vm.go#L363 and here: https://github.com/clearcontainers/proxy/blob/master/vm.go#L572-L581

sboeuf · 2017-10-26T06:03:46Z

state.go

@@ -0,0 +1,387 @@
+// Copyright (c) 2017 Dmitry Voytik <dmitry.voytik@huawei.com>


@dvoytik have you made the change here ?

sboeuf · 2017-10-26T06:13:54Z

state.go

+		if err := os.Remove(proxyStateFilePath); err != nil {
+			return fmt.Errorf("couldn't remove file %s: %v",
+				proxyStateFilePath, err)
+		}


According to your logic, if there's no VM, we should not have a state file, I am fine with that, but this means you should return nil after you properly removed the file, right ?

Good catch! Thank you. I'll add here return nil.

sboeuf · 2017-10-26T06:16:55Z

state.go

+		Version:         stateFileFormatVersion,
+		SocketPath:      proxy.socketPath,
+		EnableVMConsole: proxy.enableVMConsole,
+		ContainerIDs:    make([]string, 0, len(proxy.vms)),


You don't need to allocate dynamically the size of the buffer. Calling into append() will do that for you.

Yeah, I know that. It looks like a premature optimization, but on the other hand does it really hurt?

Well I don't consider this as an optimization since you could directly remove this line.

If you insist, I'll remove this line, no problem. My intention was to avoid re-allocations on each append operation.

sboeuf · 2017-10-26T06:28:22Z

state.go

+// On success returns nil, otherwise an error string message.
+func storeVMState(vm *vm) error {
+	vm.Lock()
+	tokens := make([]string, 0, len(vm.tokenToSession))


Does not need dynamic allocation since you're using append right after.

dvoytik · 2017-10-27T20:09:35Z

@sboeuf, thank you again for the review.
You are right about ioSession. I'll take a look how to store/restore this state in coming days.

sboeuf · 2017-10-27T20:20:08Z

@dvoytik no problem ! Let me know when you have something ready.

Introduce the high availability feature of cc-proxy by implementing store/restore of proxy's state to/from disk. This feature depends on the ability of shim to reconnect to cc-proxy if connection is lost. Fixes clearcontainers#4. Signed-off-by: Dmitry Voytik <dmitry.voytik@huawei.com>

dvoytik · 2017-11-08T08:34:55Z

Hi @sboeuf, could you please take a look?

dvoytik force-pushed the store_restore_state branch 3 times, most recently from abbbdb5 to 7a2f972 Compare August 21, 2017 11:11

clearcontainers deleted a comment from clearcontainersbot Aug 23, 2017

jodh-intel suggested changes Aug 31, 2017

View reviewed changes

dvoytik mentioned this pull request Oct 2, 2017

connect: Try to re-connect to proxy clearcontainers/shim#54

Merged

dvoytik force-pushed the store_restore_state branch 3 times, most recently from d703819 to 3e0c350 Compare October 5, 2017 21:38

jodh-intel suggested changes Oct 6, 2017

View reviewed changes

jodh-intel mentioned this pull request Oct 6, 2017

Create a high availability design proposal doc clearcontainers/runtime#683

Open

dvoytik force-pushed the store_restore_state branch 2 times, most recently from 9673679 to c2a705c Compare October 9, 2017 21:21

jodh-intel reviewed Oct 10, 2017

View reviewed changes

jodh-intel reviewed Oct 11, 2017

View reviewed changes

dvoytik force-pushed the store_restore_state branch 2 times, most recently from 97a65e3 to 11d2371 Compare October 11, 2017 21:55

sboeuf suggested changes Oct 12, 2017

View reviewed changes

dvoytik force-pushed the store_restore_state branch from 11d2371 to 885e85b Compare October 15, 2017 19:22

sboeuf suggested changes Oct 26, 2017

View reviewed changes

dvoytik force-pushed the store_restore_state branch 4 times, most recently from b932d09 to e7ebeda Compare November 7, 2017 21:03

dvoytik force-pushed the store_restore_state branch from e7ebeda to b4df8d5 Compare November 7, 2017 21:03

dvoytik mentioned this pull request Nov 13, 2017

Use one proxy per VM clearcontainers/runtime#795

Open

7 tasks


		storeFile := vmStateFilePath(vm.containerID)

		err = ioutil.WriteFile(storeFile, o, proxyStateFilesPerm)

		@@ -0,0 +1,387 @@
		// Copyright (c) 2017 Dmitry Voytik <dmitry.voytik@huawei.com>

implement (re)store of proxy's state #107

Are you sure you want to change the base?

implement (re)store of proxy's state #107

Conversation

dvoytik commented Aug 18, 2017

sboeuf commented Aug 18, 2017

dvoytik commented Aug 21, 2017

coveralls commented Aug 21, 2017 • edited Loading

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

Choose a reason for hiding this comment

dvoytik Oct 3, 2017 • edited Loading

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

dvoytik Oct 3, 2017 • edited Loading

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

jodh-intel commented Aug 31, 2017

dvoytik commented Sep 1, 2017

jodh-intel commented Sep 1, 2017

dvoytik commented Sep 1, 2017

jodh-intel commented Sep 29, 2017

dvoytik commented Oct 2, 2017

jodh-intel commented Oct 2, 2017

dvoytik commented Oct 2, 2017

dvoytik commented Oct 5, 2017 • edited Loading

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

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

jodh-intel commented Oct 6, 2017

dvoytik commented Oct 6, 2017 • edited Loading

jodh-intel commented Oct 9, 2017

dvoytik commented Oct 9, 2017

jodh-intel commented Oct 10, 2017

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

jodh-intel commented Oct 10, 2017

sboeuf commented Oct 10, 2017

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

sboeuf commented Oct 12, 2017

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 21, 2017 •

edited

Loading

dvoytik Oct 3, 2017 •

edited

Loading

dvoytik Oct 3, 2017 •

edited

Loading

dvoytik commented Oct 5, 2017 •

edited

Loading

dvoytik commented Oct 6, 2017 •

edited

Loading

dvoytik Oct 13, 2017 •

edited

Loading