Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CopyFileAsync to ICoreFileStorageService #5581

Merged
merged 1 commit into from
Mar 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Net;
Expand All @@ -16,6 +17,15 @@ namespace NuGetGallery
{
public class CloudBlobCoreFileStorageService : ICoreFileStorageService
{
/// <summary>
/// This is the maximum duration for <see cref="CopyFileAsync(string, string, string, string)"/> to poll,
/// waiting for a package copy to complete. The value picked today is based off of the maximum duration we wait
/// when uploading files to Azure China blob storage. Note that in cases when the copy source and destination
/// are in the same container, the copy completed immediately and no polling is necessary.
/// </summary>
private static readonly TimeSpan MaxCopyDuration = TimeSpan.FromMinutes(10);
private static readonly TimeSpan CopyPollFrequency = TimeSpan.FromMilliseconds(500);

private static readonly HashSet<string> KnownPublicFolders = new HashSet<string> {
CoreConstants.PackagesFolderName,
CoreConstants.PackageBackupsFolderName,
Expand Down Expand Up @@ -100,6 +110,93 @@ public async Task<IFileReference> GetFileReferenceAsync(string folderName, strin
}
}

public async Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check input

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

{
if (srcFolderName == null)
{
throw new ArgumentNullException(nameof(srcFolderName));
}

if (srcFileName == null)
{
throw new ArgumentNullException(nameof(srcFileName));
}

if (destFolderName == null)
{
throw new ArgumentNullException(nameof(destFolderName));
}

if (destFileName == null)
{
throw new ArgumentNullException(nameof(destFileName));
}

var srcContainer = await GetContainerAsync(srcFolderName);
var srcBlob = srcContainer.GetBlobReference(srcFileName);

var destContainer = await GetContainerAsync(destFolderName);
var destBlob = destContainer.GetBlobReference(destFileName);

// Determine the source blob etag.
await srcBlob.FetchAttributesAsync();
var srcAccessCondition = AccessCondition.GenerateIfMatchCondition(srcBlob.ETag);

// Check if the destination blob already exists and fetch attributes.
var destAccessCondition = AccessCondition.GenerateIfNotExistsCondition();
if (await destBlob.ExistsAsync())
{
if (destBlob.CopyState?.Status == CopyStatus.Failed)
{
// If the last copy failed, allow this copy to occur.
destAccessCondition = AccessCondition.GenerateIfMatchCondition(destBlob.ETag);
}
else if ((srcBlob.Properties.ContentMD5 != null
&& srcBlob.Properties.ContentMD5 == destBlob.Properties.ContentMD5
&& srcBlob.Properties.Length == destBlob.Properties.Length))
{
// If the blob hash is the same and the length is the same, no-op the copy.
return;
}
}

// Start the server-side copy and wait for it to complete.
try
{
await destBlob.StartCopyAsync(
srcBlob,
srcAccessCondition,
destAccessCondition);
}
catch (StorageException ex) when (ex.RequestInformation?.HttpStatusCode == (int?)HttpStatusCode.Conflict)
{
throw new InvalidOperationException(
String.Format(
CultureInfo.CurrentCulture,
"There is already a blob with name {0} in container {1}.",
destFileName,
destFolderName),
ex);
}

var stopwatch = Stopwatch.StartNew();
while (destBlob.CopyState.Status == CopyStatus.Pending
&& stopwatch.Elapsed < MaxCopyDuration)
{
await destBlob.FetchAttributesAsync();
await Task.Delay(CopyPollFrequency);
}

if (destBlob.CopyState.Status == CopyStatus.Pending)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we kill the copy operation in those cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided against this because we can still recover if the caller calls this CopyFileAsync method later. Consider if the timeout occurs, exception is thrown, and orchestrator tries again via service bus message. Things can still work out.

I would rather take this approach than aborting and trying again over and over.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(since would rather trust blob storage server side to get the job done eventually than us trying to start it every N minutes)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we trigger a copy operation and the previous copy is still in progress?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't even start. We have a precondition If-None-Match: *.

{
throw new TimeoutException($"Waiting for the blob copy operation to complete timed out after {MaxCopyDuration.TotalSeconds} seconds.");
}
else if (destBlob.CopyState.Status != CopyStatus.Success)
{
throw new StorageException($"The blob copy operation had copy status {destBlob.CopyState.Status} ({destBlob.CopyState.StatusDescription}).");
}
}

public async Task SaveFileAsync(string folderName, string fileName, Stream packageFile, bool overwrite = true)
{
ICloudBlobContainer container = await GetContainerAsync(folderName);
Expand Down
28 changes: 26 additions & 2 deletions src/NuGetGallery.Core/Services/CloudBlobWrapper.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IO;
using System.Net;
Expand All @@ -12,9 +13,9 @@ namespace NuGetGallery
{
public class CloudBlobWrapper : ISimpleCloudBlob
{
private readonly ICloudBlob _blob;
private readonly CloudBlockBlob _blob;

public CloudBlobWrapper(ICloudBlob blob)
public CloudBlobWrapper(CloudBlockBlob blob)
{
_blob = blob;
}
Expand All @@ -24,6 +25,11 @@ public BlobProperties Properties
get { return _blob.Properties; }
}

public CopyState CopyState
{
get { return _blob.CopyState; }
}

public Uri Uri
{
get { return _blob.Uri; }
Expand Down Expand Up @@ -137,6 +143,24 @@ public string GetSharedReadSignature(DateTimeOffset? endOfAccess)
return signature;
}

public async Task StartCopyAsync(ISimpleCloudBlob source, AccessCondition sourceAccessCondition, AccessCondition destAccessCondition)
{
// To avoid this we would need to somehow abstract away the primary and secondary storage locations. This
// is not worth the effort right now!
var sourceWrapper = source as CloudBlobWrapper;
if (sourceWrapper == null)
{
throw new ArgumentException($"The source blob must be a {nameof(CloudBlobWrapper)}.");
}

await _blob.StartCopyAsync(
sourceWrapper._blob,
sourceAccessCondition: sourceAccessCondition,
destAccessCondition: destAccessCondition,
options: null,
operationContext: null);
}

// The default retry policy treats a 304 as an error that requires a retry. We don't want that!
private class DontRetryOnNotModifiedPolicy : IRetryPolicy
{
Expand Down
12 changes: 12 additions & 0 deletions src/NuGetGallery.Core/Services/ICoreFileStorageService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,17 @@ public interface ICoreFileStorageService
Task<Uri> GetFileReadUriAsync(string folderName, string fileName, DateTimeOffset? endOfAccess);

Task SaveFileAsync(string folderName, string fileName, Stream packageFile, bool overwrite = true);

/// <summary>
/// Copies the source file to the destination file. If the destination already exists and the content
/// is different, an exception should be thrown. If the file already exists, the implementation can choose to
/// no-op if the content is the same instead of throwing an exception. This method should throw if the source
/// file does not exist.
/// </summary>
/// <param name="srcFolderName">The source folder.</param>
/// <param name="srcFileName">The source file name or relative file path.</param>
/// <param name="destFolderName">The destination folder.</param>
/// <param name="destFileName">The destination file name or relative file path.</param>
Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName);
}
}
3 changes: 3 additions & 0 deletions src/NuGetGallery.Core/Services/ISimpleCloudBlob.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace NuGetGallery
public interface ISimpleCloudBlob
{
BlobProperties Properties { get; }
CopyState CopyState { get; }
Uri Uri { get; }
string Name { get; }
DateTime LastModifiedUtc { get; }
Expand All @@ -27,6 +28,8 @@ public interface ISimpleCloudBlob

Task FetchAttributesAsync();

Task StartCopyAsync(ISimpleCloudBlob source, AccessCondition sourceAccessCondition, AccessCondition destAccessCondition);

/// <summary>
/// Generates the shared read signature that if appended to the blob URI
/// would allow reading the contents of the blob using the produced URI
Expand Down
40 changes: 40 additions & 0 deletions src/NuGetGallery/Services/FileSystemFileStorageService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Globalization;
using System.IO;
using System.Security.Cryptography;
using System.Threading.Tasks;
using System.Web.Hosting;
using System.Web.Mvc;
Expand Down Expand Up @@ -168,6 +169,45 @@ public Task SaveFileAsync(string folderName, string fileName, Stream packageFile
return Task.FromResult(0);
}

public Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check input

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

{
if (srcFolderName == null)
{
throw new ArgumentNullException(nameof(srcFolderName));
}

if (srcFileName == null)
{
throw new ArgumentNullException(nameof(srcFileName));
}

if (destFolderName == null)
{
throw new ArgumentNullException(nameof(destFolderName));
}

if (destFileName == null)
{
throw new ArgumentNullException(nameof(destFileName));
}

var srcFilePath = BuildPath(_configuration.FileStorageDirectory, srcFolderName, srcFileName);
var destFilePath = BuildPath(_configuration.FileStorageDirectory, destFolderName, destFileName);

_fileSystemService.CreateDirectory(Path.GetDirectoryName(destFilePath));

try
{
_fileSystemService.Copy(srcFilePath, destFilePath, overwrite: false);
}
catch (IOException e)
{
throw new InvalidOperationException("Could not copy because destination file already exists", e);
}

return Task.CompletedTask;
}

public Task<bool> IsAvailableAsync()
{
return Task.FromResult(Directory.Exists(_configuration.FileStorageDirectory));
Expand Down
5 changes: 5 additions & 0 deletions src/NuGetGallery/Services/FileSystemService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,10 @@ public IFileReference GetFileReference(string path)
var info = new FileInfo(path);
return info.Exists ? new LocalFileReference(info) : null;
}

public virtual void Copy(string sourceFileName, string destFileName, bool overwrite)
{
File.Copy(sourceFileName, destFileName, overwrite);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather use the high level file system API on File. We don't use this in production so I'd rather favor small code. We can switch to async later if the performance will be helped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some tests. Performance is worse.

}
}
}
2 changes: 2 additions & 0 deletions src/NuGetGallery/Services/IFileSystemService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,7 @@ public interface IFileSystemService
DateTimeOffset GetCreationTimeUtc(string path);

IFileReference GetFileReference(string path);

void Copy(string sourceFileName, string destFileName, bool overwrite);
}
}
Loading