Add storage connection strings for each distinct purpose in the gallery #4568

joelverhagen · 2017-08-16T18:00:24Z

Replaced configuration Gallery.AzureStorageConnectionString with new variables:

Gallery.AzureStorage.Auditing.ConnectionString
Gallery.AzureStorage.Content.ConnectionString
Gallery.AzureStorage.Errors.ConnectionString
Gallery.AzureStorage.NuGetExe.ConnectionString
Gallery.AzureStorage.Packages.ConnectionString
Gallery.AzureStorage.Statistics.ConnectionString
Gallery.AzureStorage.Uploads.ConnectionString

Why the dots in the configuration name? I think the proper mental model here is that each storage purpose (e.g. Errors) may eventually have additional properties. For example, the Gallery.AzureStorageReadAccessGeoRedundant setting could become account-specific or we could configure the table/container name that Uploads go into (for example). For that reason, I imagined the future where Gallery.AzureStorage.Statistics would refer to an object with a set of properties. Today there is only one property: ConnectionString.

Why the underscores in the property names? Today, the code that populates the AppConfiguration instance does not work on complex objects -- just simple properties. The underscore is basically a way to mimic complex objects in a flat list of properties like IAppConfiguration.

The basis of this implementation is telling our DI container (Autofac) which connection string to use for different services. We now need N instances of CloudBlobFileStorageService, instead of just one. Since this means we will have multiple implementations of IFileStorageService available in the DI container, we need a way to differentiate between them. We do this with keyed services. For more information about this general DI project, see here:
http://docs.autofac.org/en/latest/faq/select-by-context.html

After this PR is approved and before I merge the PR, I will add the new configuration values to our DEV, INT, and PROD configuration files (.cscfg).

Fix #4554.

joelverhagen · 2017-08-16T18:01:59Z

Note that the meat of the work is done in src/NuGetGallery/App_Start/DefaultDependenciesModule.cs, which is showing as collapsed in the GitHub PR diff view. You must explicitly click Load diff to see these changed.

loic-sharma · 2017-08-16T19:35:53Z

src/NuGetGallery.Core/Auditing/CloudAuditingService.cs

