Skip to content

Loading…

Improve Dockerfile and Procfile workflows #967

Merged
merged 13 commits into from

5 participants

@gabrtv
Deis member

This PR refactors the builder logic to better support Dockerfile builds alongside Heroku-style builds. Improvements and refactoring include:

  • store SHA, Procfile and Dockerfile when builds flow through the builder component
  • auto-scale web=1 for heroku workflow, cmd=1 for docker workflow
  • detect exposed ports rather than using hardcoded 5000/tcp
  • add container type check before scale operations (fixes #496)
  • use SHA in release summary if available
  • fix operator precedence bug from deis/deis#890
  • add tests for heroku and docker workflows, plus tests for invalid process types

This PR also cleans up the entire set of developer docs, including a new section on app configuration which fixes #695.

@gabrtv gabrtv was assigned by mboersma
@carmstrong
Deis member

I like that we are now auto-scaling. It makes a lot of sense, and makes for a faster Deis satisfaction.

@bacongobbler bacongobbler commented on an outdated diff
...tainer_type_num__add_field_build_sha__add_field_bu.py
((160 lines not shown))
+ 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+ 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+ 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+ 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'related_name': "u'user_set'", 'blank': 'True', 'to': u"orm['auth.Permission']"}),
+ 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+ },
+ u'contenttypes.contenttype': {
+ 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+ 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+ }
+ }
+
+ complete_apps = ['api']
@bacongobbler Deis member

nitpick: can you please add newlines to the migrations?

@gabrtv Deis member
gabrtv added a note

but but they were autogenerated :) I will add a newline when I squash the migration to a single file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bacongobbler bacongobbler commented on an outdated diff
controller/api/models.py
((6 lines not shown))
tasks.deploy_release.delay(self, release).get()
- if self.structure == {}:
- # scale the web process by 1 initially
- self.structure = {'web': 1}
+ # TODO: figure out if the logic below is what we really want
+ if initial:
+ # if there is procfile with a web worker, scale by web=1
+ if release.build.procfile and 'web' in release.build.procfile:
@bacongobbler Deis member

I think this a reasonable solution for Dockerfile + Procfile apps. If you're supplying a Procfile, you're going to be following Heroku's best practices anyways. However, I'd remove the and statement and just leave it to check if a Procfile is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bacongobbler bacongobbler commented on the diff
controller/api/models.py
@@ -357,6 +370,11 @@ class Build(UuidAuditedModel):
app = models.ForeignKey('App')
image = models.CharField(max_length=256)
+ # optional fields populated by builder
+ sha = models.CharField(max_length=40, blank=True)
@bacongobbler Deis member

:+1: this helps me greatly with #736 as well.

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

@gabrtv can you please rebase now that your config PR has been merged?

@bacongobbler bacongobbler commented on the diff
controller/api/tests/test_container.py
@@ -221,6 +292,9 @@ def test_container_errors(self):
body = {'web': 'not_an_int'}
response = self.client.post(url, json.dumps(body), content_type='application/json')
self.assertContains(response, 'Invalid scaling format', status_code=400)
+ body = {'invalid': 1}
@bacongobbler Deis member

free tests woo

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

@gabrtv can you please provide a sample Dockerfile and a sample Dockerfile + Procfile app to test these changes with? https://github.com/opdemand/example-perl will help test Procfile-free and Dockerfile-free apps, and https://github.com/opdemand/example-ruby-sinatra can help with Profile apps. Sending a PR to https://github.com/opdemand/example-dockerfile-python would be awesome if we can get that one fixed up.

I have a suspicion that procfile-free and dockerfile-free apps such as example-perl may break with these changes, but we'll find out soon enough.

@bacongobbler bacongobbler commented on an outdated diff
builder/templates/builder
@@ -104,6 +114,18 @@ if __name__ == '__main__':
body['receive_user'] = user
body['receive_repo'] = app
body['image'] = target_image
+ # use sha of master
+ with open(os.path.join(repo_dir, 'refs/heads/master')) as f:
+ body['sha'] = f.read().strip('\n')
+ # extract the Procfile and convert to JSON
+ if os.path.exists(procfile):
@bacongobbler Deis member

Some buildpacks may have a default_process_types defined in bin/release instead of injecting a Procfile into the app root. Does slugbuilder generate a Procfile from default_process_types?

@bacongobbler Deis member

Nope. Sometimes a .release file is generated instead. We should check for that as well.

ref: https://github.com/deis/slugbuilder/blob/deis/builder/build.sh#L94

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

I have tested this PR with:

