Add caching to package details page vulnerabilities query #8580

drewgillies · 2021-05-13T08:52:46Z

Addresses: #8543

Note that this change is against dev and will need to be rebased to main if we wish to deploy it as a hotfix.

This introduces the PackageVulnerabilitiesCacheService which is modelled on TypoSquatting caching. I've moved the query logic into the cache service, using an arbitrary 30-minute caching length per query result.

joelverhagen · 2021-05-13T15:38:47Z

Could we do caching at a higher level and use the same caching method used elsewhere?
HttpContext.Cache
e.g.

NuGetGallery/src/NuGetGallery/Controllers/PackagesController.cs

Lines 1025 to 1055 in f248171

    
           private PackageDependents GetPackageDependents(string id) 
        
           { 
        
               PackageDependents dependents; 
        
               var cacheDependentsCacheKey = "CacheDependents_" + id.ToLowerInvariant(); 
        
               var cacheDependents = HttpContext.Cache.Get(cacheDependentsCacheKey); 
        
               // Cache doesn't contain PackageDependents so PackageDependents gets put in the cache 
        
               if (cacheDependents == null) 
        
               { 
        
                   dependents = _packageService.GetPackageDependents(id); 
        
                   // note: this is a per instance cache 
        
                   HttpContext.Cache.Add( 
        
                       cacheDependentsCacheKey, 
        
                       dependents, 
        
                       null, 
        
                       DateTime.UtcNow.AddSeconds(_contentObjectService.CacheConfiguration.PackageDependentsCacheTimeInSeconds), 
        
                       Cache.NoSlidingExpiration, 
        
                       CacheItemPriority.Default, null); 
        
               } 
        
               // Cache contains PackageDependents 
        
               else 
        
               { 
        
                   dependents = (PackageDependents)cacheDependents; 
        
                   // TODO Make cache time configurable / slidy 
        
                   // https://github.com/NuGet/NuGetGallery/issues/4718 
        
               } 
        
               return dependents; 
        
           }

This has expiration logic built in. There are other example usages of this API.

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

drewgillies · 2021-05-13T22:24:06Z

Could we do caching at a higher level and use the same caching method used elsewhere?
HttpContext.Cache

@joelverhagen is this preferable to the typosquatting approach (i.e. https://github.com/NuGet/NuGetGallery/blob/main/src/NuGetGallery/Services/TyposquattingCheckListCacheService.cs)? What are the merits of each?

joelverhagen · 2021-05-13T22:36:38Z

HttpContext.Cache: automatically handles concurrency and cache invalidation time. Smaller code. Perhaps more consistent with other caching scenarios, but it's hard to discover all instances of the alternative.
TyposquattingCheckListCacheService: not dependent on ASP.NET/HTTP types. Probably easier to UT.

joelverhagen · 2021-05-13T22:37:26Z

What do you think about those merits? Perhaps you can take a look at pros/cons as well? I think it's possible that HttpContext.Cache automatically evicts the cache in the event of memory pressure but I'm not 100% sure.

zhhyu · 2021-05-13T23:10:33Z

I think the use case of "Typosquatting" cache may be different compared with this. The "Typosquatting" cache only contains one entry, which is a list of package names and only used when a package is pushed. For vulnerabilities, it is a very similar scenario as "Used By" info. So I agree that maybe using the highly-optimized caching component provided by the framework is better.

skofman1 · 2021-05-13T23:34:42Z

Another thought: we have a perf problem with Vulnerabilities when it comes to the 'Manage Packages' page as well (#8361). This might help to fix both.
Btw @drewgillies , should we re-open #8361 ? It's currently disabled due to perf, right?

drewgillies · 2021-05-13T23:40:25Z

@skofman1 yes, that's right--I closed because we needed an entirely new approach. I've reopened it just now so we can investigate this idea. I'd be (pleasantly) surprised if caching is going to get us over the line on that one, as my understanding is that any regression at all is unacceptable given how poor the page's perf is already. And introducing a new table (let alone 3) regresses perf whichever way we slice it.
@joelverhagen I like the notion of http caching on the strength of what you're saying--UTs will still cover the logic, and if I can make the change more elegantly in code, I'm up for that.

skofman1 · 2021-05-13T23:55:08Z

I made the comment before going over the code. My initial thinking was that we can cache in memory all the vulnerability information, and load it from DB on start-up. What do you think?

drewgillies · 2021-05-14T00:22:00Z

That's an interesting idea, @skofman1. I'll look into it.

drewgillies · 2021-05-17T08:33:10Z

Interested in thoughts on this approach @joelverhagen @skofman1 @zhhyu --based on @skofman's idea, I'm loading the cache on startup, and have chosen an arbitrary 1 day for cache invalidation. This approach could then be extended to the manage packages page if we wish to experiment. More testing required but there is UT coverage here and the cache loads nicely. I went for the complex linq statement to get the best perf out of EF, so commented heavily.

joelverhagen · 2021-05-17T16:52:18Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+
+        private bool ShouldCachedValueBeUpdated(string id) => !vulnerabilitiesByIdCache.ContainsKey(id) ||
+                                                              vulnerabilitiesByIdCache[id].cachedAt
+                                                                  .AddMinutes(CachingLimitMinutes) < DateTime.Now;


nit: DateTimeOffset.UtcNow instead of DateTime.Now for clarity

joelverhagen · 2021-05-17T16:54:06Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+            = new Dictionary<string, (DateTime, Dictionary<int, IReadOnlyList<PackageVulnerability>>)>();
+
+        private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService;
+        public PackageVulnerabilitiesCacheService(IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService)


I'm wondering whether this dependency even makes much sense. The two new methods introduced on IPackageVulnerabilitiesManagementService are very small so it might be clearer to just depend on IEntitiesContext here.

The separation of responsibilities was initially related to DI scopes. PackageVulnerabilitiesManagementService is InstancePerLifetimeScope, and this is a SingleInstance (in order for caching to work), initially with the management service passed to caching service methods (rather than having it stored in a field). I expect I'll return to this model if I'm unable to run the Initialize method from the constructor, which in that sense I like better.

The reason we have a caching service class which is separate from PackageVulnerabilitiesService (rather than just adding caching capabilities to it) is the same --PackageVulnerabilitiesService is InstancePerLifetimeScope.

joelverhagen · 2021-05-17T16:54:42Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+        public PackageVulnerabilitiesCacheService(IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService)
+        {
+            _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService;
+            Initialize();


We should not do SQL in the constructor

joelverhagen · 2021-05-17T16:57:20Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+            }
+
+            return vulnerabilitiesByIdCache[id].vulnerabilitiesById.Any()
+                ? new ReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>>(vulnerabilitiesByIdCache[id].vulnerabilitiesById)


nit: you can just return a Dictionary<TKey, TValue> which itself implements IReadOnlyDictionary<TKey, TValue>. No need to construct a ReadOnlyDictionary.

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

joelverhagen · 2021-05-17T17:06:10Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+                throw new ArgumentException("Must have a value.", nameof(id));
+            }
+
+            if (ShouldCachedValueBeUpdated(id))


This dictionary read could operate on the cache dictionary while it is being modified on line 49. Better to use a ConcurrentDictionary. See https://stackoverflow.com/a/24586924.

joelverhagen · 2021-05-17T17:06:38Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+                            .SelectMany(x => x.Packages.Select(p => new {PackageKey = p.Key, x.Vulnerability}))
+                            .GroupBy(pv => pv.PackageKey, pv => pv.Vulnerability)
+                            .ToDictionary(pv => pv.Key,
+                                pv => pv.ToList().AsReadOnly() as IReadOnlyList<PackageVulnerability>);


You don't need AsReadOnly. You can just cast to IReadOnlyList.

drewgillies · 2021-05-17T22:15:10Z

background refresh

@joelverhagen I love this idea. That's the missing piece. Thanks for all of your granular feedback above as well.

drewgillies · 2021-05-18T07:00:51Z

@joelverhagen @skofman1 @zhhyu I've implemented the background refresh on the cache, and I'll have it report load time telemetry like the downloads refresh does--it would be good to track this over time as the cache grows in size.

skofman1 · 2021-05-18T22:09:20Z

src/NuGetGallery/App_Start/AppActivator.cs

+                HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache());
+                if (configuration.StorageType == StorageType.AzureStorage)
+                {
+                    jobs.Add(new PackageVulnerabilitiesCacheRefreshJob(TimeSpan.FromMinutes(30),


30

what's the frequency of the GitHub job? Why 30 minutes?

This was arbitrary - it could easily be a day and work quite well. I thought that if we were to extend this cache's use to the manage packages page, a customer would want to see more "live" updates, so 30 minutes seemed like the happy medium.

if the GitHub job runs once a day to get updates, there's no point in having this cache updated every 30 mins. If the job runs frequently (say every 10 minutes), 30 minutes definetly makes sense.

skofman1 · 2021-05-18T22:14:52Z

src/NuGetGallery.Services/Telemetry/TelemetryService.cs

@@ -91,6 +91,7 @@ public class Events
            public const string ABTestEvaluated = "ABTestEvaluated";
            public const string PackagePushDisconnect = "PackagePushDisconnect";
            public const string SymbolPackagePushDisconnect = "SymbolPackagePushDisconnect";
+            public const string VulnerabilitiesCacheRefreshDuration = "VulnerabilitiesCacheRefreshDuration";


add units to the metric name for debuggability. i.e. VulnerabilitiesCacheRefreshDurationMs

Sure thing--I was modelling it off TrackDownloadJsonRefreshDuration - I can change that too if you like. Also, do you want the string value changed or just the name of the const?

Add to both. When you look at telemetry it will be super useful to know the units without going to the code. Regarding TrackDownloadJsonRefreshDuration , it will be nice to change that, assuming no one is depending on the event name for dashboards/monitoring.

Will do! I might hang off on ...DownloadJson... then, but yes, will change mine.

skofman1 · 2021-05-18T22:19:26Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+        private readonly ITelemetryService _telemetryService;
+
+        public PackageVulnerabilitiesCacheService(
+            IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService,


packageVulnerabilitiesManagementService

if I understand correctly, the IPackageVulnerabilitiesManagementService will be created only once, which is not good because it has a dependency on EntityContext (DB connection), meaning we need a new one with every request.

The lifecycle pieces of this are the real trick (and the reason the cache service exists at all)--I may have the Job pass the management service each time to get around this.

zhhyu · 2021-05-19T05:13:48Z

src/NuGetGallery/App_Start/AppActivator.cs

+            if (packageVulnerabilitiesCacheService != null)
+            {
+                // Perform initial refresh + schedule new refreshes every 30 minutes
+                HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache());


I don't know too much about what happens for this behind the scene. This seems running as a background thread, and the query may take some time to run. Will it have a race condition between the application and the query? For example, the application starts accepting requests while the query has not finished running. That may not mean customer impacts because we have functional/e2e tests but if we have some tests for vulnerabilities, the test may fail because the result has not been loaded yet.

If you look in the RefreshCache you'll see that this is catered for in the _isRefreshing sync'd field. The first refresh is run in AppActivator so we should have data available (and I don't believe any E2E tests cover vulnerabilities data).

zhhyu · 2021-05-19T05:23:08Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+
+                    stopwatch.Stop();
+
+                    _telemetryService.TrackVulnerabilitiesCacheRefreshDuration(stopwatch.ElapsedMilliseconds);


Besides logging the duration, I am thinking whether it will be better if we also log the status of the query, for example, possible exceptions thrown from the DB query (Timeout, etc..).

I'll trace exceptions in the query code. Good idea.

zhhyu · 2021-05-19T05:32:25Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+
+                    // We need to build a dictionary of dictionaries. Breaking it down:
+                    // - this give us a list of all vulnerable package version ranges
+                    _vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges()


This looks like a heavy query for DB. I wonder it may cause glitches for DB performance. We may need some stress tests to understand the behavior.

Besides, my understanding is that each time we reload everything from DB again. I am not sure whether we can only load the difference (for example, depend on the timestamp somehow) because it sounds like most of existing vulnerabilities should not need to be updated with a high frequency, but the effort may not be worth if the performance is ok.

I think it would be good to track telemetry on this and see how it performs generally, before introducing performance tweaks. It's only heavy in post-query transformation--the query itself loads records from the VulnerablePackageVersionRange table and related PackageVulnerabilities records--this should be relatively lightweight. But we should watch telemetry. If it turns out we need to find ways to speed up load, we could start looking at timestamping vulnerable range records, but of course, the table and all of the records would need to be loaded in order to test for changes (but it would reduce PackageVulnerabilities reads)--I think the wins would be small there. Let's investigate down the road if we need to.

Another option for reducing DTUs is, of course, increasing the refresh interval from 30 minutes to something far longer.

zhhyu · 2021-05-19T05:43:41Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+    {
+        private IDictionary<string,
+            Dictionary<int, IReadOnlyList<PackageVulnerability>>> _vulnerabilitiesByIdCache
+            = new ConcurrentDictionary<string, Dictionary<int, IReadOnlyList<PackageVulnerability>>>();


For discussions: I can see that the only write action now is to update the reference to another new dictionary when we refresh the cache. Do we still need to use "ConcurrenctDictionary"? We don't update the granularity such as some entries in the dictionary.

Good call--I'm reverting this to a Dictionary.

drewgillies · 2021-05-19T06:43:34Z

@skofman1 I've worked with @joelverhagen and @agr to land on a more elegant solution here--we're caching a scope factory in the PackageVulnerabilitiesCacheService which will spin up an entirely new scope every time we refresh, and generate a new EntitiesContext instance each time we do. I believe I've addressed all feedback now (including @zhhyu's). Interested in how close you feel we are to an acceptable design. I'll be running some more tests on it before merging.

skofman1 · 2021-05-19T16:07:34Z

src/NuGetGallery/App_Start/AppActivator.cs

+            var packageVulnerabilitiesCacheService =
+                    DependencyResolver.Current.GetService<IPackageVulnerabilitiesCacheService>() as
+                        PackageVulnerabilitiesCacheService;
+            if (packageVulnerabilitiesCacheService != null)


packageVulnerabilitiesCacheService

why would packageVulnerabilitiesCacheService ever be null? If it's null it means we have a bug and forgot to register it. In that case, it would be better to fail fast then to get into a state where the cache isn't refreshed. I suggest to drop the null check.

Why not make the necessary methods just available on the interface? I don't see why any cast is necessary.

Yep--good call. The same thing is being done for cloudDownloadCountService above, but it is redundant. I'll remove that.

skofman1 · 2021-05-19T16:09:37Z

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs

+
+        public PackageVulnerabilitiesCacheService(ITelemetryService telemetryService)
+        {
+            _telemetryService = telemetryService;


telemetryService

null check

skofman1

Some comments. Otherwise looks good :)

joelverhagen · 2021-05-19T22:12:45Z

src/NuGetGallery.Services/Telemetry/ITelemetryService.cs

+        /// Track how long it takes to populate the vulnerabilities cache
+        /// </summary>
+        /// <param name="milliseconds">Refresh duration for vulnerabilities cache</param>
+        void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds);


nit: pass TimeSpan here, don't mention "ms". The implementation below can make a "ms" specific metric emitted to AI but the interface can be cleaner than that. It's not a big deal though.

I'm so confused :) #8580 (comment)

TimeSpan makes sense but is inconsistent with other telemetry methods. I'd like us to retrofit code if we pivot on practice, so other devs can follow the lead of what's there more efficiently. I'm happy to do this here but it may be good to consider this going forward.

Thinking further, TimeSpan here would mean removing Ms from the method name, which would conflict with @skofman1's comment as well. I think I'll leave it as-is for now? When we arrive at consensus on how we want our telemetry to function for duration tracking, perhaps we can go through and change them all as a separate change.

Just did a quick visual scan: TrackDownloadJsonRefreshDuration is the only outlier. Perhaps I could change both of them in this change?

(I would still leave Ms off the method name as it's no longer ms, and would make them both use TimeSpan)

joelverhagen · 2021-05-19T22:13:23Z

src/NuGetGallery/App_Start/AppActivator.cs

@@ -265,12 +265,29 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration)

            if (configuration.StorageType == StorageType.AzureStorage)
            {
-                var cloudDownloadCountService = DependencyResolver.Current.GetService<IDownloadCountService>() as CloudDownloadCountService;
+                var cloudDownloadCountService =


nit: please minimize changes to unrelated lines.

Wasn't sure of our policy on cleanup. Thanks.

joelverhagen · 2021-05-19T22:14:52Z

src/NuGetGallery/App_Start/AppActivator.cs

+                // Perform initial refresh + schedule new refreshes every 30 minutes
+                var serviceScopeFactory = DependencyResolver.Current.GetService<IServiceScopeFactory>();
+                HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache(serviceScopeFactory));
+                if (configuration.StorageType == StorageType.AzureStorage)


Why is this predicated on AzureStorage?

It didn't seem to make sense to have this running on local environments. If you think it should be all cases, I'm happy to remove it.

joelverhagen · 2021-05-19T22:15:31Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -460,6 +460,11 @@ protected override void Load(ContainerBuilder builder)
                .As<IPackageVulnerabilitiesManagementService>()
                .InstancePerLifetimeScope();

+            builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve<ITelemetryService>()))


You can just registry the type, i.e. RegistryType. You don't need explicit ctor here right?

Yes, I'm sure there was a reason I thought this was appropriate when it was more complex, but on reflection even then I was mistaken. Will clean up.

joelverhagen · 2021-05-20T01:08:17Z

src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs

+        public PackageVulnerabilitiesCacheRefreshJob(TimeSpan interval, 
+            PackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService,
+            IServiceScopeFactory serviceScopeFactory)
+            : base("", interval)


please use explicit parameter name for ambiguous values like this empty string

joelverhagen · 2021-05-20T01:09:00Z

src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs

+
+        public override Task Execute()
+        {
+            return new Task(() => _packageVulnerabilitiesCacheService.RefreshCache(_serviceScopeFactory));


Does this need to be a new task? Who calls Execute? Is it already in a background task? If so, you can just synchronously execute RefreshCache and return Task.CompletedTask.

drewgillies requested a review from a team as a code owner May 13, 2021 08:52

zhhyu reviewed May 13, 2021

View reviewed changes

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs Show resolved Hide resolved

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 2ab3b17 to 28f6555 Compare May 17, 2021 01:16

Add caching to package details page vulnerabilities query

c0a6f85

drewgillies force-pushed the dg-pvacache-packagedetailspage branch 4 times, most recently from 842628b to 2f59b6a Compare May 17, 2021 08:29

Addressed feedback - loading cache on startup

2e4e873

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 2f59b6a to 2e4e873 Compare May 17, 2021 08:35

joelverhagen reviewed May 17, 2021

View reviewed changes

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs Outdated Show resolved Hide resolved

joelverhagen reviewed May 17, 2021

View reviewed changes

src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs Show resolved Hide resolved

joelverhagen reviewed May 17, 2021

View reviewed changes

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 6ac49fa to 6f3cce4 Compare May 18, 2021 01:18

Addressed feedback

a4032d1

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from f906559 to a4032d1 Compare May 18, 2021 04:42

skofman1 reviewed May 18, 2021

View reviewed changes

zhhyu reviewed May 19, 2021

View reviewed changes

Addressed feedback

7064b80

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 83f0372 to 7064b80 Compare May 19, 2021 06:39

skofman1 reviewed May 19, 2021

View reviewed changes

skofman1 approved these changes May 19, 2021

View reviewed changes

joelverhagen reviewed May 19, 2021

View reviewed changes

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 4452155 to 4c4f43d Compare May 20, 2021 00:21

joelverhagen reviewed May 20, 2021

View reviewed changes

joelverhagen approved these changes May 20, 2021

View reviewed changes

Addressed feedback

5a92c9f

drewgillies force-pushed the dg-pvacache-packagedetailspage branch from 4c4f43d to 5a92c9f Compare May 20, 2021 01:34

drewgillies merged commit 5c5e8f9 into dev May 20, 2021

drewgillies deleted the dg-pvacache-packagedetailspage branch May 20, 2021 11:23

zhhyu mentioned this pull request May 24, 2021

[2021.05.24] Deployment #8600

Closed

23 tasks

drewgillies mentioned this pull request Jun 7, 2021

Utilize vulnerabilities caching service for in manage packages page #8620

Merged


		stopwatch.Stop();

		_telemetryService.TrackVulnerabilitiesCacheRefreshDuration(stopwatch.ElapsedMilliseconds);

Add caching to package details page vulnerabilities query #8580

Add caching to package details page vulnerabilities query #8580

Conversation

drewgillies commented May 13, 2021

joelverhagen commented May 13, 2021

drewgillies commented May 13, 2021

joelverhagen commented May 13, 2021

joelverhagen commented May 13, 2021

zhhyu commented May 13, 2021

skofman1 commented May 13, 2021

drewgillies commented May 13, 2021

skofman1 commented May 13, 2021

drewgillies commented May 14, 2021

drewgillies commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewgillies May 17, 2021 • 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

drewgillies commented May 17, 2021 • edited Loading

drewgillies commented May 18, 2021

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

zhhyu May 19, 2021 • edited Loading

Choose a reason for hiding this comment

drewgillies May 19, 2021 • 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

drewgillies commented May 19, 2021

skofman1 May 19, 2021 • 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

skofman1 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

Choose a reason for hiding this comment

drewgillies May 17, 2021 •

edited

Loading

drewgillies commented May 17, 2021 •

edited

Loading

zhhyu May 19, 2021 •

edited

Loading

drewgillies May 19, 2021 •

edited

Loading

skofman1 May 19, 2021 •

edited

Loading