+
+        public async Task<bool> IsAvailableAsync()
+        {
+            return await _auditContainer.ExistsAsync();


Can you just return here without await?

In general I prefer await since it eases refactoring, but I don't care much. I'll remove the await.

loic-sharma · 2017-08-16T19:37:19Z

src/NuGetGallery/Services/CloudReportService.cs

+        public async Task<bool> IsAvailableAsync()
+        {
+            var container = GetCloudBlobContainer();
+            return await container.ExistsAsync();


Same thing here, is await actually needed?

Same as above.

loic-sharma · 2017-08-16T19:39:17Z

src/NuGetGallery/Services/CloudReportService.cs

@@ -49,5 +45,18 @@ public CloudReportService(string connectionString, bool readAccessGeoRedundant)

            return new StatisticsReport(content, blob.Properties.LastModified?.UtcDateTime);
        }
+
+        private CloudBlobContainer GetCloudBlobContainer()


Should the CloudBlobContainer be cached? Or do we want to fetch a fresh container reference each time?

I prefer to keep this out of scope -- we would need to analyze whether we would be regressing anything here by caching.

loic-sharma · 2017-08-16T20:11:52Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+
+            var connectionStringToBindingKey = dependents
+                .GroupBy(d => d.AzureStorageConnectionString ?? DefaultBindingKey)
+                .ToDictionary(g => g.Key, g => string.Join(" ", g.Select(d => d.ImplementationType)));


Should you use d.ImplementationType.FullName here instead to be consistent with StorageDependent.Create?

This should actually be g.Select(d => d.BindingKey).

dtivel · 2017-08-16T19:56:36Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+            var completedBindingKeys = new HashSet<string>();
+            foreach (var dependent in GetStorageDependents(configuration.Current))
+            {
+                if (!completedBindingKeys.Contains(dependent.BindingKey))


You could just call Add(..) without ! here.

dtivel · 2017-08-16T20:15:37Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+            public Type ImplementationType { get; }
+
+            /// <summary>
+            /// The storage dependent's interface tyep.


Oops, a tyepo

agr · 2017-08-16T20:21:16Z

src/NuGetGallery/Configuration/AppConfiguration.cs

+        public string AzureStorage_Statistics_ConnectionString { get; set; }
+
+        [DisplayName("AzureStorage.Uploads.ConnectionString")]
+        public string AzureStorage_Uploads_ConnectionString { get; set; }


Why use underscores in the property names?

Why the underscores in the property names? Today, the code that populates the AppConfiguration instance does not work on complex objects -- just simple properties. The underscore is basically a way to mimic complex objects in a flat list of properties like IAppConfiguration.

agr · 2017-08-16T21:00:25Z

src/NuGetGallery/Services/StatusService.cs

@@ -19,7 +20,7 @@ public class StatusService : IStatusService
    {
        private readonly HttpClient _httpClient = new HttpClient();
        private readonly IEntitiesContext _entities;
-        private readonly IFileStorageService _fileStorageService;
+        private readonly IEnumerable<ICloudStorageAvailabilityCheck> _cloudStorageAvailabilityChecks;


Use ICollection instead of IEnumerable. The code potentially reads it multiple times and generally, IEnumerable does not support multiple iterations over it.

Yeah, good point. Who knows how Autofac implements this (I don't care to know)

scottbommarito

nitpicks, looks awesome

scottbommarito · 2017-08-16T19:29:50Z

src/NuGetGallery/Configuration/IAppConfiguration.cs

+        string AzureStorage_Errors_ConnectionString { get; set; }
+
+        /// <summary>
+        /// The Azure Storage connection string used downloading nuget.exe.


used downloading -> used for downloading

scottbommarito · 2017-08-16T21:35:06Z

tests/NuGetGallery.Facts/App_Start/DefaultDependenciesModuleFacts.cs

+    public class DefaultDependenciesModuleFacts
+    {
+        [Fact]
+        public void AllTypesDependingOnIFileStorageServiceAreReturnedByGetStorageDependents()


These tests are amazing and absolutely get rid of any concerns I had about the DI approach.

Great job.

scottbommarito · 2017-08-16T21:40:49Z

src/NuGetGallery.Core/ICloudStorageAvailabilityCheck.cs

+    /// Determines whether a cloud storage object (e.g. a blob container) is available. The purpose of this interface
+    /// is to assess a gallery instance's connection to Azure Storage.
+    /// </summary>
+    public interface ICloudStorageAvailabilityCheck


nit: semantically, ICloudStorageAvailabilityCheck doesn't make a whole lot of sense--this is an object that can be checked for availability, not an availability check itself. Something along the lines of ICloudStorageResource or IStatusDependency would make more sense to me (there's nothing about this resource that implies that we must be checking a cloud storage object for availability).

scottbommarito · 2017-08-16T21:42:26Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+
+        private static void ConfigureForAzureStorage(ContainerBuilder builder, IGalleryConfigurationService configuration)
+        {
+            var completedBindingKeys = new HashSet<string>();


Given the complexity of this code, it may be beneficial to add some comments here, much like you did in the summary of this PR, describing what this code is and what it's accomplishing.

scottbommarito · 2017-08-16T21:45:04Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+        /// <summary>
+        /// Represents a type that depends on <see cref="IFileStorageService"/>.
+        /// </summary>
+        internal class StorageDependent


This file is already massive, it may be better to move this as well as GetStorageDependents to its own file.

scottbommarito · 2017-08-16T21:46:21Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+        {
+            const string DefaultBindingKey = "Default";
+
+            var dependents = new[]


May want to add a comment here detailing that this array must be added to when a new StorageDependent is added.

scottbommarito · 2017-08-16T21:48:55Z

tests/NuGetGallery.Facts/App_Start/DefaultDependenciesModuleFacts.cs

+
+namespace NuGetGallery
+{
+    public class DefaultDependenciesModuleFacts


nit: These are really just StorageDependentFacts. As suggested above, you should move StorageDependent to its own file and then rename this class.

…ry (#4568) Fix #4554

joelverhagen requested review from agr, loic-sharma, shishirx34, ryuyu, dtivel, chenriksson, skofman1 and scottbommarito August 16, 2017 18:00

dnfclas added the cla-already-signed label Aug 16, 2017

loic-sharma reviewed Aug 16, 2017

View reviewed changes

dtivel approved these changes Aug 16, 2017

View reviewed changes

agr reviewed Aug 16, 2017

View reviewed changes

scottbommarito approved these changes Aug 16, 2017

View reviewed changes

Add storage connection strings for each distinct purpose in the galle…

bb6201d

…ry (#4568) Fix #4554

joelverhagen merged commit bb6201d into NuGet:dev Aug 18, 2017

joelverhagen deleted the storage-config branch August 22, 2017 21:40

Add storage connection strings for each distinct purpose in the gallery #4568

Add storage connection strings for each distinct purpose in the gallery #4568

Conversation

joelverhagen commented Aug 16, 2017

joelverhagen commented Aug 16, 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

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

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

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

scottbommarito left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment