fix(deploy): add global settings DEIS_DEPLOY_PROCFILE_MISSING_REMOVE and DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING by helgi · Pull Request #1099 · deis/controller

helgi · 2016-10-04T00:06:06Z

Add 2 new settings, only available globally

DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING rejects a deploy if the previous build had a Procfile but the current deploy is missing it for some reason. A 409 is thrown

DEIS_DEPLOY_PROCFILE_MISSING_REMOVE when set to false will allow an empty Procfile to go through without remove missing process types if the previous build had a Procfile

ref #1062

deis-bot · 2016-10-04T00:06:10Z

@kmala, @mboersma and @bacongobbler are potential reviewers of this pull request based on my analysis of git blame information. Thanks @helgi!

codecov-io · 2016-10-04T00:29:51Z

Current coverage is 87.91% (diff: 100%)

Merging #1099 into master will increase coverage by 0.03%

@@             master      #1099   diff @@
==========================================
  Files            42         42          
  Lines          3639       3650    +11   
  Methods           0          0          
  Messages          0          0          
  Branches        623        625     +2   
==========================================
+ Hits           3198       3209    +11   
  Misses          290        290          
  Partials        151        151

Powered by Codecov. Last update f4ef652...89b5003

kmala · 2016-10-04T00:35:52Z

+        config = previous_release.config
+        # See if processes are permitted to be removed
+        remove_procs = bool(config.values.get('PROCFILE_REMOVE_PROCS_ON_DEPLOY', settings.PROCFILE_REMOVE_PROCS_ON_DEPLOY))  # noqa
+        print(remove_procs)


i think this was just for testing.

yes, indeed. removing

heynemann · 2016-10-04T01:16:02Z

+        url = "/v2/apps/{app_id}/builds".format(**locals())
+        body = {'image': 'autotest/example'}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)


shouldn't this be an error? I don't quite understand how it will not change my targets, yet still succeed. Am I missing something?

This is the example of default Deis, which removes processes if you do not provide any Procfile information.

Look at the next test case, that's the one where the "remove processes even if Procfile is missing or has changes" - This implies that you (or your users) have to specifically scale down processes yourself if you do not want it anymore.

It could be extended to only work that way if a Procfile is missing vs if you provide a Procfile that may remove some of the proc types but not all.

heynemann · 2016-10-04T02:35:14Z

+        url = "/v2/apps/{app_id}/builds".format(**locals())
+        body = {'image': 'autotest/example'}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)


I'm sorry, I meant to comment this test case. Shouldn't this return a 406 - Not Acceptable response? What actually happened in my app if it didn't scale? Did it update any units?

No, a 201 is returned because the setting is not rejecting the changes, it is saying that it won't remove any process type despite the lack of procfile or the procfile removing any process types - it is up to the end user to spin processes down by hand

To clarify, no process types get removed but all other changes are applied. New image, new config, etc - instead of rejecting the change then instead it allows it through without spinning process types down from underneath you

Are you rather looking for a reject deploy model?

I think this is a step forward, but I was expecting that if the flag is up, the API would reject the push as it would be an "incomplete" push if it goes through, right?
From what I grasped, the images will be there, configuration as well, but there won't be any processes running the new version. Is that correct?

They will all be there, including the processes - since we are not rejecting the deploy and we are specifically not spinning down any process types of the flag is set.

I could make it reject instead of letting changes through - could also make that a different flag to make it very configurable

@felipejfc Awesome

@heynemann Was looking at bit more into 406 and it both feels like the right status code and also the wrong one based on https://httpstatuses.com/406

Think I will use 406 none the less but there are certain "expectations" in there which we will not be fulfilling but that's probably okay? Does anyone have thoughts on a better code? 400?

I think 409 Conflict is the most appropriate one, due to it's description:

The request could not be completed due to a conflict with the current state of the resource. This code is only allowed in situations where it is expected that the user might be able to resolve the conflict and resubmit the request. The response body SHOULD include enough information for the user to recognize the source of the conflict.

What this means is that in the response the user should be able to identify that no procfile was specified and be able to decide what to do next. Sounds like what we need.

Ah yes! We use 409 in other places, going to use that.

Have a look now guys - 2 different settings, one rejects (and takes priority) when previous deploy had a Procfile but the current one does and one setting that lets a deploy through only when the setting is on if you are missing a Procfile

Looks great!!! :)

bacongobbler · 2016-10-05T15:09:47Z

