Give better feedback on synchronous zipdeploy #2641

nikolaia · 2017-12-04T11:11:24Z

As mentioned in #2630 it would be nice to know if the deployment succeeded or failed when running a synchronous zipdeploy

davidebbo · 2017-12-04T13:58:26Z

@nickwalkmsft can you review this change to zip push deploy?

nickwalkmsft · 2017-12-04T17:55:29Z

Kudu.Services/Deployment/PushDeploymentController.cs

+                        response.Content = new StringContent("Succeeded");
+                        break;
+                    case FetchDeploymentRequestResult.RanSynchronouslyFailed:
+                        response.StatusCode = HttpStatusCode.OK;


Seems like this should be BadRequest or something similar?

Along with error text?

I changed FetchDeploymentRequestResult to include a StatusText and renamed the enum. Don't know if you approve of the pattern, let me know. Changed to BadRequest, agree it's better.

nickwalkmsft · 2017-12-04T17:58:59Z

Kudu.Services/ServiceHookHandlers/FetchHandler.cs

@@ -118,6 +118,7 @@ public class FetchHandler : HttpTaskAsyncHandler
                        context.ApplicationInstance.CompleteRequest();
                        break;
                    case FetchDeploymentRequestResult.RanSynchronously:
+                    case FetchDeploymentRequestResult.RanSynchronouslyFailed:


This is actually the reason I left the feature in question out of Zipdeploy - because synchronous fetch deployment didn't do it. I didn't just want to add it and change the expected behavior, and it made more sense (to me, at the time) to keep the behavior the same between fetch and zip/push, so I did. @davidebbo what's your take on this?

So is the /api/zip behavior today that if something goes wrong on the server, the request doesn't fail? If so, that doesn't seem good, and I wouldn't make push zip behave the same way (ideally we'd fix both).

I changed fetch to act the same way as push on RanSynchronously, but I'm not 100% sure of the consequences here so feedback is appreciated. Tests seems to run fine at least 💯

Not /api/zip, but /deploy (fetch deployment), which zipdeploy has the most in common with (a zip deployment just pushes the zip to the server and then does a "fetch" deployment with a custom fetch action that unzips the local file and sets the deployment source to the extract directory).

In DeploymentManager.Build(), the call to builder.Build() (which does most of the actual work of the deployment) is wrapped in a try/catch that doesn't rethrow. See https://github.com/projectkudu/kudu/blob/master/Kudu.Core/Deployment/DeploymentManager.cs#L651-L675. Everything further up the call stack relies on exceptions and doesn't actually check the status, so the caller gets a 200 back if an exception occurs there, even though the deployment failed. It's always been this way.

Ah I see. I guess the reason is that in those scenarios, the http call normally comes from GitHub/BitBucket, and the error is less relevant. Instead, the deployment status shows it. But for Zip, I can see how not having an error becomes more quirky.

nickwalkmsft · 2017-12-06T18:11:53Z

Okey doke - overall I think this is a good idea and would like to merge it. Could you make a couple of revisions? I am adding a couple comments now.

Thanks for the contrib!

nickwalkmsft · 2017-12-06T18:15:27Z

Kudu.Services/ServiceHookHandlers/FetchHandler.cs

                        context.Response.StatusCode = (int)HttpStatusCode.Conflict;
                        context.Response.Write(Resources.Error_DeploymentInProgress);
                        context.ApplicationInstance.CompleteRequest();
                        break;
-                    case FetchDeploymentRequestResult.RanSynchronously:
+                    case FetchDeploymentRequestResultStatus.RanSynchronously:


This new behavior is good for PushDeploymentController since it's new and likely to be used interactively, but here for FetchHandler, please put this bit back how it was (have RanSynchronously fall through to the default case), and have RanSynchronouslyFailed fall through to the default case as well. I'm hesitant to change the behavior here, since this is generally called by webhooks from many different CI services and I don't want to break expectations related to how they react to the response. A short comment to acknowledge the RanSynchronouslyFailed case and explaining that we're avoiding a behavior change would be good also.

Reverted to default case

nickwalkmsft · 2017-12-06T18:30:41Z

Kudu.Services/Deployment/PushDeploymentController.cs

                        response.StatusCode = HttpStatusCode.OK;
                        break;
-                    case FetchDeploymentRequestResult.ConflictDeploymentInProgress:
+                    case FetchDeploymentRequestResultStatus.RanSynchronouslyFailed:
+                        response.StatusCode = HttpStatusCode.BadRequest;


I misspoke on this earlier; it should probably be a 500 instead of a 400.

nickwalkmsft · 2017-12-06T18:43:07Z

Kudu.Core/Deployment/FetchDeploymentManager.cs

                    }

                    await PerformDeployment(deployInfo);
-                    return FetchDeploymentRequestResult.RanSynchronously;
+                    var result = _deploymentManager.GetResults().FirstOrDefault();


GetResults() is not a good idea here (it hits the filesystem to enumerate all the existing results, does locking stuff, and has the side effect of purging old deployment records, none of which I want to incur here) but unfortunately there's no other option the way things are factored now.

DeploymentManager.DeployAsync() currently doesn't return anything. Given the functionality it would make sense to return the DeployResult, and in turn that could be returned up through PerformDeployment() and checked here. Please give that a go; if it's not looking great let me know and I'll take a swing at it or something similar.

Now using GetResult(id) to return the DeployResult from DeployAsync. Does that solve the lockingissues?

nikolaia · 2017-12-08T10:24:31Z

Kudu.Core/Deployment/DeploymentManager.cs

@@ -274,6 +274,8 @@ public void Delete(string id)
                {
                    throw new DeploymentFailedException(exception);
                }
+
+                return GetResult(id);


Is this safe?

Sorry for the delay, I'll get to this soon...

Bump! Don't wanna nag, but thought I'd remind you about this in case it's been forgotten ⏳

Thanks @nikolaia, have been working on other priorities but will get to this as soon as I can, it's still on my radar.

Any updates here? :)

honorew · 2019-01-23T06:03:41Z

when isAsync=true,could return the deploy id ?

nikolaia · 2019-02-10T17:04:49Z

I'm gonna close this. Ended up just using the Azure CLI to deploy. (Example: https://github.com/nikolaia/weather-in-harstad/blob/master/provision.fsx#L82)

dnfclas added cla-required and removed cla-required labels Dec 4, 2017

davidebbo requested a review from nickwalkmsft December 4, 2017 13:58

nickwalkmsft reviewed Dec 4, 2017

View reviewed changes

ghost deleted a comment from dnfclas Dec 5, 2017

ghost removed the cla-signed label Dec 5, 2017

nickwalkmsft suggested changes Dec 6, 2017

View reviewed changes

nikolaia commented Dec 8, 2017

View reviewed changes

nikolaia mentioned this pull request Mar 1, 2018

Add support for Kudu Zip Deploy fsprojects/FAKE#1757

Merged

6 tasks

Give better feedback on synchronous zipdeploy

f32d0f0

nikolaia force-pushed the f_zipdeploy_failed_sync_example branch from de572eb to f32d0f0 Compare March 21, 2018 09:38

nikolaia closed this Feb 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give better feedback on synchronous zipdeploy #2641

Give better feedback on synchronous zipdeploy #2641

nikolaia commented Dec 4, 2017

davidebbo commented Dec 4, 2017

nickwalkmsft Dec 4, 2017

nickwalkmsft Dec 4, 2017

nikolaia Dec 5, 2017

nickwalkmsft Dec 4, 2017

davidebbo Dec 4, 2017

nikolaia Dec 5, 2017

nickwalkmsft Dec 5, 2017 •

edited

Loading

davidebbo Dec 6, 2017

nickwalkmsft commented Dec 6, 2017

nickwalkmsft Dec 6, 2017

nikolaia Dec 8, 2017

nickwalkmsft Dec 6, 2017

nikolaia Dec 8, 2017

nickwalkmsft Dec 6, 2017

nikolaia Dec 8, 2017 •

edited

Loading

nikolaia Dec 8, 2017

nickwalkmsft Dec 12, 2017

nikolaia Jan 9, 2018

nickwalkmsft Jan 9, 2018

christiansyoung Nov 2, 2018

honorew commented Jan 23, 2019

nikolaia commented Feb 10, 2019

Give better feedback on synchronous zipdeploy #2641

Give better feedback on synchronous zipdeploy #2641

Conversation

nikolaia commented Dec 4, 2017

davidebbo commented Dec 4, 2017

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

nickwalkmsft Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickwalkmsft commented Dec 6, 2017

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

nikolaia Dec 8, 2017 • edited Loading

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

honorew commented Jan 23, 2019

nikolaia commented Feb 10, 2019

nickwalkmsft Dec 5, 2017 •

edited

Loading

nikolaia Dec 8, 2017 •

edited

Loading