Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 #483

rfay · 2017-09-27T17:30:25Z

The Problem/Issue/Bug:

#126 explains: we need to test ports 80 and 443 before we let docker-compose try to bring up the router. If we find that somebody is already listening on them, we need to explain the problem to the user gracefully and provide resources to resolve the problem.

This also fixes #393 (Stopped site, double notifications of broken router)

How this PR Solves The Problem:

Stop the router before starting it again.
Test our ports at that time
If there's a problem stop then and explain to the user, giving resources.

Manual Testing Instructions:

Stop all sites (or manually use docker to stop the router)
Occupy port 80 or 443 using a technique like docker run -p 80:80 -p 443:443 --entrypoint=/bin/cat -it --rm busybox You can also start netcat or apache or nginx or many other ways.
Run ddev start with or without, you should see correct behavior.

To test the fix for #393

Start one site
Stop the router using docker docker stop ddev-router
Use ddev list and ddev describe to look for the issue

This now has:

Documentation (troubleshooting) links to help people to resolve their issue.
An automated test to verify behavior.
A link to the troubleshooting docs when this problem happens.

Automated Testing Overview:

Added TestRouterPortsCheck() which occupies port 80 using a docker container and then tests to see if we can start the router.

Release/Deployment notes:

rfay · 2017-09-28T14:08:06Z

This should be ready for review. @rickmanelius it probably deserves manual testing from you to see if it accomplishes what we need at this point.

rickmanelius · 2017-09-28T14:34:58Z

Happy to assist with a manual review.

rickmanelius

Preparation

git fetch --all
git checkout rfay/20170926_test_port_80
make clean && make darwin
Confirmed ddev version outputs v0.9.1-17-g78f2c569
Stop all ddev sites.

Review

Validated that ddev stop and ddev start work correctly on a singular site.
Validated that ddev list shows the router in a stopped state.
Started macOS's Apache via sudo apachectl start and validated it is in control when visiting http://localhost
Attempted a ddev start, which started the containers and at the Network step, I received the following error message: "Failed to start rickmanelius: Unable to listen on required ports, Localhost port 80 is in use,
Troubleshooting suggestions at https://github.com/drud/ddev/blob/master/docs/users/troubleshooting.md"
Linking to "https://github.com/drud/ddev/blob/master/docs/users/troubleshooting.md" doesn't work until the PR is fixed. However, I was able to confirm the PR contains it here https://github.com/rfay/ddev/blob/78f2c569840da88170920c48a0d195bbd8b0c3cb/docs/users/troubleshooting.md.
I confirmed the most obvious example sudo apachectl stop was provided. I ran the instructions and I was able to confirm that ddev start worked.
I verified that there were other great suggestions for other scenarios.
CONFIRMED

Suggestions

At least on macOS, running lsof doesn't require sudo.
I'm guilty of using OS X and macOS interchangeably, but macOS is the accurate term.

rickmanelius · 2017-09-28T16:34:33Z

@rfay I still think it would be of value for @tannerjfco to complete a code review, but manual end-user testing was a +1 from me.

beeradb · 2017-09-28T16:39:23Z

docs/users/troubleshooting.md

+or `sudo launchctl stop homebrew.mxcl.nginx`
+* nginx (Ubuntu): `sudo service nginx stop`
+* apache (often named "httpd") (many environments): `sudo apachectl stop` or on Ubuntu `sudo service apache2 stop`
+* vpnkit (OSX): You likely have a docker container bound to port 80, do you have containers up for Kalabox or another docker-based development environment? If so, stop the other environment.


I would put this "some suggestions" section at the top before the lsof stuff. Perhaps renaming it to "Common tools that use port 80" or similar. I feel like most people just want a fast solution, and if they see something like mamp/apache that they've previously used, would just jump right to that.

Also, a suggested addition:

Kalabox: If you have previously used Kalabox try running kbox poweroff

rfay · 2017-09-28T16:44:52Z

I went ahead and added a commit to fix all usages of OSX. Not actually legitimate to do here, but it seems pretty harmless. Didn't make sense to fix this PR without fixing other usages. I didn't change the sudo on lsof because it is required on Ubuntu and just seemed like it would muck things up to make the difference.

tannerjfco

Great work on this, thanks @rfay! A couple comments and questions here.

tannerjfco · 2017-09-29T21:41:07Z

pkg/plugins/platform/router.go

