deployresult synchronization and cleanup #471

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Owner

suwatch commented Mar 21, 2013

(1) trim the deployments on reading. see PurgeDeploymentsTests for various scenarios being trim.
(2) instead of fail/retry, we do a proper lock on read/write to status file. I did not do it for log.xml since there is no known race access pattern from Portal.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Contracts/Infrastructure/LockExtensions.cs
+ public static bool TryLockOperation(this IOperationLock lockObj,
+ Action operation,
+ TimeSpan timeout)
+ {
+ var interval = TimeSpan.FromMilliseconds(250);
+ var elapsed = TimeSpan.Zero;
+
+ while (!lockObj.Lock())
+ {
+ if (elapsed >= timeout)
+ {
+ return false;
+ }
+
+ Thread.Sleep(interval);
+ elapsed += interval;
@davidebbo

davidebbo Mar 22, 2013

Owner

Seems like a slighly imprecise way of counting time compared to starting with DateTime.Now and comparing with latest Now. I guess you have more of an iteration count than a timeout.

@suwatch

suwatch Mar 22, 2013

Owner

umm... how is that imprecise.

  1. timeout <= Zero, then we only wait once.
  2. if timeout > Zero, we will try and wait in multiple of 250ms

we could have made Sleep be precise about the final remainder .. I am sure one could implement with DateTime.Now. BTW i just follow on existing codes. https://github.com/projectkudu/kudu/blob/master/Kudu.Core/Infrastructure/LockFile.cs#L129

@davidebbo

davidebbo Mar 22, 2013

Owner

It's imprecise because it doesn't account for time spent between the sleeps. In this case it probably doesn't matter because it doesn't do much, but generally, it's best to have a fixed time reference point if you want a precise timeout.

@suwatch

suwatch Mar 22, 2013

Owner

Ok, got it.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/OperationLockTests.cs
@@ -0,0 +1,137 @@
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Kudu.Contracts.Infrastructure;
+using Moq;
+using Xunit;
+using Xunit.Extensions;
+
+namespace Kudu.FunctionalTests
@davidebbo

davidebbo Mar 22, 2013

Owner

Should be Kudu.Core.Test right?

@suwatch

suwatch Mar 22, 2013

Owner

yes. I started it as Functional and realized it should be in Core - so I moved it there and forgot to change the namespace.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/OperationLockTests.cs
+ public void LockMultipleTest()
+ {
+ // Mock
+ var lockObj = MockOperationLock();
+ var actual = 0;
+ var threads = 5;
+ var tasks = new List<Task>();
+
+ // Test
+ for (int i = 0; i < threads; i++)
+ {
+ tasks.Add(Task.Factory.StartNew(() =>
+ {
+ bool succeed = lockObj.TryLockOperation(() =>
+ {
+ var temp = actual;
@davidebbo

davidebbo Mar 22, 2013

Owner

Is temp needed since it's under the lock?

@suwatch

suwatch Mar 22, 2013

Owner
                    var temp = actual;
                    Thread.Sleep(500);
                    actual = temp + 1;

The goal is to simulate delay get and set (typical synchronized problem if not lock properly). I made them more obvious so the test will fail if the lock does not do what it is supposed to do.

I will add a comment.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/OperationLockTests.cs
+ {
+ tasks.Add(Task.Factory.StartNew(() =>
+ {
+ return lockObj.TryLockOperation(() =>
+ {
+ var temp = actual;
+ Thread.Sleep(5000);
+ actual = temp + 1;
+ }, TimeSpan.FromSeconds(1));
+ }));
+ }
+
+ Task.WaitAll(tasks.ToArray());
+
+ // Assert
+ Assert.True(tasks[0].Result ^ tasks[1].Result);
@davidebbo

davidebbo Mar 22, 2013

Owner

Easier to read if you replace ^ by != :)

@suwatch

suwatch Mar 22, 2013

Owner

On the other hand, ^ is way cooler than this boring !=. How many time you get to write a line of codes with ^! I will fix it anyhow.

@davidebbo

davidebbo Mar 22, 2013

Owner

Miss the old days when it was cool to write a ^= b ^= a ^= b to swap two variables ;)

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+using Kudu.Core.Deployment;
+using Kudu.Core.Tracing;
+using Moq;
+using Xunit;
+using Xunit.Extensions;
+
+namespace Kudu.FunctionalTests
+{
+ public class PurgeDeploymentsTests
+ {
+ private static int _id;
+ private MethodInfo _method;
+
+ public PurgeDeploymentsTests()
+ {
+ _method = typeof(DeploymentManager).GetMethod("PurgeDeployments",
@davidebbo

davidebbo Mar 22, 2013

Owner

That's kind of ugly. Can't we just make it internal? I think we already have InternalsVisibleTo set up.

@suwatch

suwatch Mar 22, 2013

Owner

Didn't know that. I fixed it (call directly - no reflection).

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
@@ -0,0 +1,210 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using System.Threading;
+using System.Threading.Tasks;
+using Kudu.Contracts.Infrastructure;
+using Kudu.Contracts.Tracing;
+using Kudu.Core.Deployment;
+using Kudu.Core.Tracing;
+using Moq;
+using Xunit;
+using Xunit.Extensions;
+
+namespace Kudu.FunctionalTests
@davidebbo

davidebbo Mar 22, 2013

Owner

Kudu.Core.Test

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+ // Mock
+ var toDelete = actual.Where(r => r.Id.StartsWith("delete-"));
+ var expect = actual.Where(r => !r.Id.StartsWith("delete-"));
+ var manager = MockDeploymentManager(id =>
+ {
+ Assert.True(toDelete.Any(r => r.Id == id));
+ toDelete = toDelete.Where(r => r.Id != id);
+ });
+
+ // Test
+ var results = (IEnumerable<DeployResult>)_method.Invoke(manager, new object[] { actual });
+
+ // Assert
+ Assert.False(toDelete.Any());
+ Assert.Equal(expect.Count(), results.Count());
+ for (int i = 0; i < expect.Count(); ++i)
@davidebbo

davidebbo Mar 22, 2013

Owner

There is probably a more Linq-ish way of testing enumeration equality, but I wouldn't use it myself :)

@pranavkm

pranavkm Mar 22, 2013

Contributor

You can replace this with Assert.Equal(expect, results). Assert.Equal has an overload which takes IEnumerable

@suwatch

suwatch Mar 22, 2013

Owner
        Assert.Equal(expect, results, DeployResultComparer.Instance);

Done!

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+ return list;
+ }
+
+ private static IEnumerable<DeployResult> GetTooManyItemsWithPendings()
+ {
+ var list = CreateMaxSuccessItems();
+ list.Insert(0, CreateResult(DeployStatus.Failed, isTemp: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ list.Add(CreateResult(DeployStatus.Pending, lastSuccess: true));
+ list.Add(CreateResult(DeployStatus.Building, lastSuccess: true));
+ list.Add(CreateResult(DeployStatus.Deploying, lastSuccess: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ return list;
+ }
+
+ private static List<DeployResult> CreateMaxSuccessItems()
@davidebbo

davidebbo Mar 22, 2013

Owner

The method name implies success, but the code seems to alternate success/fail

@suwatch

suwatch Mar 22, 2013

Owner

Change to CreateMaxLastSuccessItems. This is the items that ever success.

Add comment to help with code read.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core/Deployment/DeploymentManager.cs
@@ -265,6 +272,107 @@ public static ChangeSet CreateTemporaryChangeSet(string authorName = null, strin
};
}
+ // since the expensive part (reading all files) is done,
+ // we opt for simplicity rather than performance when purging.
+ private IEnumerable<DeployResult> PurgeDeployments(IEnumerable<DeployResult> results)
@davidebbo

davidebbo Mar 22, 2013

Owner

Not clear looking at this that the input is always sorted with newest first. Add comment.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+ private static IEnumerable<DeployResult> GetMultipleFails()
+ {
+ return new[]
+ {
+ CreateResult(DeployStatus.Failed, lastSuccess: false),
+ CreateResult(DeployStatus.Failed, lastSuccess: true),
+ CreateResult(DeployStatus.Failed, lastSuccess: false, toDelete: true),
+ CreateResult(DeployStatus.Failed, lastSuccess: true),
+ };
+ }
+
+ private static IEnumerable<DeployResult> GetMultiplePendings()
+ {
+ return new[]
+ {
+ CreateResult(DeployStatus.Pending, isTemp: true, toDelete: true),
@davidebbo

davidebbo Mar 22, 2013

Owner

Can there be that many pending items in real world scenarios?

@suwatch

suwatch Mar 22, 2013

Owner

They can be two at most (and one is always Temporary). If it works for N, it works for two. The test try to illustrate we always prefer non-temporary pending > temporary one.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+ {
+ var list = CreateMaxSuccessItems();
+ list.Insert(0, CreateResult(DeployStatus.Pending, isTemp: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ return list;
+ }
+
+ private static IEnumerable<DeployResult> GetTooManyItemsWithActive()
+ {
+ var list = CreateMaxSuccessItems();
+ list.Insert(0, CreateResult(DeployStatus.Pending, isTemp: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ list.Add(CreateResult(DeployStatus.Success, isActive: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ return list;
@davidebbo

davidebbo Mar 22, 2013

Owner

Here do we end up deleting one of the items from initial list, or do we end up with max+2?

@suwatch

suwatch Mar 22, 2013

Owner

in stable state, most we end up is Max.

When deploying new item, it will be Max + 1 (max succeeded item + pending/fail).

In extremely rare case, max + 2 (where redeploying/resyncing active/pending is not part of max succeeded item).

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core.Test/PurgeDeploymentsTests.cs
+ var list = CreateMaxSuccessItems();
+ list.Insert(0, CreateResult(DeployStatus.Pending, isTemp: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ list.Add(CreateResult(DeployStatus.Success, isActive: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ return list;
+ }
+
+ private static IEnumerable<DeployResult> GetTooManyItemsWithPendings()
+ {
+ var list = CreateMaxSuccessItems();
+ list.Insert(0, CreateResult(DeployStatus.Failed, isTemp: true));
+ list.Add(CreateResult(DeployStatus.Success, toDelete: true));
+ list.Add(CreateResult(DeployStatus.Pending, lastSuccess: true));
+ list.Add(CreateResult(DeployStatus.Building, lastSuccess: true));
+ list.Add(CreateResult(DeployStatus.Deploying, lastSuccess: true));
@davidebbo

davidebbo Mar 22, 2013

Owner

How can these items that are way down the list still be pending? And shouldn't they get cleaned up?

@suwatch

suwatch Mar 22, 2013

Owner

This is to illustrate that we will honor all active/pending items that falls of the max. In real life, it will only be one item.

@davidebbo davidebbo commented on the diff Mar 22, 2013

Kudu.Core/Deployment/DeploymentStatusManager.cs
}
public IDeploymentStatusFile Open(string id)
{
- return DeploymentStatusFile.Open(id, _fileSystem, _environment);
+ return DeploymentStatusFile.Open(id, _fileSystem, _environment, _statusLock);
+ }
+
+ public void Delete(string id)
+ {
+ string path = Path.Combine(_environment.DeploymentCachePath, id);
+
+ _statusLock.LockOperation(() =>
+ {
+ if (_fileSystem.Directory.Exists(path))
+ {
+ _fileSystem.Directory.Delete(path, recursive: true);
@davidebbo

davidebbo Mar 22, 2013

Owner

If this fails due to a random lock, will it cause the whole deployment status API to blow up?

@suwatch

suwatch Mar 22, 2013

Owner

if everything behaves properly, once we have a lock, no one should be modifying, reading any file and its content. The exist and, then, delete should work. However, if delete fail (really file system issue), we could opt to tolerate it (SafeDelete). The reading part does similarly. Let discuss in person.

Owner

davidebbo commented Mar 22, 2013

Unit test framework looks great. It's probably best if you walk me through some of the scenarios in person to clarify them as it's kind of subtle.

@pranavkm pranavkm commented on the diff Mar 22, 2013

Kudu.Core/Deployment/DeploymentManager.cs
@@ -31,6 +31,7 @@ public class DeploymentManager : IDeploymentManager
private const string LogFile = "log.xml";
private const string ManifestFile = "manifest";
private const string TemporaryDeploymentIdPrefix = "temp-";
+ public const int MaxSuccessDeploymentResults = 10;
@pranavkm

pranavkm Mar 22, 2013

Contributor

Would there be any benefit to making this a setting?

@suwatch

suwatch Mar 22, 2013

Owner

I leave it be for now. Let hear what users think and go from there.

@pranavkm pranavkm commented on the diff Mar 22, 2013

Kudu.Core/Deployment/DeploymentManager.cs
+ }
+
+ results = results.Where(r => !toDelete.Any(i => i.Id == r.Id));
+ }
+ }
+
+ return results;
+ }
+
+ private static IEnumerable<DeployResult> GetPurgeTemporaryDeployments(IEnumerable<DeployResult> results)
+ {
+ var toDelete = new List<DeployResult>();
+
+ // more than one pending/building, remove all temporary pending
+ var pendings = results.Where(r => r.Status != DeployStatus.Failed && r.Status != DeployStatus.Success);
+ if (pendings.Count() > 1)
@pranavkm

pranavkm Mar 22, 2013

Contributor

A slightly better way of checking more than one is calling Skip(1).Any(), but might not make much of a difference for small sequences

@davidebbo

davidebbo Mar 22, 2013

Owner

I think the decrease in readibility outweight so minute perf gain here (given the very small sizes of the lists). So I'd leave it alone.

Linq needs a list.HasAtLeast(n) extension :)

suwatch closed this Mar 22, 2013

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