Add Memcached service configuration file and documentation. #927

jeffsheltren · 2018-06-17T13:33:19Z

The Problem/Issue/Bug:

Currently ddev does not provide a Memcached container. Memcached is used as a caching layer by many applications.

How this PR Solves The Problem:

Adds an example docker-compose file and documentation to setup a Memcached container within a ddev environment.

Manual Testing Instructions:

Follow instructions in documentation included in PR to create a Memcached container.

Automated Testing Overview:

This should get picked up automatically by the already-existing servicetest test script.

Related Issue Link(s):

#269

rfay

Wow, thanks! This worked absolutely great out of the box for me.

I think the environment stanza should go away.
This needs a test. TestServices wants to test it, but didn't foresee a service without an http interface, so it doesn't actually test it.

If you want to give a shot at a test, that would be fantastic, otherwise we'll try to add one in next release cycle if you're OK with us pushing into your PR.

Without a test, the example file runs the risk of becoming obsolete with future changes to ddev, causing infinite pain to users.

Thanks for this!

rfay · 2018-06-26T13:38:33Z

pkg/servicetest/testdata/services/docker-compose.memcached.yaml

+      com.ddev.site-name: ${DDEV_SITENAME}
+      com.ddev.approot: $DDEV_APPROOT
+      com.ddev.app-url: $DDEV_URL
+    environment:


I'm pretty sure the entire environment stanza is not useful; we don't support access via http on named project via port 11211 because 11211 is a tcp interface, not http. So ddev-router is not involved at all. (I tested and it works fine without this section)

rfay · 2018-06-26T13:55:12Z

pkg/servicetest/testdata/services/docker-compose.memcached.yaml

+    labels:
+    # These labels ensure this service is discoverable by ddev
+      com.ddev.site-name: ${DDEV_SITENAME}
+      com.ddev.approot: $DDEV_APPROOT


The "labels" section is also not useful since site-name, app-root, and app-url aren't used here. The commend about "Discoverable by ddev" is incorrect, I think, I know it's from the example.

Oops, I was wrong about this. com.ddev-site-name is absolutely required, it's what is used by ddev rm and ddev stop to take down the container. The other two don't seem to be useful. (Noticed a spare memcached container hanging around on my host!)

rfay · 2018-06-26T14:00:57Z

pkg/servicetest/testdata/services/docker-compose.memcached.yaml

+
+    command: ["-m", "128"]
+
+# This links the memcached service to the web service defined in the main docker-compose.yml, allowing applications running in the web service to access the memcached service at memcached:11211


The 'web' stanza is also not useful. Pretty sure it should be removed.

rfay · 2018-07-13T00:12:12Z

Hoping to get this in before it's too stale, know you're busy. Would love to have a rebase of it and get it going again. I don't think it will take too much to get it in. Let us know if you want us to run with it instead (and you don't mind commits being pushed into your fork).

rfay · 2018-07-14T18:45:40Z

Moving to v1.1.0

jeffsheltren · 2018-07-18T15:37:20Z

Thanks for the feedback @rfay! I believe I addressed everything in the docker-compose file with the commit today. I'd love your help with adding a test for it. I believe we can simply test a TCP connection to port 11211, but I'm not sure how to do so. Feel free to push to my fork if that's easiest.

rfay · 2018-07-18T23:32:05Z

Could you rebase this? Or mind if I rebase and push back into your fork?

rfay · 2018-07-31T03:19:34Z

@jeffsheltren Could you please click the checkbox that allows collaborators to push into this, or grant me access to your fork? Was just trying to rebase and push to get the last test fail out of there (which had nothing to do with this of course)

…v#269

andrewfrench

I've updated the tests to not assume HTTP interaction and got the memcached service responding to TCP connections. This requires a bit more configuration in the test itself to define the expected protocol, private port, and interaction path (as the HTTP test was returning HTTP301 at /, not 200).

Aside from my changes, the additions here look good to me and I was able to get the service up and running with the new docker-compose file and docs.

rfay

Thanks, I'm impressed how fast you turned this around! I do think we should try to access the correct port from within the web container.

Also, it only looks for tcp/http connectivity, it would be lovely if it did something to see that the daemon listening is the right one and actually responds correctly to some URI (or tcp command for memcached).

rfay · 2018-07-31T19:30:27Z

pkg/servicetest/servicetest_test.go