The logic seems to be what we want. I'm going to move on to documentation now...

@gabrtv
Deis member

OK, docs are in and this PR is ready for testing. To test, try deploying the 3 repositories described in #967 (comment).

@gabrtv gabrtv referenced this pull request
Closed

Allow container-less apps #815

@gregwebs

Thanks for the great doc improvements!

@bacongobbler
Deis member

Seeing this when deploying example-ruby-sinatra:

root@2dfd6f3b2652:~# bundle exec ruby web.rb -p $PORT
/app/vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/main.rb:15:in `<class:Application>': missing argument: -p (OptionParser::MissingArgument)
    from /app/vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/main.rb:4:in `<module:Sinatra>'
    from /app/vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/main.rb:3:in `<top (required)>'
    from /app/vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra.rb:2:in `require'
    from /app/vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra.rb:2:in `<top (required)>'
    from web.rb:1:in `require'
    from web.rb:1:in `<main>'

This is probably due to $PORT no longer being available inside buildpack deployments. What's the recommended workflow for those apps?

@carmstrong
Deis member

Is this ready for further testing, or is there a known bug as per #967 (comment)?

@gabrtv
Deis member

Let's hold off on testing until we get a handle on the $PORT issue. I'll tackle it tomorrow.

@mboersma mboersma added this to the 0.9.0 milestone
Gabriel Monroy added some commits
Gabriel Monroy feat(dockerfile): improve dockerfile and procfile workflow
- add SHA, Procfile and Dockerfile to Build model (if available)
- fix operator precedence bug from deis/deis#890
- add container type check before scale operations (fixes #496)
- add uniqueness constraint check on container type/num
- use SHA in release summary if available
- auto-scale web=1 for heroku workflow, cmd=1 for docker workflow
- add tests for heroku and docker workflows, plus test for invalid process types
c5aec20
Gabriel Monroy feat(builder): update builder to pass SHA/Procfile/Dockerfile (if ava…
…ilable)
730149c
Gabriel Monroy fix(announcer): detect first exposed port instead of hardcoding to 50…
…00/tcp
c8efffd
Gabriel Monroy fix(schema): remove uniqueness constraint on containers 2984581
Gabriel Monroy fix(controller): only scale web=1 if procfile has web entry 553978c
Gabriel Monroy fix(scale): move initial scaling logic to model, add tests cdbd9b9
Gabriel Monroy fix(migration): squash migrations, add newline b4ff75e
Gabriel Monroy fix(builder): include default_process_types from .release 877a87b
Gabriel Monroy fix(deploy): rework initial deploy logic based on workflow f680df6
Gabriel Monroy fix(flake8): resolve code formatting issues 5710ccb
Gabriel Monroy fix(tests): close db connections during threaded execution
This appears to resolve https://code.djangoproject.com/ticket/22420
allowing us to get test coverage for threads executed within
TransactionTestCases.
3d5d243
Gabriel Monroy docs(developer): flesh out developer docs 87159d5
Gabriel Monroy fix(scheduler): detect exposed ports from image for PORT envvar
2dee62c
@gabrtv
Deis member

We are now detecting the first exposed port from the Docker image and setting that as the $PORT envvar. This should provide the Heroku compatibility we need.

This PR is now rebased and ready for testing.

@carmstrong
Deis member

LGTM. This works great. Tested all the example apps listed above, scaled each to 2 containers, and I got 'Powered by Deis' on each.

@carmstrong
Deis member

Also seeing this, but I assume it's the known Docker container ARP bug:

fleetctl status zanier-mongoose_v2.cmd.2.service
● zanier-mongoose_v2.cmd.2.service - zanier-mongoose_v2.cmd.2
   Loaded: loaded (/run/systemd/system/zanier-mongoose_v2.cmd.2.service; static)
   Active: failed (Result: exit-code) since Fri 2014-05-16 19:06:46 UTC; 1min 29s ago
  Process: 3596 ExecStop=/usr/bin/docker rm -f zanier-mongoose_v2.cmd.2 (code=exited, status=0/SUCCESS)
  Process: 2639 ExecStartPost=/bin/sh -c arping -Idocker0 -c1 `docker inspect -f '{{ .NetworkSettings.IPAddress }}' zanier-mongoose_v2.cmd.2` (code=exited, status=1/FAILURE)
  Process: 1815 ExecStartPost=/bin/sh -c until docker inspect zanier-mongoose_v2.cmd.2 >/dev/null 2>&1; do sleep 1; done (code=exited, status=0/SUCCESS)
  Process: 1814 ExecStart=/bin/sh -c port=$(docker inspect -f '{{range $k, $v := .config.ExposedPorts }}{{$k}}{{end}}' 172.17.8.100:5000/zanier-mongoose:v2 | cut -d/ -f1) ; /usr/bin/docker run --name zanier-mongoose_v2.cmd.2 -P -e PORT=$port 172.17.8.100:5000/zanier-mongoose:v2  (code=exited, status=2)
  Process: 1762 ExecStartPre=/bin/sh -c docker inspect zanier-mongoose_v2.cmd.2 >/dev/null 2>&1 && docker rm -f zanier-mongoose_v2.cmd.2 || true (code=exited, status=0/SUCCESS)
  Process: 32249 ExecStartPre=/usr/bin/docker pull 172.17.8.100:5000/zanier-mongoose:v2 (code=exited, status=0/SUCCESS)
 Main PID: 1814 (code=exited, status=2)

May 16 19:06:44 deis-1 docker[32249]: Pulling repository 172.17.8.100:5000/zanier-mongoose
May 16 19:06:46 deis-1 sh[2639]: ARPING 10.1.0.14 from 10.1.42.1 docker0
May 16 19:06:46 deis-1 sh[2639]: Sent 1 probes (1 broadcast(s))
May 16 19:06:46 deis-1 sh[2639]: Received 0 response(s)
May 16 19:06:46 deis-1 systemd[1]: zanier-mongoose_v2.cmd.2.service: control process exited, code=exited status=1
May 16 19:06:46 deis-1 systemd[1]: zanier-mongoose_v2.cmd.2.service: main process exited, code=exited, status=2/INVALIDARGUMENT
May 16 19:06:46 deis-1 docker[3596]: zanier-mongoose_v2.cmd.2
May 16 19:06:46 deis-1 systemd[1]: Failed to start zanier-mongoose_v2.cmd.2.
May 16 19:06:46 deis-1 systemd[1]: Unit zanier-mongoose_v2.cmd.2.service entered failed state.
@mboersma
Deis member

Our arping command failed? I hadn't seen that before. I thought it was still prefixed with a dash so it wouldn't be considered fatal, and probably should be if we lost that--it's a last-gasp hack.

@gabrtv
Deis member

@mboersma The ARP issue is resolved in the latest Docker (and latest CoreOS), so we should be able to remove that soon.

@carmstrong not sure what went wrong with your deis/helloworld exec. Can you try scaling it down and back up? Are your other apps working?

@carmstrong
Deis member

@gabrtv I started again with a fresh clone of helloworld and it worked fine. All other apps worked as well. Only issue was the ARP issue.

@gabrtv
Deis member

ARP issue doesn't break functionality, it just pollutes logs. Plus it's going away soon. I refer back to your LGTM from an hour ago. :smile:

@carmstrong
Deis member

My LGTM still stands. :checkered_flag:

@mboersma
Deis member

LGTM, reviewed code and docs and tested sample apps:

Running deis apps:create testing... Success!
Running git push deis master... Success!
Running deis scale web=2... Success!
Running curl -s http://testing.local.deisapp.com | grep -q 'Powered by Deis'... Success!
@bacongobbler bacongobbler added a commit to deis/example-dockerfile-python that referenced this pull request
@bacongobbler bacongobbler update project to reflect deis/deis#967 dab649c
@bacongobbler
Deis member

Apps are still failing if you supply both a Procfile and a Dockerfile. This is due to the start command not being present in the container. Sample app that will show this behaviour is at https://github.com/opdemand/example-dockerfile-python/tree/967-improved-workflow

Asking users to supply a start command was recommended before, but then the procfile would be redundant, as you could just generate a script like the following in your Dockerfile:

RUN echo 'python -m SimpleHTTPServer $PORT' > /bin/start
RUN chmod 755 /bin/start

Unit file deployed to Deis:

$ fleetctl cat deluxe-drumroll_v2.web.1.service

[Unit]
Description=deluxe-drumroll_v2.web.1

[Service]
ExecStartPre=/usr/bin/docker pull 172.17.8.100:5000/deluxe-drumroll:v2
ExecStartPre=/bin/sh -c "docker inspect deluxe-drumroll_v2.web.1 >/dev/null 2>&1 && docker rm -f deluxe-drumroll_v2.web.1 || true"
ExecStart=/bin/sh -c "port=$(docker inspect -f '{{range $k, $v := .config.ExposedPorts }}{{$k}}{{end}}' 172.17.8.100:5000/deluxe-drumroll:v2 | cut -d/ -f1) ; /usr/bin/docker run --name deluxe-drumroll_v2.web.1 -P -e PORT=$port 172.17.8.100:5000/deluxe-drumroll:v2 start web"
ExecStartPost=/bin/sh -c "until docker inspect deluxe-drumroll_v2.web.1 >/dev/null 2>&1; do sleep 1; done";     /bin/sh -c "arping -Idocker0 -c1 `docker inspect -f '{{ .NetworkSettings.IPAddress }}' deluxe-drumroll_v2.web.1`"
ExecStop=/usr/bin/docker rm -f deluxe-drumroll_v2.web.1

Output of running the ExecStart command (with -d option added as well):

/bin/sh -c "port=$(docker inspect -f '{{range $k, $v := .config.ExposedPorts }}{{$k}}{{end}}' 172.17.8.100:5000/deluxe-drumroll:v2 | cut -d/ -f1) ; /usr/bin/docker run --name deluxe-drumroll_v2.web.1 -P -d -e PORT=$port 172.17.8.100:5000/deluxe-drumroll:v2 start web"
e1242d68176608e00d61659602fb60a52a9ac43f7f1ee13efbc3c63052514d79
core@deis-1 ~ $ docker logs e1242d68176608e00d61659602fb60a52a9ac43f7f1ee13efbc3c63052514d79
2014/05/16 20:40:36 exec: "start": executable file not found in $PATH
@bacongobbler
Deis member

Asides the above comment, everything else is A-OK :)

@gabrtv gabrtv merged commit 36544e7 into master

1 of 2 checks passed

default Merged build triggered.
Details continuous-integration/travis-ci The Travis CI build passed
@gabrtv gabrtv deleted the dockerfile-workflow branch
@bacongobbler
Deis member

:tada:

@bacongobbler bacongobbler added a commit that referenced this pull request
@bacongobbler bacongobbler refactor(controller): bump migration number
PR #956 was developed at the same time as PR #967. The latter was
merged first, so let's bump the number up by one.
b09799d
@bacongobbler bacongobbler added a commit that referenced this pull request
@bacongobbler bacongobbler refactor(controller): bump migration number
PR #956 was developed at the same time as PR #967. The latter was
merged first, so let's bump the number up by one.
61068c6
@bacongobbler bacongobbler added a commit that referenced this pull request
@bacongobbler bacongobbler refactor(controller): bump migration number
PR #956 was developed at the same time as PR #967. The latter was
merged first, so let's bump the number up by one.
0f31dfa
@ghost Unknown pushed a commit that referenced this pull request
@bacongobbler bacongobbler refactor(controller): bump migration number
PR #956 was developed at the same time as PR #967. The latter was
merged first, so let's bump the number up by one.
f162e92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 16, 2014
  1. feat(dockerfile): improve dockerfile and procfile workflow

    Gabriel Monroy committed
    - add SHA, Procfile and Dockerfile to Build model (if available)
    - fix operator precedence bug from deis/deis#890
    - add container type check before scale operations (fixes #496)
    - add uniqueness constraint check on container type/num
    - use SHA in release summary if available
    - auto-scale web=1 for heroku workflow, cmd=1 for docker workflow
    - add tests for heroku and docker workflows, plus test for invalid process types
  2. feat(builder): update builder to pass SHA/Procfile/Dockerfile (if ava…

    Gabriel Monroy committed
    …ilable)
  3. fix(announcer): detect first exposed port instead of hardcoding to 50…

    Gabriel Monroy committed
    …00/tcp
  4. fix(schema): remove uniqueness constraint on containers

    Gabriel Monroy committed
  5. fix(controller): only scale web=1 if procfile has web entry

    Gabriel Monroy committed
  6. fix(scale): move initial scaling logic to model, add tests

    Gabriel Monroy committed
  7. fix(migration): squash migrations, add newline

    Gabriel Monroy committed
  8. fix(builder): include default_process_types from .release

    Gabriel Monroy committed
  9. fix(deploy): rework initial deploy logic based on workflow

    Gabriel Monroy committed
  10. fix(flake8): resolve code formatting issues

    Gabriel Monroy committed
  11. fix(tests): close db connections during threaded execution

    Gabriel Monroy committed
    This appears to resolve https://code.djangoproject.com/ticket/22420
    allowing us to get test coverage for threads executed within
    TransactionTestCases.
  12. docs(developer): flesh out developer docs

    Gabriel Monroy committed
  13. fix(scheduler): detect exposed ports from image for PORT envvar

    Gabriel Monroy committed
Something went wrong with that request. Please try again.