+        """
+        Specifically test PROCFILE_REMOVE_PROCS_ON_DEPLOY being turned off
+        and a user posting a new build without a Procfile that was previously
+        applid


typo: applied

heynemann · 2016-10-06T13:47:25Z

+        url = "/v2/apps/{app_id}/builds".format(**locals())
+        body = {'image': 'autotest/example'}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)


Looks great!!! :)

felipejfc

Looks good! Thanks for covering both cases @helgi

helgi · 2016-10-06T15:33:06Z

All that is left then is to figure out if this should also be per app setting - I removed that notion in the last code push since Deis Operators may not wish to let their devs change that but then again you may want to have a different global setting than per app... Thoughts on making it per app as well before reviews happen?

heynemann · 2016-10-06T15:56:25Z

We wouldn't use a per app setting, so from our perspective it does not make sense.

helgi · 2016-10-06T16:03:57Z

@bacongobbler, @mboersma or @kmala thoughts?

kmala · 2016-10-06T16:24:28Z

+        # See if processes are permitted to be removed
+        remove_procs = (
+            # If set to True then contents of Procfile does not affect the outcome
+            settings.DEIS_DEPLOY_PROCFILE_MISSING_REMOVE is True or


The point to note here is that if this is false and DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING is false then user can deploy with procfile missing and would have other proctype still running but he can't scale them or may not do another deploy properly as the command to run won't be present as there is no procfile.
I feel we should have only one setting DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING to accept or reject a deploy.

@kmala those 2 settings do 2 different things. One is all about rejecting if you are missing a Procfile and the other is about letting a deploy through without scaling down any procs types if you miss the Procfile

I'm not sure I am following your reasoning around both of those settings being set to false - Is that something you tried doing?

I could do the reject only option or we could copy the procfile info between builds if the fear is the Build object now has empty Procfile field (a valid concern to have).

Oh, and are you also talking about App::structures being wrong due to this?

yes.....app structure will different than build.procfile and it causes issues with deploys and scale

@kmala in that case app.structure will be correct (see the tests, I verify structure has not changed, unless you mean something else) whereas the latest build.procfile will be empty.

2 options then, ditch "remove if missing" setting or copy procfile info from the previous build when that setting is enabled and the Procfile info is missing. That'd let things work for scale / deploy.

yes app.structure won't change but build.procfile will be empty which means we can't scale as the proctype validates against it and can't get the command to run for that proctype.
I want to remove this setting and just one setting for the build to pass or not when a procfile is missing.

Okay fair - going to wait for others to weigh in.

I want to keep both setting since I can fix your concern with one line of code.

yes that can be fixed but it will definitely make things corrupt and i am not sure why do we want the build to succeed in that scenario.

@kmala because it's something the user wants, to allow services to push a new build (new image via Jenkins for example) without having to reference the Procfile - I see your point tho. See below for improvements.

In any case, I fixed the code and expanded the tests (see api.tests.test_build.BuildTest.test_build_no_remove_process) to prove that running a scale at the end. Prior to my fix that test failed as build.procfile was empty. Now it is not and the tests pass.

What are you worried will get corrupt at this point?

@kmala just to be clear I am fine with removing the second flag, I just want to make sure that's actually what we want to do since it feels like your worries are more tech related than the feature itself being useful (or not useful) - That's why I fixed your last concern to see how people feel about it now :)

bacongobbler · 2016-10-06T16:36:23Z

+# If the user has a Procfile in both deploys then processes are scaled up / down as per usual
+#
+# By default the process types are scaled down unless this setting is turned on
+DEIS_DEPLOY_PROCFILE_MISSING_REMOVE = strtobool(os.environ.get('DEIS_DEPLOY_PROCFILE_MISSING_REMOVE', 'true'))  # noqa


strtobool appears to return a number instead of a boolean. Is this what we'd like?

><> python3 >>> bool('true') True >>> bool('false') True >>> from distutils.util import strtobool >>> strtobool('false') 0 >>> strtobool('true') 1

Perhaps bool(strtobool(...)) would be preferred here?

Good point - I had not looked at the return value - I only made sure it worked in an if statement as per normal. 0 and 1 work fine in those cases but I'm going to make it explicit as per your suggestion. Thanks!

bacongobbler · 2016-10-06T16:37:12Z

+# If a previous deploy had a Procfile but then the following deploy has no Procfile then it will
+# result in a 406 - Not Acceptable
+# Has priority over DEIS_DEPLOY_PROCFILE_MISSING_REMOVE
+DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING = strtobool(os.environ.get('DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING', 'false'))  # noqa


same here IRT the strtobool comment

helgi · 2016-10-07T23:56:23Z

Can I get another round of reviews on this? Otherwise, I'll bump it to the 2.8 milestone, which is not a big deal.

bacongobbler · 2016-10-11T19:48:57Z

sorry @helgi that I wasn't able to get back to this in time! I'll assign myself as a reviewer so we can definitely get this in for v2.8 if we feel it's good to .

bacongobbler · 2016-10-12T19:10:09Z

@kmala would you mind taking a look at this?

kmala · 2016-10-12T19:18:53Z

I am fine with this changes but i think we should document on the use of these setting.

bacongobbler · 2016-10-12T19:30:22Z

indeed. @helgi would you please refrain from merging this until some documentation is in deis/workflow, mmkay? :)

helgi · 2016-10-12T19:52:18Z

@bacongobbler @kmala Yeah, for sure - Was waiting for reviews so I wouldn't have to change that around since this was highly discussed.

@bacongobbler you added the "needs tests" - Which are you referring to? e2e specifically or more tests in the Controller (already wrote some)

…OCS_ON_DEPLOY to allow turning off / on removal of processes on deploys By default this setting is On to keep backwards compatibility but will allow Deis operators and developers to turns this off. To set the global version of PROCFILE_REMOVE_PROCS_ON_DEPLOY give it an empty value to be considered off ref deis#1062

… in controller/#1099 deis/controller#1099

helgi · 2016-10-13T15:45:26Z

Doc added deis/workflow#551

… in controller/#1099 deis/controller#1099

… in controller/#1099 (#551) deis/controller#1099

helgi added bug awaiting review labels Oct 4, 2016

helgi added this to the v2.7 milestone Oct 4, 2016

helgi self-assigned this Oct 4, 2016

helgi mentioned this pull request Oct 4, 2016

Deploying from outside the app's repo with deis pull -a xxx destroys custom process types #1062

Closed

kmala reviewed Oct 4, 2016

View reviewed changes

helgi force-pushed the ticket_1062 branch from 2154e98 to ceede0d Compare October 4, 2016 00:37

heynemann reviewed Oct 4, 2016

View reviewed changes

helgi force-pushed the ticket_1062 branch from ceede0d to aedb792 Compare October 4, 2016 18:35

bacongobbler reviewed Oct 5, 2016

View reviewed changes

helgi force-pushed the ticket_1062 branch 7 times, most recently from 8cbb2a4 to d765a46 Compare October 5, 2016 23:15

heynemann approved these changes Oct 6, 2016

View reviewed changes

felipejfc approved these changes Oct 6, 2016

View reviewed changes

kmala reviewed Oct 6, 2016

View reviewed changes

bacongobbler reviewed Oct 6, 2016

View reviewed changes

helgi force-pushed the ticket_1062 branch 2 times, most recently from 46eeb22 to d16d434 Compare October 6, 2016 17:58

helgi changed the title ~~fix(deploy): add a global / per-app setting called PROCFILE_REMOVE_PROCS_ON_DEPLOY to allow turning off / on removal of processes on deploys~~ fix(deploy): add global settings DEIS_DEPLOY_PROCFILE_MISSING_REMOVE and DEIS_DEPLOY_REJECT_IF_PROCFILE_MISSING Oct 6, 2016

mboersma modified the milestones: v2.8, v2.7 Oct 10, 2016

bacongobbler self-assigned this Oct 11, 2016

bacongobbler added the LGTM1 label Oct 11, 2016

kmala added the LGTM2 label Oct 12, 2016

bacongobbler added needs tests and removed awaiting review labels Oct 12, 2016

helgi force-pushed the ticket_1062 branch from d16d434 to 89b5003 Compare October 13, 2016 15:32

helgi added a commit to helgi/workflow that referenced this pull request Oct 13, 2016

feat(deploys): Add new configuration options to Controller introduced…

0e4b5db

… in controller/#1099 deis/controller#1099

helgi mentioned this pull request Oct 13, 2016

feat(deploys): Add new configuration options to Controller introducedin controller/#1099 deis/workflow#551

Merged

helgi added a commit to helgi/workflow that referenced this pull request Oct 13, 2016

feat(deploys): Add new configuration options to Controller introduced…

27d0603

… in controller/#1099 deis/controller#1099

helgi added a commit to deis/workflow that referenced this pull request Oct 13, 2016

feat(deploys): Add new configuration options to Controller introduced…

4196ca3

… in controller/#1099 (#551) deis/controller#1099

helgi merged commit 0d83024 into deis:master Oct 13, 2016

helgi deleted the ticket_1062 branch October 13, 2016 17:00

bacongobbler mentioned this pull request Oct 27, 2016

Existing app processes killed when using "deis pull" in directory without Procfile #1123

Closed

Conversation

helgi commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deis-bot commented Oct 4, 2016

Uh oh!

codecov-io commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 87.91% (diff: 100%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heynemann Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipejfc left a comment

Choose a reason for hiding this comment

Uh oh!

helgi commented Oct 6, 2016

Uh oh!

heynemann commented Oct 6, 2016

Uh oh!

helgi commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helgi Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

helgi commented Oct 4, 2016 •

edited

Loading

codecov-io commented Oct 4, 2016 •

edited

Loading

heynemann Oct 4, 2016 •

edited

Loading

helgi Oct 6, 2016 •

edited

Loading

bacongobbler commented Oct 11, 2016 •

edited

Loading