New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the monolith #33022

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@cpuguy83
Copy link
Contributor

cpuguy83 commented May 4, 2017

This moves the old engine code into /monolith.
Part of #32871

api, pkg, and client remain at the top level since these are packages that are used by external consumers.

Have not touched cli yet since this is changing significantly with #32694

This does move the Makefile into monolith/, which will most certainly break existing workflows, and most likely CI, however tests and everything can be run from monolith/

TODO:

  • Fix moby logo in README.md
  • Remove CLI components (dependent on #32694)
  • Fix Windows CI
@aaronlehmann

This comment has been minimized.

Copy link
Contributor

aaronlehmann commented May 4, 2017

Maybe engine would be a more descriptive directory name.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 5, 2017

I don't think engine is very descriptive at all, it has very little meaning.
In addition some new component could potentially be called "engine" thus making it confusing.
"monolith" is very much not confusing, we know what it is and that is going away.

If the goal of moby is to split up the monolith, and we have a dir called "monolith", it's pretty simple to tell where the work lies.

To all, let's please not bike-shed on the naming here. Ultimately this is going away.

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch 4 times, most recently from 1e73fa6 to 83600cf May 5, 2017

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented May 5, 2017

README.md needs to be on the root

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented May 5, 2017

daemon/graphdriver/devmapper can be moved to the monolith directory

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch from 83600cf to 7a18dc8 May 5, 2017

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 5, 2017

@AkihiroSuda Thanks!

I've added symlinks to the real Dockerfiles under monolith/ at the root of the repo. This is so CI can continue to work as apparently CI does not use Makefile to build.
I also added a dummy Makefile at the root which just passes everything through to monolith/
Once this is merged we can update the CI to use the new paths and then remove them from the project root.

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented May 5, 2017

Moby logo in README broken 😅

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 5, 2017

Added a checklist of things to fix, which is currently quite small, feel free to update as you notice issues.

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch 2 times, most recently from e57555a to 86b5eae May 6, 2017

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 6, 2017

Fixed logo, updated with docker/cli changes.

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch 3 times, most recently from dc1e638 to 61c26c8 May 6, 2017

@cpuguy83 cpuguy83 changed the title [WIP] Move the monolith Move the monolith May 6, 2017

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 8, 2017

ping @jhowardmsft Could use some help with the Windows CI.
Assuming the Windows CI doesn't like the symlink I put in for Dockerfile.windows.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 8, 2017

powerpc and z are failing for the same reason.
Sadly we don't get any archives of the daemon logs on these.

09:29:13 FAIL: docker_cli_daemon_test.go:2831: DockerDaemonSuite.TestRemoveContainerAfterLiveRestore
09:29:13 
09:29:13 [de8ef8ec90e68] waiting for daemon to start
09:29:13 [de8ef8ec90e68] exiting daemon
09:29:13 docker_cli_daemon_test.go:2833:
09:29:13     s.d.StartWithBusybox(c, "--live-restore", "--storage-driver", "overlay")
09:29:13 daemon/daemon.go:198:
09:29:13     t.Fatalf("Error starting daemon with arguments: %v", args)
09:29:13 ... Error: Error starting daemon with arguments: [--live-restore --storage-driver overlay]

EDIT:
Adding some extra logging to the test daemon startup to deal with this.

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch from 61c26c8 to a0089b3 May 8, 2017

@jhowardmsft

This comment has been minimized.

Copy link
Contributor

jhowardmsft commented May 8, 2017

@cpuguy83 I'm slammed between the Linux containers on Windows bring-up and containerd. Perhaps @johnstep has time to look. However, if the symlink is in the source tree itself, that won't work at all on Windows - git won't clone them.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 8, 2017

We discussed just keeping two copies of Dockerfile.windows for a (very) short period... long enough to change CI over to use the new Dockerfile path.

WDYT?

@jhowardmsft

This comment has been minimized.

Copy link
Contributor

jhowardmsft commented May 8, 2017

To be clear, the use of ANY symlinks in a git repo mean that the clone won't work correctly on Windows. This needs to be solved without any symlinks in the repo.

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch 3 times, most recently from ddc24ee to c213fab May 8, 2017

Moves monolith to monolith/
Leaves copies of Dockerfiles at the root (real files in monolith/) to
make sure CI works.
Also provides a fallthrough Makefile at the root that just passes
everything to monolith/Makefile.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch 16 times, most recently from 016e5cf to 015c276 May 8, 2017

Add tmp validation to check updated Dockerfiles
We need to temporarily have two copies of certain files while
transitioning to `monolith/`.
This script ensures that the copies stay in sync.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

@cpuguy83 cpuguy83 force-pushed the cpuguy83:move_the_monolith branch from 015c276 to 9559ebc May 8, 2017

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 8, 2017

Running into some issues with Windows CI expecting paths to be under the root (like docker.exe, dockerd.exe) instead of monolith/.
These two in particular are fairly easy to work-around by copying the binaries from monolith/bundles to bundles/.

dockerversion/version_autogen.go is slightly more complicated.

@alexellis

This comment has been minimized.

Copy link
Contributor

alexellis commented May 9, 2017

Trying to catch up on the naming of everything. What does a folder called Monolith actually mean?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jun 8, 2017

Closing based on discussions on the forums it seems like we will just move the engine out of the repo.

@cpuguy83 cpuguy83 closed this Jun 8, 2017

@cpuguy83 cpuguy83 deleted the cpuguy83:move_the_monolith branch Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment