fix unmarshal container config failure with Docker 1.8.3 #990
Conversation
|
Can one of the admins verify this patch? |
|
Looks good but needs a couple of tests if possible with configs from < 1.8, 1.8.3 & >=1.9 just to check we haven't broken anything. |
|
Yeah, I will do more tests to make sure there is no any regression. Thanks |
| @@ -159,8 +195,22 @@ func ReadConfig(dockerRoot, dockerRun, containerID string) (*configs.Config, err | |||
|
|
|||
| var state libcontainer.State | |||
| err = json.Unmarshal(out, &state) | |||
| if err != nil { | |||
| return nil, err | |||
| if err != nil && strings.Contains(err.Error(), "json: cannot unmarshal") { | |||
There was a problem hiding this comment.
Should we check the type of this is a json.UnmarshalTypeError instead of checking the string contents?
|
I'm wondering if we could just drop the throttled devices for now as we don't use that config AFAIK so processing for no need. I'm happy to do that & put that in a comment unless you can think of a reason why not to do that. |
|
When I implemented this, I also want to drop these throttled devices, but I am afraid I missed something. So I will remove those codes and put the new changes into a new function. Thanks, |
|
I updated the changes, and tested it with docker 1.8.2, 1.8.3 and 1.9.1. All of them works well. Could you please review them again? |
| } | ||
|
|
||
| // For now, we do not need these ThrottleDevices | ||
| newCgroup.BlkioThrottleReadBpsDevice = []*configs.ThrottleDevice{} |
There was a problem hiding this comment.
These are all redundant assignments aren't they?
There was a problem hiding this comment.
+1. I'd expect higher layers to be resilient to nil values.
|
Minor nits. Really need some tests now just to confirm it's fixed & compatible. |
|
ok to test |
I also think so, it will be better to have more integration test cases for different docker version. |
|
We have integration tests for multiple versions right now, this is why most PRs are broken now... Could you please add some unit tests & then we can merge? Thanks very much for this - very much appreciated. |
|
Sure, I will try to add some unit tests on this. |
|
Thank you! |
| @@ -144,6 +144,40 @@ type preAPINetwork struct { | |||
| TxQueueLen int `json:"txqueuelen,omitempty"` | |||
| } | |||
|
|
|||
| type oldCgroup struct { | |||
There was a problem hiding this comment.
nit: old doesn't really mean anything once we face this issue again.
Shall we rename this to v1Cgroup or something similar to that? Let's also add a few comments describing the versions of docker this version is compatible with.
There was a problem hiding this comment.
Yeah, I will rename it and add some comments in next commit with the unit test.
|
Thanks for the fix @carmark ! |
|
Made some changes based on your comments and add the basic unit tests. I am not sure why the |
|
Flaky test |
|
retest this please |
1 similar comment
|
retest this please |
|
LGTM |
fix unmarshal container config failure with Docker 1.8.3
|
Thanks @carmark! |
fix #989
Test with docker 1.8.3, it works well.