Skip to content

Windows: Refactor container#12884

Merged
icecrime merged 1 commit intomoby:masterfrom
microsoft:10662-startcontainerrefactor
May 19, 2015
Merged

Windows: Refactor container#12884
icecrime merged 1 commit intomoby:masterfrom
microsoft:10662-startcontainerrefactor

Conversation

@lowenna
Copy link
Member

@lowenna lowenna commented Apr 29, 2015

Signed-off-by: John Howard jhoward@microsoft.com
@swernli

This PR is part of the proposal described in issue 10662 to port the docker daemon to Windows. This PR is one of the more complex PRs unfortunately, but many future PRs will depend on this PR, so hopefully this one can be merged relatively quickly :) This PR is the first step at forking container.go into Linux and Windows versions. In doing that, it also required forking volumes.go to get compilation and integration tests to succeed, so I have also pulled in some minor changes which were slated for future PRs too in volumes. Note that container_windows.go still contains TODOs which will be addressed in future PRs which depend on this PR (didn't want to make a huge PR affecting dozens of files).

(Rebased 4/30 after engine removed)

@crosbymichael
Copy link
Contributor

LGTM

daemon/daemon.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhowardmsft why this change? I actually prefer the way it was :S

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but there's a good reason. When I split out the Container structure into common and platform specific parts, everything is good in go except for the constructor "container:=&Container{field: value, etc}.". Accessors are fine, but the constructor will fail compilation. Hence the need to rework it to be field by field. Sucks, I know. As far as I can tell, it's a limitation in go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh I see, what you need is this:

container := &Container{
    CommonContainer: CommonContainer{
        State: NewState(),
    },
    root: daemon.containerRoot(id),
    execCommands: newExecStore(),
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh, lightbulb moment. That makes sense. I just learnt more about go. Will verify and address in the morning, Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fixed. Thanks for the pointer in the right direction :)

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch 5 times, most recently from 9b02025 to 5a27719 Compare May 1, 2015 18:13
@lowenna
Copy link
Member Author

lowenna commented May 1, 2015

@tiborvass Fixed constructors. Thanks for the pointer to the right syntax.

@tiborvass
Copy link
Contributor

@jhowardmsft awfully sorry but needs a rebase

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch from 5a27719 to ccdac75 Compare May 4, 2015 22:50
@lowenna
Copy link
Member Author

lowenna commented May 4, 2015

Rebased

@jessfraz
Copy link
Contributor

jessfraz commented May 7, 2015

ah so sorry needs rebase

@jessfraz
Copy link
Contributor

jessfraz commented May 7, 2015

you can ping me after :)

@lowenna
Copy link
Member Author

lowenna commented May 8, 2015

Again??! Wow, things change fast around here :)

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch from ccdac75 to 1383510 Compare May 8, 2015 00:03
@lowenna
Copy link
Member Author

lowenna commented May 8, 2015

Rebased

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch 2 times, most recently from d078347 to 25049b6 Compare May 8, 2015 00:22
@lowenna lowenna closed this May 12, 2015
@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch from 3effc23 to cacf90c Compare May 12, 2015 02:34
@lowenna lowenna reopened this May 12, 2015
@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch 3 times, most recently from 97e9966 to a0fe49b Compare May 12, 2015 02:58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that -v something:ro will be unsupported on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the initial release, yes that is the current plan.

@unclejack
Copy link
Contributor

@jhowardmsft Sorry, this requires rebasing again.

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch 4 times, most recently from de1453f to d38055d Compare May 14, 2015 15:42
@lowenna
Copy link
Member Author

lowenna commented May 14, 2015

@unclejack Rebased. Waiting for Jenkins to complete.

@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch 7 times, most recently from 42ac3fb to 973504d Compare May 16, 2015 19:27
Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna lowenna force-pushed the 10662-startcontainerrefactor branch from 973504d to b9e4b95 Compare May 16, 2015 19:38
@icecrime
Copy link
Contributor

LGTM

icecrime pushed a commit that referenced this pull request May 19, 2015
@icecrime icecrime merged commit 0cc5da0 into moby:master May 19, 2015
@lowenna lowenna deleted the 10662-startcontainerrefactor branch May 28, 2015 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants