Stage 1 of Package Reference Compatibility Validation #346

nkolev92 · 2018-02-21T19:00:33Z

I will update the common packages later.

Add an in-proc validator for package reference compatibility, that persist the issues in it's own table, that should allow us to nuke it later.

The validator always "succeeds" because don't want to fail validation, and we don't have the means to rerun a validation right now.

Stage 2 will make it out of proc.

joelverhagen · 2018-02-21T19:24:12Z

src/NuGet.Services.Validation.Orchestrator/settings.json

@@ -40,6 +40,13 @@
      "SubscriptionName": ""
    }
  },
+  "PackageCompatibility": {
+    "ServiceBus": {
+      "ConnectionString": "servicebus string",


nit: empty strings like the other ones

cleaned up.

joelverhagen · 2018-02-21T19:24:29Z

Got a link to the ServerCommon PR as well?

joelverhagen · 2018-02-21T19:25:07Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+                    return validatorStatus.ToValidationResult();
+                }
+
+                // Add the status, so subsequent calls don't try to reevaluate the same thing


nit: re-evaluate

joelverhagen · 2018-02-21T19:25:41Z

src/Validation.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessage.cs

+    {
+        public PackageCompatibilityValidationMessage(string packageId, string packageVersion, Uri nupkgUri, Guid validationId)
+        {
+            PackageId = packageId;


null checks

joelverhagen · 2018-02-21T19:26:07Z

...dation.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessageSerializer.cs

+
+namespace Validation.PackageCompatibility.Core.Messages
+{
+    public class PackageCompatibilityValidationMessageSerializer : IBrokeredMessageSerializer<PackageCompatibilityValidationMessage>


This code is not needed right now, right?

joelverhagen · 2018-02-21T19:26:38Z

src/Validation.PackageCompatibility.Core/PackageCompatibilityService.cs

+            }
+            catch (DbUpdateConcurrencyException e)
+            {
+                _logger.LogWarning("trouble saving changes async - {0}", e.Message);


How could this case happen?

Meant to use DbUpdateException.

Just in case.

I would instead recommend having a big try/catch around your entire validation flow so that it does not effect package publish.

There is one already.
https://github.com/NuGet/NuGet.Jobs/pull/346/files#diff-aa5c3804fc3334cabdf6a923cebce639R49

joelverhagen · 2018-02-21T19:26:45Z

src/Validation.PackageCompatibility.Core/Properties/AssemblyInfo.cs

+[assembly: AssemblyTitle("Validation.PackageCompatibility.Core")]
+[assembly: AssemblyDescription("")]
+[assembly: AssemblyConfiguration("")]
+[assembly: AssemblyCompany("HP Inc.")]


No clue lol.

It's supposed to be autogened.
Didn't find any mentions of HP in the code otherwise.

joelverhagen · 2018-02-21T19:27:01Z

src/Validation.PackageCompatibility.Core/Validation.PackageCompatibility.Core.csproj

+      <Version>4.7.0-preview1-4929</Version>
+    </PackageReference>
+  </ItemGroup>
+  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />


This needs our sign targets -- see other csproj

joelverhagen · 2018-02-21T19:28:35Z

...ing.ExtractAndValidateSignature/Validation.PackageSigning.ExtractAndValidateSignature.csproj

@@ -85,6 +85,11 @@
      <Name>Validation.PackageSigning.Core</Name>
    </ProjectReference>
  </ItemGroup>
+  <ItemGroup>


Is this necessary?

Yes, otherwise the packaging version gets bumped up because of the different version in NuGet.PackageCompatibility.Core project.

Attempting to get the same version throughout the whole solution breaks some signing APIs.

nkolev92 · 2018-02-21T20:43:30Z

🔔
@joelverhagen

joelverhagen · 2018-02-21T20:44:04Z

.vs/config/applicationhost.config

@@ -163,12 +163,20 @@
            </site>
            <site name="Validation.Callback.Vcs" id="2">
                <application path="/" applicationPool="Clr4IntegratedAppPool">
-                    <virtualDirectory path="/" physicalPath="C:\src\NuGet.Jobs\src\Validation.Callback.Vcs" />


This sutff shouldn't change

Why isn't this in ignore?

joelverhagen · 2018-02-21T20:44:18Z

src/NuGet.Services.Validation.Orchestrator/Job.cs

@@ -86,7 +91,7 @@ public override void Init(IDictionary<string, string> jobArgsDictionary)
                return;
            }

-            var runner = GetRequiredService<OrchestrationRunner>();
+            var runner = GetRequiredService<OrchestrationRunner>();            


nit: trailing spaces

joelverhagen · 2018-02-21T20:45:19Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            // Try check the whole thing
+            try
+            {
+                var validatorStatus = await _validatorStateService.GetStatusAsync(request);


nit: move all of the code in the try into a helper method to minimize chance of putting logic outside of the try

joelverhagen · 2018-02-21T20:46:35Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            }
+            catch (Exception e)
+            {
+                _logger.LogWarning($"Validation failed for the validation request {0}.{Environment.NewLine}Exception: {1}", request.ToString(), e.ToString());


LogWarning has an overload that accepts an exception I think.

loic-sharma · 2018-02-21T20:57:59Z

src/NuGet.Services.Validation.Orchestrator/Job.cs

+        private static void ConfigurePackageCompatibilityValidator(ContainerBuilder builder)
+        {
+
+            // Configure the validator state service for the package compatibility validator.


Configure [](start = 15, length = 9)

There's an extra line on 357 and on 376

loic-sharma · 2018-02-21T20:58:15Z

src/NuGet.Services.Validation.Orchestrator/Job.cs

@@ -5,6 +5,8 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Net;
+using System.Net.Http;
+using System.Reflection;


Are these usings necessary?

cleaned up.

loic-sharma · 2018-02-21T21:01:40Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            }
+            catch (Exception e)
+            {
+                _logger.LogWarning($"Validation failed for the validation request {0}.{Environment.NewLine}Exception: {1}", request.ToString(), e.ToString());


Let the logger call ToString. ToString may be expensive and the logger may choose to not emit the log.

Changed that up, after getting the same feedback from @joelverhagen

loic-sharma · 2018-02-21T21:04:52Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+                    );
+
+                // Do validation
+                await Validate(message, CancellationToken.None);


Validate [](start = 22, length = 8)

If Validate throws, won't a validation get "stuck" in Incomplete state? Should Validate happen before the TryAddValidatorStatusAsync?

loic-sharma · 2018-02-21T21:10:00Z

src/Validation.PackageCompatibility.Core/Messages/PackageCompatibilityValidationMessage.cs

+{
+    public class PackageCompatibilityValidationMessage
+    {
+        public PackageCompatibilityValidationMessage(string packageId, string packageVersion, Uri nupkgUri, Guid validationId)


PackageCompatibilityValidationMessage [](start = 15, length = 37)

Is this just used for the PackageCompatibilityValidator.Validate method? A public class seems like over kill, maybe a private class in PackageCompatibilityValidator would be better. Also, are all of these parameters used? It seems like PackageId is not.

In phase 2, this validation will run in it's own proc, and it'll be used there for logging and better visibility, analogous to the way the package id/version are used in the signing validation.

loic-sharma · 2018-02-21T21:14:00Z

src/Validation.PackageCompatibility.Core/PackageCompatibilityService.cs

+            }
+            catch (DbUpdateException e)
+            {
+                _logger.LogWarning("trouble saving changes async - {0}", e.Message);


Message [](start = 75, length = 7)

You should log the full exception instead of e.Message as the real exception may be hidden in a InnerException. If the database fails to save here, won't the PackageCompatibilityValidator think that it's successfully finished SetPackageCompatibilityState and set the validator status to ValidationStatus.Succeeded?

loic-sharma · 2018-02-21T21:27:36Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+    /// <summary>
+    /// Configuration for initializing the <see cref="PackageCompatibilityValidator"/>.
+    /// </summary>
+    public class PackageCompatibilityValidator : IValidator


PackageCompatibilityValidator [](start = 17, length = 29)

Validators that are outside of the NuGet.Services.Validation.Orchestrator project are not supported today. This is a low-priority "bug" in the Orchestrator that we will fix in the future. The type that extends IValidator will need to be moved, the implementation can be kept here.

See this:

NuGet.Jobs/src/NuGet.Services.Validation.Orchestrator/ValidatorProvider.cs

Line 27 in 9722ac9

IEnumerable<Type> candidateTypes = GetCandidateTypes(Assembly.GetCallingAssembly());

That's useful feedback.

This is currently in the Orchestrator project though, just making sure you noticed that.

Oh. I can't read :)

loic-sharma · 2018-02-21T21:38:44Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            }
+
+            // Add the status, so subsequent calls don't try to re-evaluate the same thing
+            await _validatorStateService.TryAddValidatorStatusAsync(request, validatorStatus, ValidationStatus.Incomplete);


TryAddValidatorStatusAsync [](start = 41, length = 26)

TryAddValidatorStatusAsync can return a ValidationStatus that is NOT Incomplete. In such a case, you should immediately return the other status.

This could happen if the message to validate a package is duplicated and one message finishes validating the package AFTER both validators have verified that the validation hasn't started yet.

loic-sharma · 2018-02-21T21:45:40Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            validatorStatus.State = ValidationStatus.Succeeded;
+
+            await _validatorStateService.SaveStatusAsync(validatorStatus);
+            return validatorStatus.ToValidationResult();


ToValidationResult [](start = 35, length = 18)

You should be returning the status according to the SaveStatusAsync as two threads may attempt the SaveStatusAsync call. It seems like today this won't be an issue as wewill only persist Succeeded, but I would rather be resilient to future changes :)

nkolev92 · 2018-02-21T22:30:53Z

🔔
@joelverhagen @loic-sharma
I refactored around the validator code after some discussions.
This way it won't get stuck in a non-terminal state.

joelverhagen · 2018-02-21T22:44:18Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            }
+            catch (Exception e)
+            {
+                _logger.LogWarning(0, e, "Validation failed for the validation request {request}", request);


just request ID

er, Validation ID

joelverhagen · 2018-02-21T22:46:00Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            // Treat every validation as succeeded, as we don't want to block 
+            validatorStatus.State = ValidationStatus.Succeeded;
+
+            var result = await _validatorStateService.SaveStatusAsync(validatorStatus);


If this case fails do we really want to fail push?

Why would this fail?

What state would the validation be stuck in this case?

Refactor it back to the "old" state.

It's not the prettiest but, it makes sure we're not in a weird state even though it doesn't hurt it.

joelverhagen · 2018-02-21T22:48:51Z

Looks good so far. Things to do prior to checking in:

Verify that checking into dev branch is fine with this stuff, /cc @skofman1
Test on DEV environment (I can help with this)

loic-sharma · 2018-02-21T23:58:10Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+                }
+                catch (Exception e)
+                {
+                    _logger.LogWarning(0, e, "Validation failed in the validator for the following request {request}", request.ValidationId);


Nit: for the following request {request} -> for the following validation id {ValidationId}. Same thing with line 82

nkolev92 · 2018-02-22T01:24:27Z

I've added some more changes.

skofman1 · 2018-02-23T01:45:21Z

...NuGet.Services.Validation.Orchestrator/PackageCompatibility/PackageCompatibilityValidator.cs

+            IPackageDownloader packageDownloader,
+            ILogger<PackageCompatibilityValidator> logger)
+        {
+            _validatorStateService = validatorStateService;


null checks

skofman1 · 2018-02-23T01:54:49Z

src/Validation.PackageCompatibility.Core/PackageCompatibilityService.cs

+                _validationContext.PackageCompatibilityIssues.Add(
+                              new PackageCompatibilityIssue()
+                              {
+                                  ClientIssueCode = log.Code.ToString(),


Storing the client issue code as is, can cause issues if you want to have server specific validations that are not available on client side.
For Package Signing Joel has a server side error code that "wraps" all client side error codes, and granular server side error codes for other problems.
I recommend the same approach here.

I discussed this with @joelverhagen.
The idea for the Package Reference validations is that they'll always be client controlled.
The APIs used are the exact ones as the one in the client code.

Also not reusing that table allows us more freedom to evolve the design moving forward.

skofman1 · 2018-02-23T01:57:34Z

src/NuGet.Services.Validation.Orchestrator/settings.json

@@ -71,6 +78,7 @@
    "AnnouncementsUrl": "https://github.com/NuGet/Announcements/issues",
    "TwitterUrl": "https://twitter.com/nuget"
  },
+  "PackageDownloadTimeout": "10:00",


if this is specific to PackageCompatibility validator, please move the config there.

This is only here, because the validator is currently running in-proc.
Once the validator is in it's own process it will get moved out of here.

skofman1 · 2018-02-23T01:58:21Z

I'm ok with checking this in to DEV branch, however, it shouldn't run as is because downloading the package in the Orchestrator might impact perf.

More progress on NuGet/NuGetGallery#6317.

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

nkolev92 · 2020-04-29T22:00:24Z

Unfortunately this work didn't get merged a few years back and I don't see this getting put high on the priority list soon again.
Closing this PR.

joelverhagen · 2020-04-29T22:16:41Z

😭

More progress on NuGet/NuGetGallery#6317.

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

nkolev92 requested a review from joelverhagen February 21, 2018 19:00

joelverhagen reviewed Feb 21, 2018

View reviewed changes

loic-sharma reviewed Feb 21, 2018

View reviewed changes

loic-sharma requested a review from agr February 21, 2018 21:46

nkolev92 changed the title ~~WIP - Stage 1 of Package Reference Compatibility Validation~~ Stage 1 of Package Reference Compatibility Validation Feb 21, 2018

joelverhagen reviewed Feb 21, 2018

View reviewed changes

nkolev92 mentioned this pull request Feb 21, 2018

Add package compatibility issue schema NuGet/ServerCommon#138

Merged

nkolev92 force-pushed the dev-nkolev92-prval branch from a1d1008 to a7bd6d9 Compare February 21, 2018 23:42

loic-sharma reviewed Feb 21, 2018

View reviewed changes

nkolev92 force-pushed the dev-nkolev92-prval branch from dd404d4 to f25b0a4 Compare February 22, 2018 01:23

skofman1 reviewed Feb 23, 2018

View reviewed changes

nkolev92 added 4 commits March 7, 2018 23:00

add package compatibility skeleton

ee229c9

Cleanup redundant code

a4990b5

Remove changes in dependencies for local development

a415a12

Refactor the validator so it doesn't get stuck in a weird state

e7931a0

nkolev92 force-pushed the dev-nkolev92-prval branch from f25b0a4 to 34c4c46 Compare March 8, 2018 07:17

cleanup the validator to amtch the ew API

1143816

nkolev92 force-pushed the dev-nkolev92-prval branch from f65f784 to 1143816 Compare March 8, 2018 07:19

joelverhagen pushed a commit that referenced this pull request Sep 27, 2019

Test: improve and add registration tests (#346)

79e85df

More progress on NuGet/NuGetGallery#6317.

nkolev92 closed this Apr 29, 2020

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Test: improve and add registration tests (#346)

ddfd546

More progress on NuGet/NuGetGallery#6317.

Stage 1 of Package Reference Compatibility Validation #346

Stage 1 of Package Reference Compatibility Validation #346

Conversation

nkolev92 commented Feb 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen commented Feb 21, 2018

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

nkolev92 Feb 21, 2018 • edited

Choose a reason for hiding this comment

nkolev92 commented Feb 21, 2018

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

loic-sharma Feb 21, 2018 • edited

Choose a reason for hiding this comment

loic-sharma Feb 21, 2018 • edited

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

loic-sharma Feb 21, 2018 • edited

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

nkolev92 commented Feb 21, 2018

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

joelverhagen commented Feb 21, 2018

loic-sharma Feb 21, 2018 • edited

Choose a reason for hiding this comment

nkolev92 commented Feb 22, 2018

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

skofman1 commented Feb 23, 2018

nkolev92 commented Apr 29, 2020

joelverhagen commented Apr 29, 2020

nkolev92 commented Feb 21, 2018 •

edited

nkolev92 Feb 21, 2018 •

edited

loic-sharma Feb 21, 2018 •

edited

loic-sharma Feb 21, 2018 •

edited

loic-sharma Feb 21, 2018 •

edited

loic-sharma Feb 21, 2018 •

edited