@@ -76,8 +79,20 @@ func StartDdevRouter() error {
 	_, err = f.WriteString(doc.String())
 	util.CheckErr(err)

+	// Stop router so we can test port. Of course, it may not be running.
+	err = dockerutil.ComposeCmd([]string{dest}, "-p", RouterProjectName, "down")


Couldn't we check for our healthy router instead? If we already have our router running, we already have the ports we require, so why stop it and start over? It seems like stopping it could also allow for breaking already-running sites, should the new site encounter an error that results in the router not returning successfully

Related idea: We could probably generally check running docker containers for ports occupied and report back if a specific container has our port. It's a nice idea from viewpoint of giving user most specific path to resolution, but not sure if it's worth it overall.

Pretty sure we could create a FindContainerByName() and look for "ddev-router" and that would be a faster & more stable check.

Between GetDockerContainers():
https://github.com/drud/ddev/blob/a0ee2e90c0aa388659d721b4b3f316727fba5506/pkg/dockerutil/dockerutils.go#L55-L60

and ContainerName()

https://github.com/drud/ddev/blob/a0ee2e90c0aa388659d721b4b3f316727fba5506/pkg/dockerutil/dockerutils.go#L161-L164

we already have the primitives. just need to loop the containers and check the name and ensure they are running.

If I understand all the comments here, the gist is "Let's not stop the router to test. If it's running, call it good and assume we have the ports." I'm good with that and can use the suggested techniques to do it. Slack link to full conversation: #483 (comment)

tannerjfco · 2017-09-29T21:43:59Z

docs/users/troubleshooting.md

+## Webserver ports are already occupied by another webserver
+
+If you get a message from ddev about a port conflict on port 80 or 443, like this:
+```


silly nit, but for best results w/ MkDocs' markdown parsing (what we're using on RTD), I suggest empty lines before and after formatting such as this. GitHub's markdown parsing is fantastic and doesn't seem to break much on this kind of thing anymore, but I've had to go back and fix little frustrations like this later in RTD.

Fixed in 278c462, thanks.

tannerjfco · 2017-09-29T21:46:29Z

docs/users/troubleshooting.md

+```
+sudo apachectl stop
+```
+


(suggestion) Just a little more text here would help it flow: e.g. "There are many applications that could be using port 80. Here are some more common tools and how to stop them".

Also fixed in 278c462

tannerjfco · 2017-09-29T21:52:46Z

cmd/ddev/cmd/start.go

@@ -33,7 +33,7 @@ provide a working environment for development.`,
 func appStart() {
 	app, err := platform.GetActiveApp("")
 	if err != nil {
-		util.Failed("Failed to start: %s", err)
+		util.Failed("Failed to GetActiveApp err: %v", err)


this error carries a bit more meaning for us, but less meaning for users. we also lose the context of what command produced the failure. I'd prefer to keep the original error message. If we need better identification of GetActiveApp errors, maybe we could address that in the error messages it returns?

Fixed in 40cfd5b, good point.

tannerjfco · 2017-09-29T22:02:48Z

pkg/plugins/platform/router.go

@@ -96,6 +111,7 @@ func StartDdevRouter() error {

 // PrintRouterStatus outputs router status and warning if not
 // running or healthy, as applicable.
+// An easy way to make these ports unavailable: sudo netcat -l -p 80  (brew install netcat)


this comment seems out of place.

It's actually intended here, but I added to it "In order to test this". The comment was to remind anybody how you'd test the port being occupied without a major effort of setting up apache or something.

beeradb · 2017-10-03T17:26:04Z

pkg/plugins/platform/router.go

+	}
+	err = CheckRouterPorts()
+	if err != nil {
+		return fmt.Errorf("Unable to listen on required ports, %v,\nTroubleshooting suggestions at https://github.com/drud/ddev/blob/master/docs/users/troubleshooting.md", err)


Please link to an anchor just for troubleshooting ports in this. Right now, the entire troubleshooting doc is great, but as it grows we don't we want to avoid sending people to a sea of information, particularly if we have already identified their problem.

The actual usage is like this in StartDdevRouter():

err = CheckRouterPorts() if err != nil { return fmt.Errorf("Unable to listen on required ports, %v,\nTroubleshooting suggestions at https://github.com/drud/ddev/blob/master/docs/users/troubleshooting.md", err) }

I assume that's what you'd want.

I think I may not have been clear enough. Sorry!

As you know, this PR introduces a new troubleshooting.md document. We're linking to it right here.

This works well for now, as the document is small, and finding information in it is very easy. However, this troubleshooting.md document is likely to grow over time. As such, it would be great to create anchors in it and link directly to those. That way if we add 3 more pages of information to the troubleshooting document over time, the user is still getting sent to an anchor that covers their specific issue. In this case, we'd want to link directly to the "Webserver ports are already occupied by another webserver" section of that file.

Added anchor in 4ccd462

beeradb

This works as intended but there are some small improvements to the code and messaging that could be made.

rfay · 2017-10-17T20:57:14Z

Rebased to current master.

…d not pull info

…tion

rfay · 2017-10-31T20:32:49Z

I think everybody's requests are addressed, most importantly, we don't stop the router just to see if we can start it again.

tannerjfco · 2017-11-01T17:35:17Z

pkg/plugins/platform/router.go

+	if err != nil || container.State != "running" {
+		err = CheckRouterPorts()
+		if err != nil {
+			return fmt.Errorf("Unable to listen on required ports, %v,\nTroubleshooting suggestions at https://github.com/drud/ddev/blob/master/docs/users/troubleshooting.md#unable-listen", err)


Troubleshooting link should be to our readthedocs site vs github.

tannerjfco

👍

rickmanelius · 2017-11-02T03:51:16Z

Given that I believe a bulk of the requested changes were on @beeradb, I think once we get a 👍 from him we are good to go.

beeradb

This works as advertised and the code looks good. Thanks!

rfay added the work in progress label Sep 27, 2017

rfay self-assigned this Sep 27, 2017

rfay removed the work in progress label Sep 27, 2017

rfay requested a review from tannerjfco September 27, 2017 20:17

rfay force-pushed the 20170926_test_port_80 branch 3 times, most recently from 74fa0cd to 78f2c56 Compare September 28, 2017 13:48

rfay requested a review from rickmanelius September 28, 2017 14:07

rickmanelius approved these changes Sep 28, 2017

View reviewed changes

beeradb reviewed Sep 28, 2017

View reviewed changes

tannerjfco suggested changes Sep 29, 2017

View reviewed changes

beeradb reviewed Oct 3, 2017

View reviewed changes

beeradb suggested changes Oct 3, 2017

View reviewed changes

rfay force-pushed the 20170926_test_port_80 branch from 519cd1d to 4af614a Compare October 17, 2017 20:56

rfay added 8 commits October 31, 2017 13:03

Basic port-test capabilities before trying to bring up router

38076ba

Add test to check if port is occupied

50ceefc

Change to use shell for invocation so we can get just container id an…

294d8f5

…d not pull info

Add troubleshooting docs

8aa68df

Add link to docs in the error message

09ef4b4

Add ClearDockerEnv in the right place to avoid docker-compose interac…

f7c6613

…tion

Change usages of OSX to macOS

bc1870e

Reorganize troubleshooting per Brad's suggestion

1408150

rfay force-pushed the 20170926_test_port_80 branch from 4af614a to 1408150 Compare October 31, 2017 19:04

rfay added 2 commits October 31, 2017 13:07

Improve GetActiveApp error message, back to original

40cfd5b

Improve troubleshooting doc - needed linefeed

278c462

rfay added 4 commits October 31, 2017 13:19

Reformat comment about why occupy port 80

ce129a7

Add anchor into troubleshooting.md doc

4ccd462

Remove double-notification about bad router, fixes ddev#393

2c40ebd

Don't stop router if it's running to see if it can run

8109d4d

rfay mentioned this pull request Oct 31, 2017

Stopped site can result in double notifications about router #393

Closed

rfay changed the title ~~Basic port-test capabilities before trying to bring up router, fixes #126~~ Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 Oct 31, 2017

Replace accidental removal of table in local Describe()

94673da

tannerjfco reviewed Nov 1, 2017

View reviewed changes

tannerjfco approved these changes Nov 1, 2017

View reviewed changes

beeradb approved these changes Nov 2, 2017

View reviewed changes

Change error prompt url to point to readthedocs instead of github

37f4e47

rfay merged commit d500dab into ddev:master Nov 2, 2017

rfay deleted the 20170926_test_port_80 branch November 2, 2017 19:36

rickmanelius mentioned this pull request Nov 20, 2017

v0.10.0 Release Checklist #542

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 #483

Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 #483

rfay commented Sep 27, 2017 •

edited

rfay commented Sep 28, 2017

rickmanelius commented Sep 28, 2017

rickmanelius left a comment

rickmanelius commented Sep 28, 2017

beeradb Sep 28, 2017 •

edited

rfay commented Sep 28, 2017

tannerjfco left a comment

tannerjfco Sep 29, 2017

tannerjfco Sep 29, 2017

beeradb Oct 3, 2017 •

edited

rfay Oct 31, 2017

tannerjfco Sep 29, 2017

rfay Oct 31, 2017 •

edited

tannerjfco Sep 29, 2017

rfay Oct 31, 2017

tannerjfco Sep 29, 2017

rfay Oct 31, 2017 •

edited

tannerjfco Sep 29, 2017

rfay Oct 31, 2017

beeradb Oct 3, 2017

rfay Oct 31, 2017

beeradb Oct 31, 2017

rfay Oct 31, 2017

beeradb left a comment

rfay commented Oct 17, 2017

rfay commented Oct 31, 2017

tannerjfco Nov 1, 2017

tannerjfco left a comment

rickmanelius commented Nov 2, 2017

beeradb left a comment

Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 #483

Basic port-test capabilities before trying to bring up router, fixes #126, fixes #393 #483

Conversation

rfay commented Sep 27, 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:

rfay commented Sep 28, 2017

rickmanelius commented Sep 28, 2017

rickmanelius left a comment

Choose a reason for hiding this comment

Preparation

Review

Suggestions

rickmanelius commented Sep 28, 2017

beeradb Sep 28, 2017 • edited

Choose a reason for hiding this comment

rfay commented Sep 28, 2017

tannerjfco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeradb Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfay Oct 31, 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 Oct 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeradb left a comment

Choose a reason for hiding this comment

rfay commented Oct 17, 2017

rfay commented Oct 31, 2017

Choose a reason for hiding this comment

tannerjfco left a comment

Choose a reason for hiding this comment

rickmanelius commented Nov 2, 2017

beeradb left a comment

Choose a reason for hiding this comment

rfay commented Sep 27, 2017 •

edited

beeradb Sep 28, 2017 •

edited

beeradb Oct 3, 2017 •

edited

rfay Oct 31, 2017 •

edited

rfay Oct 31, 2017 •

edited