feat(api): Allow using an unreleased image for the run command

[robholland]

This is useful for build steps which must be completed before an app is fully
deployed to deis, for example asset compilation.

[deis-admin]

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

[vdice]

Jenkins, add to whitelist.

[vdice]

@robholland looks like a simple test-style/flake8 error (line length complaints) from the travis build: https://travis-ci.org/deis/controller/builds/193398357

[bacongobbler]

Hey @robholland, first off I want to say thank you for the PR. I'm sure this is something you require for your application's use case.

My only concern with this PR is that it conflicts directly from the heroku run model in that all run commands are expected to be run inside the currently deployed application image. By supporting the idea of allowing a run command outside of the container image deployed to Workflow, we break this model. However, I understand that the alternative is an out-of-band schema migration before deploying the new image release. I know that Heroku handles this use case by running schema migrations inside the Ruby buildpack, however this is an unsolved problem with Dockerfile deploys.

What are your thoughts on all this? Is there perhaps another way we can resolve this problem while maintaining compatibility with Heroku's run model?

[robholland]

An approach we considered earlier was adding a hook to require workflow to run a set command inside a new release after building, but before starting any other pods. If the command failed then the release would be rolled back (as if the readiness probe had failed etc). This would also solve this specific use case of asset compilation and may be closer to the heroku model you are trying to follow?

The downside is that you lose the flexibility that the run argument provides, any command can be run at anytime, with either previous or as-yet-unreleased versions to help find regressions/bugs in production easily. Also potentially useful for migrations, as you mention. If we create a new app, copy configuration over etc then any inferred namespaces in config settings would need adjusting to make services discoverable and so forth, it's quite a lot of overhead to achieve this with a duplicated app.

If the concern is keeping the run command pure, maybe another dedicated command would sit better? No good names are coming to mind right now.

[bacongobbler]

I think implementing the deis hooks command you proposed in another issue (can't seem to find it now) would be the better way to resolve this use case. That way we can retain staying true to Heroku's run model while still allowing one to run post-build hooks like a schema migration before the app is deployed.

[robholland]

For reference this is the issue for the hooks: deis/dockerbuilder#89

[bacongobbler]

ping @robholland, are you planning on implementing deis/dockerbuilder#89 or do you still want to go ahead with this?

[robholland]

I'm working on patches for deis/dockerbuilder#89, but there are quite a few use cases that doesn't cover, it's only suitable for commands that need to be run pre-deploy everytime. We could still really use the ability to run non-deployed revisions to avoid the need to duplicate applications in order to drain sidekiq queues and other adhoc tasks.

[codecov-io]

Codecov Report

Merging #1208 into master will increase coverage by <.01%.

@@            Coverage Diff            @@
##           master   #1208      +/-   ##
=========================================
+ Coverage   87.29%   87.3%   +<.01%     
=========================================
  Files          43      43              
  Lines        3826    3829       +3     
  Branches      665     667       +2     
=========================================
+ Hits         3340    3343       +3     
  Misses        321     321              
  Partials      165     165

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62673f2...09b8a3b. Read the comment docs.

[robholland]

Please can I have some feedback on whether this is likely to be included in the next workflow release? We have a lot of applications which can't have their web frontend migrated to workflow until we have the ability to compile their assets during deploys.

[mboersma]

I have the same concerns as @bacongobbler mentioned: this doesn't fit the heroku run model that we've tried to emulate. Pre- and post-compile hooks as described in deis/dockerbuilder#89 would be a cleaner solution, but I understand that's not trivial to implement. If there were any other way to achieve this behavior, it would be preferable and allow deis run to stay simple.

Having said that, I don't think allowing this would open any particular security issues, just create a bit of conceptual discord.

[robholland]

We've implemented our requirement using an ugly kubectl run -n $APP --env="RACK_ENV=$(deis config | grep RACK_ENV ...),MEMCACHE_SERVERS=$(deis config | grep MEMCACHE_SERVERS ...)" ... style command.

Feel free to close this and related PRs if you're not happy with the run override concept.

[robholland]

I should say I don't plan to work on the deploy hook PRs currently as they don't solve enough of our use cases to be worth the investment required for me to implement (I don't know django/python well and there are a lot of moving parts here).

[mboersma]

@robholland I'm sorry you had to resort to the kubectl hack to accomplish this. Ultimately we considered this to break the Heroku-inspired paradigm a bit too much to be merged into master.

I apologize for dawdling on this one--it was difficult to decide. We appreciate all your contributions immensely. Let's try to implement deploy hooks for a future release, since that seems like a more consistent solution. I'll see if we can find someone to work on that soon.