implement (re)store of proxy's state

[dvoytik]

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 [email protected]

[sboeuf]

@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 https://github.com/clearcontainers/shim/pull/54 and https://github.com/clearcontainers/proxy/pull/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]

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]

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

[jodh-intel]

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]

@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]

Hi @dvoytik - no problem! And enjoy your holiday! :sunglasses:

[dvoytik]

@jodh-intel, thanks! :smiley:

[jodh-intel]

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]

@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]

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

[dvoytik]

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]

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

[jodh-intel]

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 :smile:

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]

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]

Thanks @dvoytik! ;)

[dvoytik]

The PR has been updated.

[jodh-intel]

Thanks @dvoytik - reviewing and testing with https://github.com/clearcontainers/shim/pull/54 ...

[jodh-intel]

This is looking very nice.

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

[sboeuf]

@jodh-intel sure I'll take a look

[sboeuf]

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

[dvoytik]

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]

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]

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]

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

[jodh-intel]

@sboeuf - ping.

[devimc]

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]

@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]

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

[dvoytik]

@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]

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