ddev rm, describe, and stop are now reliable even when a site directory is missing; fixes #459 #460

nmccrory · 2017-09-14T22:13:15Z

The Problem/Issue/Bug:

ddev rm, describe, and stop were unable to remove sites that were missing their config.yml or sites that did not have directories associated with them. This made it impossible to cleanup corrupted sites pointed out by ddev list.

How this PR Solves The Problem:

This PR solves the problem by using a site's containers to grab it's site-name and using that site-name to run Cleanup() on the site. It also checks for a missing directory and if the approot is missing it skips app.Down() and uses Cleanup().

Manual Testing Instructions:

Test this PR using the following steps:

Start a site using ddev start
rm -r the site directory
ddev list the site to see that it reports its status as app directory missing:
Use ddev rm <sitename> to remove the site

Automated Testing Overview:

Tests for ddev rm are run in TestCleanupWithoutCompose. Tests for ddev describe are run in TestDescribeMissingDirectory. And tests for ddev stop are run in TestLocalStopMissingDirectory.

Release/Deployment notes:

beeradb · 2017-09-14T22:27:05Z

I'll give this a more thorough review in the AM, but at first glance, I would love it if this could be consolidated into the Down() method rather than living in the cmd for the following reasons:

As a general rule, we try to keep as much logic outside of things in the cmd directory as possible. We don't always achieve this, but the idea is that the cmd directory is just for defining commands and their flags + arguments and then passing the work along to our packaged code.
The conditional here is important logic for Down() to work correctly. We'd have to do the same checks anywhere we tried to call Down() from. With that in mind, it's probably better to just include it in that method and ensure Down() is as robust as it can be.

To be clear, I mean this code: https://github.com/drud/ddev/pull/460/files#diff-5c1b23b6448d5099a5a94f5b2b0685f2R43

rfay

Just leaving my comments for now. Will look again after tests pass.

As mentioned in slack, "To make things easier in the future (but use with care) on a PR that exactly solves an issue, you can add "fixes #xxx" to the title. So #460 will exactly fix #459, so please add "fixes #459" to the PR title. That way it will auto-close or at least if the person who's pulling makes sure that makes it into the commit message it will."

rfay · 2017-09-14T22:16:51Z

pkg/plugins/platform/utils.go

-	case strings.Contains(status, SiteDirMissing):
-		status = color.RedString(status)
-	case strings.Contains(status, SiteConfigMissing):
-		status = color.RedString(status)


Is the removal here a rebase failure?

rfay · 2017-09-14T22:20:27Z