+
+	for _, port := range container.Ports {
+		if port.PrivatePort == targetPort && port.PublicPort != 0 {
+			address := fmt.Sprintf("http://127.0.0.1:%d%s", port.PublicPort, path)


This is a decent test, but I think it may be leaving out the important fact that we have to be able to access the port from within the web container. I imagine this approach is what we were doing before, but we should consider using app.Exec() to take an action within the web container. nc (netcat) is installed in there, it would do fine I think.

rfay · 2018-07-31T19:31:03Z

pkg/servicetest/servicetest_test.go

+
+	for _, port := range container.Ports {
+		if port.PrivatePort == targetPort {
+			address := fmt.Sprintf("127.0.0.1:%d%s", port.PublicPort, path)


As with the other one, this tests from the host's perspective, but may not be an adequately valid test because it's not testing from the web container's perspective.

rfay

Thanks to both of you for this. Thanks for the more-realistic testing scenario @andrewfrench . This should be ready to go.

rickmanelius

Looks good to me. I was able to add the yaml file and see it pull down and stand up. I'm not as familiar with the actual testing of memcached, so if Randy and Andrew were able to successfully interact with it, I'm good with that!

andrewfrench self-requested a review June 22, 2018 20:49

rickmanelius requested review from rfay and rickmanelius June 25, 2018 20:25

rickmanelius mentioned this pull request Jun 25, 2018

v0.20.0 Release Checklist Due 2018-06-19 #907

Closed

8 tasks

rfay requested changes Jun 26, 2018

View reviewed changes

rfay reviewed Jun 26, 2018

View reviewed changes

rfay added this to the v1.1.0 milestone Jul 14, 2018

jeffsheltren added 2 commits July 30, 2018 21:26

Add Memcached service configuration file and documentation. Fixes dde…

b6c8b42

…v#269

Remove unneeded config from memcached example

890b23c

rfay force-pushed the memcache-service branch from 2db3f2e to 890b23c Compare July 31, 2018 03:27

andrewfrench added 2 commits July 31, 2018 12:27

Improve error message in EnsureHTTPStatus()

5e038a6

Support TCP test, require slightly more test configuration.

19b5073

andrewfrench approved these changes Jul 31, 2018

View reviewed changes

rfay requested changes Jul 31, 2018

View reviewed changes

Improve tests with interaction from web container.

3bb68b8

andrewfrench self-assigned this Aug 1, 2018

rfay approved these changes Aug 2, 2018

View reviewed changes

rickmanelius approved these changes Aug 2, 2018

View reviewed changes

andrewfrench merged commit 6da3770 into ddev:master Aug 2, 2018

andrewfrench mentioned this pull request Aug 6, 2018

Create a service configuration for memcache #269

Closed

rickmanelius mentioned this pull request Aug 15, 2018

v1.1.0 Release Checklist Due 2018-08-15 #994

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Memcached service configuration file and documentation. #927

Add Memcached service configuration file and documentation. #927

jeffsheltren commented Jun 17, 2018

rfay left a comment

rfay Jun 26, 2018

rfay Jun 26, 2018

rfay Jun 28, 2018

rfay Jun 26, 2018

rfay commented Jul 13, 2018

rfay commented Jul 14, 2018

jeffsheltren commented Jul 18, 2018

rfay commented Jul 18, 2018

rfay commented Jul 31, 2018

andrewfrench left a comment •

edited

rfay left a comment

rfay Jul 31, 2018

rfay Jul 31, 2018

rfay left a comment

rickmanelius left a comment


		command: ["-m", "128"]

		# This links the memcached service to the web service defined in the main docker-compose.yml, allowing applications running in the web service to access the memcached service at memcached:11211

Add Memcached service configuration file and documentation. #927

Add Memcached service configuration file and documentation. #927

Conversation

jeffsheltren commented Jun 17, 2018

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

rfay left a comment

Choose a reason for hiding this comment

rfay Jun 26, 2018

Choose a reason for hiding this comment

rfay Jun 26, 2018

Choose a reason for hiding this comment

rfay Jun 28, 2018

Choose a reason for hiding this comment

rfay Jun 26, 2018

Choose a reason for hiding this comment

rfay commented Jul 13, 2018

rfay commented Jul 14, 2018

jeffsheltren commented Jul 18, 2018

rfay commented Jul 18, 2018

rfay commented Jul 31, 2018

andrewfrench left a comment • edited

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

rfay Jul 31, 2018

Choose a reason for hiding this comment

rfay Jul 31, 2018

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

rickmanelius left a comment

Choose a reason for hiding this comment

andrewfrench left a comment •

edited