pkg/plugins/platform/local.go

 	appRoot, err := CheckForConf(siteDir)
 	if err != nil {
-		return "", fmt.Errorf("unable to determine the application for this command. Have you run 'ddev config'? Error: %s", err)
+		// In the case of a missing .ddev/config.yml just return the site directory.
+		return siteDir, nil


What what about the situation where they do a ddev start and haven't done a ddev config in advance, which is where I think this came from? Is that still handled?

Yes because it will still get the app root from the running web container and return it by itself if it can't find a config.

This is the message shown with this PR in the situation @rfay describes (ddev start on dir that config has not been run for): "Failed to start: Could not find a site in /Users/tannerjfco/sites/test. Please specify a site name or change directories: no .ddev/config.yaml file was found in this directory or any parent" This may be ok enough, but we no longer suggest to run config. Maybe change to "Could not find a configured site?"

nmccrory · 2017-09-15T17:22:15Z

Still need to refactor some tests to work but this is ready for a second review @beeradb

beeradb · 2017-09-19T17:45:55Z

pkg/plugins/platform/local.go

+		if err != nil {
+			util.Failed("Failed to remove %s: %s", l.GetName(), err)
+		}
+	} else if missingConfig != nil {


This conditional can just be combined with the one above it, as they both do the same thing.

if !fileutil.FileExists(l.AppRoot()) || missingConfig != nil { ... }

I might also suggest configErr over missingConfig. The latter really reads like it should be a bool.

beeradb · 2017-09-19T17:50:49Z

pkg/plugins/platform/local.go

+	if !fileutil.FileExists(l.AppRoot()) {
+		err := Cleanup(l)
+		if err != nil {
+			util.Failed("Failed to remove %s: %s", l.GetName(), err)


This results in a really verbose message when running ddev rm in a directory where a config doesn't exist, example:

"Failed to remove: Could not find a site in /Users/beeradb/Projects/go/src/github.com/drud/ddev. Please specify a site name or change directories: no .ddev/config.yaml file was found in this directory or any parent"

It feels like we could simplify that a lot.

Also, "%v" is the preferred Printf token for errors.

beeradb · 2017-09-19T18:03:21Z

pkg/plugins/platform/local.go

@@ -812,13 +812,26 @@ func (l *LocalApp) Config() error {
 func (l *LocalApp) Down(removeData bool) error {


I'm a little uncomfortable with how much nesting and complexity we have in this function now. There are a lot of paths through this function, and along with those paths, lots of room for bugs. It's also much harder to maintain than I would like.

Here are a couple competing ideas for how we might go about making this more maintainable:

We split out the removeData portion into its own method. I'm not entirely convinced that Down() should handle that anyway, as it's arguably separate concern from stopping/removing the containers. Doing this would allow us to simplify the conditionals at the top to something more like the following:

if !fileutil.FileExists(l.AppRoot()) || missingConfig != nil { return Cleanup(l) } err := dockerutil.ComposeCmd(l.ComposeFiles(), "down", "-v") if err != nil { util.Warning("Could not stop site with docker-compose. Attempting manual cleanup.") return Cleanup(l) }

We could consider just always calling Cleanup() regardless of whether a docker-compose file exists or not. At this point I don't believe we are using any volumes, so removing a site is just a matter of stopping and removing the containers. If we think we can trust the Cleanup() method then it would definitely simplify our code to just rely on it.

I would love to use the "just call Cleanup()" technique. I see no downside, and no reason that we should have been doing other things. The reason we do the dockerutil.ComposeCmd("down") is just that the code was always there. Now we have better code, let's use it instead.

+1 for split removeData into own function.

nmccrory · 2017-09-19T21:07:34Z

Today I'll be pushing the changes requested in Brad's review however tomorrow I would really benefit from a paired programming session to help write tests for this. Any time for this? @beeradb @tannerjfco

rfay · 2017-09-19T21:30:32Z

Originally I thought this bug was too niche to deserve a test. But then there was an accident toward the end of Nick's work in which the tests still worked but the basic premise of the PR stopped working. Reminded me how valuable tests really are. So I suggested to Nick to go ahead and add a test in pkg that just starts a single site, rm -rf the codebase, then check that it can describe, then stop, and rm.

rfay

I had a catastrophic experience testing this manually, my current directory was completely destroyed. I restored and repeated the experience. I had used the make testcmd approach to getting a dummy ddev site going, and ctrl-c to stop it, then removed the directory where the site was:

rfay@Randy-Air:~/go/src/github.com/drud/ddev$ ddev list
2 local sites found.
NAME                  TYPE       LOCATION                                                                                URL                                     STATUS
TestMainCmdWordpress  wordpress  /private/var/folders/db/741gx8tj20d5z1f3zjpvw2vh0000gp/T/TestMainCmdWordpress954110703  http://TestMainCmdWordpress.ddev.local  app directory missing: /private/var/folders/db/741gx8tj20d5z1f3zjpvw2vh0000gp/T/TestMainCmdWordpress954110703
gchistory             drupal7    ~/workspace/gchistory                                                                   http://gchistory.ddev.local             running

DDEV ROUTER STATUS: running
rfay@Randy-Air:~/go/src/github.com/drud/ddev$ ddev rm --remove-data TestMainCmdWordpress
Stopping container: ddev-TestMainCmdWordpress-web
Stopping container: ddev-TestMainCmdWordpress-dba
Stopping container: ddev-TestMainCmdWordpress-db
Removing container: ddev-TestMainCmdWordpress-web
Removing container: ddev-TestMainCmdWordpress-dba
Removing container: ddev-TestMainCmdWordpress-db
Failed to remove TestMainCmdWordpress: failed to remove data directories: remove .: invalid argument

After completion, the entire current directory I was working in, which happened to be drud/ddev, was empty. I restored it with time machine, then tried it again, but same result. Obviously this is not what we want.

rickmanelius · 2017-09-26T22:12:16Z

I added myself to the reviewer list because data loss is nothing to mess around with and I'd love to manually test this alongside Randy to ensure this is buttoned up.

rickmanelius · 2017-09-26T23:13:01Z

Reset the testers now that the tests passed on circle. Going to try and recreate the issue @rfay surfaced.

rickmanelius · 2017-09-26T23:44:44Z

Initial Testing

Preparation

Remove homebrew install. brew uninstall ddev
Get commit from nmccrory.
- Add remote: git remote add nmccrory git@github.com:nmccrory/ddev.git
- Fetch git fetch --all
- Checkout git checkout nmccrory/20170912-ddev-rm
Build make clean && make darwin
Confirmed ddev version reports v0.9.1-43-g2756d26f

Testing

Active

Catastrophic scenario that Randy reported.
- Setup a WordPress site using https://github.com/drud/wordpress.
- Remove directory rm -r ~/git/wordpress
- pwd shows ~/git/ddev
- ddev list confirms wordpress is at status app directory missing: /Users/rmanelius/git/wordpress
- running ddev rm wordpress2 shows the containers being stopped and then removed.
- I verified the working directory was still in tact.

TODO

All other scenarios in scope of the parent issue, including rm, describe, and stop.

rickmanelius · 2017-09-27T03:03:39Z

Testing

Confirmed

rm
- Started a fresh wordpress site.
- Confirmed site in ddev list
- Move directory cd
- Remove directory rm -rf ~/git/wordpress
- ddev list confirms wordpress is at status app directory missing: /Users/rmanelius/git/wordpress
- ddev rm wordpress stop and removes the containers
stop
- Same prep steps as rm above
- ddev stop wordpress shows a warning and prompts to run ddev rm wordpress although it doesn't wrap that in quotes.
describe
- Same prep steps as rm above
- ddev describe wordpress notes the status is "app directory missing: /Users/rmanelius/git/wordpress"

rickmanelius

My manual testing worked, including the data loss scenario that @rfay identified. If he also gives a thumbs up, this is ready to merge.

rfay

Needs to use GetGlobalDdevDir(), especially for cross-platform predictability.
I'd rather see AppConfig get built right in the first place if that's possible.
I think it's really important to fix the shortsighted if !fileutil.FileExists(dir) { that allowed this to happen. We can at the very least check to see if dir is not empty; I kind of think that logic is pretty shortsighted though and can be structured better.

rfay · 2017-09-27T04:05:37Z

pkg/plugins/platform/local.go

+		if fileutil.FileExists(l.AppConfig.AppRoot) {
+			dir = filepath.Dir(l.AppConfig.DataDir)
+		} else {
+			user, err := user.Current()


This should all be using GetDdevGlobalDir(), don't reinvent the wheel.

rfay · 2017-09-27T04:10:33Z

pkg/plugins/platform/local.go

+		// If it does not exist, manually set the data directory to remove.
+		var dir string
+		if fileutil.FileExists(l.AppConfig.AppRoot) {
+			dir = filepath.Dir(l.AppConfig.DataDir)


Another way to deal with this is to make sure that AppConfig.DataDir was already set correctly. I'd like that better. Could we push this farther up, to make sure that AppConfig gets populated a bit better? We're building it, right? So we can build it right? I think the question is about why it didn't get set right in the first place.

rfay · 2017-09-27T21:20:29Z

pkg/plugins/platform/local.go

+		// Since the site may be used for Cleanup(), also check that AppConfig.DataDir is set.
+		// This ensures that the sites data can be removed with the containers.
+		if app.AppConfig.DataDir == "" {
+			dataDir := fmt.Sprintf("%s/%s", util.GetGlobalDdevDir(), app.AppConfig.Name)


Is siteName aka AppConfig.Name guaranteed to be populated and not empty? If it were somehow empty seems like we might delete ~/.ddev ?

I could see it potentially being empty if someone were to run ddev rm by itself with no site-name argument in a directory with a deleted .ddev/config.yml. Not quite sure how that would play out.

Simply because if they didn't give a site-name then we wouldn't even be able to use GetActiveAppRoot in the first place so it would likely error out.

nmccrory · 2017-09-27T21:55:25Z

pkg/plugins/platform/local.go

+		_, err = CheckForConf(siteDir)
+		if err != nil {
+			return "", fmt.Errorf("Could not find a site in %s. Please specify a site name or change directories: %s", siteDir, err)
+		}


If no site name is given to ddev rm and the site's .ddev/config.yaml is missing, I have tested and confirmed that it will be caught here and properly error out telling the user to specify a site name.

Yeah, please check in the code to make sure that siteName is never empty so there's no way we can construct a data directory of ~/.ddev

we check to see if siteName is empty inside of GetActiveAppRoot and handle it there.

rickmanelius · 2017-09-28T15:51:40Z

@rfay Wondering if this addresses your PR feedback.

rfay

There are these remaining things that are pretty important to me:

Consolidate the handling of Down into one thing, without if/then/else. Unless there's a reason not to. I thought we had discussed it and we could just use the container-approach to down and not do any if-then-else about the config dirs.
Please look at where the data removal happens and put some code there so it can never, ever delete the wrong thing. I'd say look at the front of the filepath and make sure it's GlobalDdevDir, and look a the back of the filepath and make sure it's something past the /.
Take a look at my comment about the removal of filepath on the datadir and confirm that that removal was intentional. (Edit: I looked and it seems legit, but looks like it was sketchy before).

Let's make absolutely sure that we don't remove the user's home directory. Or ~/.ddev, or the current directory. Our --remove-data is a bit too powerful for my liking right now.

rfay · 2017-09-28T19:31:36Z

pkg/plugins/platform/local.go

-		util.Warning("Could not stop site with docker-compose. Attempting manual cleanup.")
-		err = Cleanup(l)
+	if !fileutil.FileExists(l.AppRoot()) {
+		err := Cleanup(l)


Hmm, I thought we were going to unconditionally do Cleanup() and skip the extra if/then/else stuff here, and simplify the code. Let's do that please, unless there was some reason not to do that.

rfay · 2017-09-28T19:33:42Z

pkg/plugins/platform/local.go

@@ -837,7 +854,8 @@ func (l *LocalApp) Down(removeData bool) error {
 				}
 			}
 		}
-		dir := filepath.Dir(l.AppConfig.DataDir)
+		// Convenience identifier
+		dir := l.AppConfig.DataDir


Just wondering why the filepath.Dir() was removed here? Is DataDir already a fully rooted path?

Wow, took a look and it seems to me that filepath.Dir() should never have been used here right? That removes a level from the DataDir? Could end up removing the user's home directory?

filepath.Dir() was actually what was removing the tail end of the dataDir filepath because filepath.Dir() removes the last element of a filepath. For example if GlobalDdevDir was /Users/Nick/.ddev and I appended the site name to the end to make /Users/Nick/.ddev/site-name, filepath.Dir would trim that back to /Users/Nick/.ddev thus deleting every site's data. Very slippery slope and I agree that this situation needs to have an air tight seal.

rfay · 2017-09-28T19:39:08Z

pkg/plugins/platform/local_test.go

+func removeAllErrCheck(path string, assert *asrt.Assertions) {
+	err := os.RemoveAll(path)
+	assert.NoError(err)
+}


It looks like you had some trouble with os.RemoveAll() - Was that on windows testing on the test we had to move out? This code doesn't seem like it should be that necessary, although I don't think it does any harm.

This is the code that Brad and I use to cleanup the directory name changes that we make to mock a missing directory in TestLocalStopMissingDirectory, TestDescribeMissingDirectory, and CleanupWithoutCompose.

rfay · 2017-09-28T19:39:58Z

pkg/plugins/platform/types.go

@@ -17,7 +17,7 @@ const SiteNotFound = "not found"
 const SiteDirMissing = "app directory missing"

 // SiteConfigMissing defines the string used to denote when a site is missing its .ddev/config.yml file.
-const SiteConfigMissing = ".ddev/config.yml missing"
+const SiteConfigMissing = ".ddev/config.yaml missing"


Hmm, thought this would happen if .ddev was missing or docker_compose.yml was missing? Fine with me, don't think it's that important.

rfay · 2017-09-28T19:44:02Z

Oh and @beeradb please put this one on your docket for a revisit. I know you have commits in it, but still, needs your eyes. The big bug we uncovered has me spooked.

rickmanelius · 2017-09-29T19:48:09Z

Looks like the build failed.

…be used easily with defer

… function to site.Chdir()

…moves application data by manually setting the site's data directory if the site is missing its directory. Test coverage included.

…ites with missing .ddev/configs

…. Add validation for unsafe AppConfig.DataDir values. Down() uses Cleanup() in all cases.

…Head is equal to GlobalDdevDir as tests revealed it did not meet our needs.

…en if containers are gone

rfay · 2017-11-16T22:40:51Z

Handled @beeradb 's main comments, rebased against master. Will go with either implicit or explicit approval by @rickmanelius after tests are green.

rickmanelius

I'm going to go ahead and approve because I gave a previous thumbs up and we now have essentially approvals across the board. This is a longstanding issue that I don't want to see blocked/stale, and I intend to re-review on our next major release as per usual. Any nitpicks or edge case issues can be filed as subsequent tickets versus holding up this larger PR.

rfay · 2017-11-16T23:53:37Z

Thanks @nmccrory for all your work on this. It was far more ambitious than anybody expected, and you hung with it a long time. Hope it feels good that it's finally in!

nmccrory requested review from rfay and beeradb September 14, 2017 22:13

rfay reviewed Sep 15, 2017

View reviewed changes

nmccrory force-pushed the 20170912-ddev-rm branch from 2fa9856 to a5a3ade Compare September 18, 2017 17:30

beeradb reviewed Sep 19, 2017

View reviewed changes

nmccrory force-pushed the 20170912-ddev-rm branch from 8aa0354 to 0b6ef95 Compare September 21, 2017 00:32

nmccrory changed the title ~~ddev remove now removes sites with missing directories and/or configs~~ ddev rm, describe, and stop are now reliable even when a site directory is missing; fixes #459 Sep 25, 2017

rfay requested changes Sep 26, 2017

View reviewed changes

rickmanelius self-requested a review September 26, 2017 22:11

nmccrory force-pushed the 20170912-ddev-rm branch from ad32df5 to 2756d26 Compare September 26, 2017 22:55

rickmanelius requested a review from rfay September 26, 2017 23:12

rickmanelius approved these changes Sep 27, 2017

View reviewed changes

rfay requested changes Sep 27, 2017

View reviewed changes

rfay reviewed Sep 27, 2017

View reviewed changes

nmccrory commented Sep 27, 2017

View reviewed changes

rfay requested changes Sep 28, 2017

View reviewed changes

rfay self-assigned this Oct 2, 2017

nmccrory and others added 24 commits November 16, 2017 15:38

group cleanup for temp directories into their own function which can …

466bdaf

…be used easily with defer

[20170912-ddev-rm] group chdir functions together and add explanatory…

93e182f

… function to site.Chdir()

[20170912-ddev-rm] Use app.init instead of GetActiveApp

39bba98

[20170912-ddev-rm] move site start to before directory rename

491e337

[20170912-ddev-rm] ClearDockerEnv() before restarting site

c004109

Add conditional to skip TestCleanupWithoutCompose if running on Windows.

ed5de5c

Handle ddev removes when the --remove-data flag is set to true. It re…

dc34a1a

…moves application data by manually setting the site's data directory if the site is missing its directory. Test coverage included.

Use GetGlobalDdevDir() and set DataDir inside of GetActiveApp

df07d29

minor typo fix

945e765

Caught a spelling error in the siteStatusMissingConfig presented on s…

ec49ef6

…ites with missing .ddev/configs

Separate recently introduced checks into their own validation methods…

e4383ce

…. Add validation for unsafe AppConfig.DataDir values. Down() uses Cleanup() in all cases.

Add check for empty dataDir value and remove check to see if the path…

3ace989

…Head is equal to GlobalDdevDir as tests revealed it did not meet our needs.

Make app.Cleanup() and thus app.Down() more resilient so they work ev…

1ca0079

…en if containers are gone

Minor cleanup of utils.go after rebase

fe0c46d

Revert back to upstream/master constructContainerName() func name

d770de8

Make a few functions non-exported

4db3d51

Simplify Cleanup()

5fe5716

Fix tests broken by api/json merge

9245f68

Minor improvement to validateDataDirRemoval

191c011

Minor simplification of restoreApp

e5b9f04

Improve validateDataDirRemoval

84a7e30

Re-add prompt to get them to run ddev config

dd4543b

Add comment on why err is ignored on app.Init()

dcb3a28

Quote 'ddev remove' and use remove instead of rm in description

119ef48

rfay force-pushed the 20170912-ddev-rm branch from 8260e04 to 119ef48 Compare November 16, 2017 22:39

rickmanelius approved these changes Nov 16, 2017

View reviewed changes

rfay merged commit bdfbf02 into ddev:master Nov 16, 2017

rickmanelius added this to the v0.9.4 milestone Nov 17, 2017

		@@ -812,13 +812,26 @@ func (l *LocalApp) Config() error {
		func (l *LocalApp) Down(removeData bool) error {

ddev rm, describe, and stop are now reliable even when a site directory is missing; fixes #459 #460

ddev rm, describe, and stop are now reliable even when a site directory is missing; fixes #459 #460

Conversation

nmccrory commented Sep 14, 2017 • edited

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

beeradb commented Sep 14, 2017 • edited

rfay left a 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

nmccrory commented Sep 15, 2017

Choose a reason for hiding this comment

beeradb Sep 19, 2017 • edited

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

nmccrory commented Sep 19, 2017

rfay commented Sep 19, 2017

rfay left a comment

Choose a reason for hiding this comment

rickmanelius commented Sep 26, 2017

rickmanelius commented Sep 26, 2017

rickmanelius commented Sep 26, 2017

Preparation

Testing

Active

TODO

rickmanelius commented Sep 27, 2017

Testing

Confirmed

rickmanelius left a comment

Choose a reason for hiding this comment

rfay left a 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

nmccrory Sep 27, 2017 • edited

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

rickmanelius commented Sep 28, 2017

rfay left a comment • edited

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

nmccrory Sep 28, 2017 • edited

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

rfay commented Sep 28, 2017

rickmanelius commented Sep 29, 2017

rfay commented Nov 16, 2017

rickmanelius left a comment

Choose a reason for hiding this comment

rfay commented Nov 16, 2017

nmccrory commented Sep 14, 2017 •

edited

beeradb commented Sep 14, 2017 •

edited

beeradb Sep 19, 2017 •

edited

nmccrory Sep 27, 2017 •

edited

rfay left a comment •

edited

nmccrory Sep 28, 2017 •

edited