From eebae93bd1d751a6f08ad27acc78afa143383417 Mon Sep 17 00:00:00 2001 From: William Baker Date: Thu, 10 Oct 2019 10:41:00 -0700 Subject: [PATCH 1/4] maintenance: move maintenance task management to service Previously, Scalar maintenance tasks were performed in the Scalar.Mount process. As a step toward completely removing the mount process (and its associated verbs) the changes in this commit move the scheduling and calling of maintenance tasks into the service. The service runs the maintenance tasks by way of the 'scalar maintenance' verb, and it does so for all of the that the current user has registered. With this change the 'mount' and 'unmount' verbs serve only to register and unregister the repo with the service. Deprecation of these verbs, as well as the mount process itself, will be performed in a subsequent patch series. --- .../Maintenance/GitMaintenanceQueue.cs | 120 ------------ .../Maintenance/GitMaintenanceScheduler.cs | 91 --------- Scalar.Common/ScalarConstants.cs | 12 ++ Scalar.Mount/InProcessMount.cs | 15 +- Scalar.Service/IRepoMounter.cs | 7 - Scalar.Service/IRepoRegistry.cs | 2 +- Scalar.Service/IScalarVerbRunner.cs | 7 + Scalar.Service/IServiceTask.cs | 8 + Scalar.Service/MaintenanceTaskScheduler.cs | 178 ++++++++++++++++++ Scalar.Service/Program.Mac.cs | 3 +- Scalar.Service/RepoRegistry.cs | 101 +++++----- Scalar.Service/Scalar.Service.Mac.csproj | 7 +- Scalar.Service/Scalar.Service.Windows.csproj | 7 +- Scalar.Service/ScalarMountProcess.Mac.cs | 87 --------- Scalar.Service/ScalarMountProcess.Windows.cs | 46 ----- Scalar.Service/ScalarService.Mac.cs | 68 ++++--- Scalar.Service/ScalarService.Windows.cs | 15 +- Scalar.Service/ScalarVerbRunner.Mac.cs | 76 ++++++++ Scalar.Service/ScalarVerbRunner.Windows.cs | 41 ++++ Scalar.Service/ServiceTaskQueue.cs | 103 ++++++++++ .../Service/Mac/MacServiceTests.cs | 58 ++---- Scalar.UnitTests/Service/RepoRegistryTests.cs | 69 +------ .../ServiceTaskQueueTests.cs} | 88 ++++----- Scalar/CommandLine/CloneVerb.cs | 2 +- Scalar/CommandLine/MaintenanceVerb.cs | 29 ++- 25 files changed, 604 insertions(+), 636 deletions(-) delete mode 100644 Scalar.Common/Maintenance/GitMaintenanceQueue.cs delete mode 100644 Scalar.Common/Maintenance/GitMaintenanceScheduler.cs delete mode 100644 Scalar.Service/IRepoMounter.cs create mode 100644 Scalar.Service/IScalarVerbRunner.cs create mode 100644 Scalar.Service/IServiceTask.cs create mode 100644 Scalar.Service/MaintenanceTaskScheduler.cs delete mode 100644 Scalar.Service/ScalarMountProcess.Mac.cs delete mode 100644 Scalar.Service/ScalarMountProcess.Windows.cs create mode 100644 Scalar.Service/ScalarVerbRunner.Mac.cs create mode 100644 Scalar.Service/ScalarVerbRunner.Windows.cs create mode 100644 Scalar.Service/ServiceTaskQueue.cs rename Scalar.UnitTests/{Maintenance/GitMaintenanceQueueTests.cs => Service/ServiceTaskQueueTests.cs} (56%) diff --git a/Scalar.Common/Maintenance/GitMaintenanceQueue.cs b/Scalar.Common/Maintenance/GitMaintenanceQueue.cs deleted file mode 100644 index c50ae1ccf7..0000000000 --- a/Scalar.Common/Maintenance/GitMaintenanceQueue.cs +++ /dev/null @@ -1,120 +0,0 @@ -using Scalar.Common.Tracing; -using System; -using System.Collections.Concurrent; -using System.IO; -using System.Threading; - -namespace Scalar.Common.Maintenance -{ - public class GitMaintenanceQueue - { - private readonly object queueLock = new object(); - private ScalarContext context; - private BlockingCollection queue = new BlockingCollection(); - private GitMaintenanceStep currentStep; - - public GitMaintenanceQueue(ScalarContext context) - { - this.context = context; - Thread worker = new Thread(() => this.RunQueue()); - worker.Name = "MaintenanceWorker"; - worker.IsBackground = true; - worker.Start(); - } - - public bool TryEnqueue(GitMaintenanceStep step) - { - try - { - lock (this.queueLock) - { - if (this.queue == null) - { - return false; - } - - this.queue.Add(step); - return true; - } - } - catch (InvalidOperationException) - { - // We called queue.CompleteAdding() - } - - return false; - } - - public void Stop() - { - lock (this.queueLock) - { - this.queue?.CompleteAdding(); - } - - this.currentStep?.Stop(); - } - - /// - /// This method is public for test purposes only. - /// - public bool EnlistmentRootReady() - { - return GitMaintenanceStep.EnlistmentRootReady(this.context); - } - - private void RunQueue() - { - while (true) - { - // We cannot take the lock here, as TryTake is blocking. - // However, this is the place to set 'this.queue' to null. - if (!this.queue.TryTake(out this.currentStep, Timeout.Infinite) - || this.queue.IsAddingCompleted) - { - lock (this.queueLock) - { - // A stop was requested - this.queue?.Dispose(); - this.queue = null; - return; - } - } - - if (this.EnlistmentRootReady()) - { - try - { - this.currentStep.Execute(); - } - catch (Exception e) - { - this.LogErrorAndExit( - area: nameof(GitMaintenanceQueue), - methodName: nameof(this.RunQueue), - exception: e); - } - } - } - } - - private void LogError(string area, string methodName, Exception exception) - { - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", area); - metadata.Add("Method", methodName); - metadata.Add("ExceptionMessage", exception.Message); - metadata.Add("StackTrace", exception.StackTrace); - this.context.Tracer.RelatedError( - metadata: metadata, - message: area + ": Unexpected Exception while running maintenance steps (fatal): " + exception.Message, - keywords: Keywords.Telemetry); - } - - private void LogErrorAndExit(string area, string methodName, Exception exception) - { - this.LogError(area, methodName, exception); - Environment.Exit((int)ReturnCode.GenericError); - } - } -} diff --git a/Scalar.Common/Maintenance/GitMaintenanceScheduler.cs b/Scalar.Common/Maintenance/GitMaintenanceScheduler.cs deleted file mode 100644 index 8b74883599..0000000000 --- a/Scalar.Common/Maintenance/GitMaintenanceScheduler.cs +++ /dev/null @@ -1,91 +0,0 @@ -using Scalar.Common.Git; -using System; -using System.Collections.Generic; -using System.Threading; - -namespace Scalar.Common.Maintenance -{ - public class GitMaintenanceScheduler : IDisposable - { - private readonly TimeSpan looseObjectsDueTime = TimeSpan.FromMinutes(5); - private readonly TimeSpan looseObjectsPeriod = TimeSpan.FromHours(6); - - private readonly TimeSpan packfileDueTime = TimeSpan.FromMinutes(30); - private readonly TimeSpan packfilePeriod = TimeSpan.FromHours(12); - - private readonly TimeSpan commitGraphDueTime = TimeSpan.FromMinutes(15); - private readonly TimeSpan commitGraphPeriod = TimeSpan.FromHours(1); - - private readonly TimeSpan defaultFetchCommitsAndTreesPeriod = TimeSpan.FromMinutes(15); - - private List stepTimers; - private ScalarContext context; - private GitObjects gitObjects; - private GitMaintenanceQueue queue; - - public GitMaintenanceScheduler(ScalarContext context, GitObjects gitObjects) - { - this.context = context; - this.gitObjects = gitObjects; - this.stepTimers = new List(); - this.queue = new GitMaintenanceQueue(context); - - this.ScheduleRecurringSteps(); - } - - public void EnqueueOneTimeStep(GitMaintenanceStep step) - { - this.queue.TryEnqueue(step); - } - - public void Dispose() - { - this.queue.Stop(); - - foreach (Timer timer in this.stepTimers) - { - timer?.Dispose(); - } - - this.stepTimers = null; - } - - private void ScheduleRecurringSteps() - { - if (this.context.Unattended) - { - return; - } - - TimeSpan actualFetchCommitsAndTreesPeriod = this.defaultFetchCommitsAndTreesPeriod; - if (!this.gitObjects.IsUsingCacheServer()) - { - actualFetchCommitsAndTreesPeriod = TimeSpan.FromHours(24); - } - - this.stepTimers.Add(new Timer( - (state) => this.queue.TryEnqueue(new FetchCommitsAndTreesStep(this.context, this.gitObjects)), - state: null, - dueTime: actualFetchCommitsAndTreesPeriod, - period: actualFetchCommitsAndTreesPeriod)); - - this.stepTimers.Add(new Timer( - (state) => this.queue.TryEnqueue(new LooseObjectsStep(this.context)), - state: null, - dueTime: this.looseObjectsDueTime, - period: this.looseObjectsPeriod)); - - this.stepTimers.Add(new Timer( - (state) => this.queue.TryEnqueue(new PackfileMaintenanceStep(this.context)), - state: null, - dueTime: this.packfileDueTime, - period: this.packfilePeriod)); - - this.stepTimers.Add(new Timer( - (state) => this.queue.TryEnqueue(new CommitGraphStep(this.context)), - state: null, - dueTime: this.commitGraphDueTime, - period: this.commitGraphPeriod)); - } - } -} diff --git a/Scalar.Common/ScalarConstants.cs b/Scalar.Common/ScalarConstants.cs index 89afcff5e0..8972068a10 100644 --- a/Scalar.Common/ScalarConstants.cs +++ b/Scalar.Common/ScalarConstants.cs @@ -204,6 +204,18 @@ public static class Unmount { public const string SkipLock = "skip-wait-for-lock"; } + + public static class Maintenance + { + public const string Task = "task"; + + public const string FetchCommitsAndTreesTaskName = "fetch-commits-and-trees"; + public const string LooseObjectsTaskName = "loose-objects"; + public const string PackFilesTaskName = "pack-files"; + public const string CommitGraphTaskName = "commit-graph"; + + public const string BatchSizeOptionName = "batch-size"; + } } public static class UpgradeVerbMessages diff --git a/Scalar.Mount/InProcessMount.cs b/Scalar.Mount/InProcessMount.cs index 35fe979ced..81799b087a 100644 --- a/Scalar.Mount/InProcessMount.cs +++ b/Scalar.Mount/InProcessMount.cs @@ -17,7 +17,6 @@ public class InProcessMount private ScalarEnlistment enlistment; private ITracer tracer; - private GitMaintenanceScheduler maintenanceScheduler; private CacheServerInfo cacheServer; private RetryConfig retryConfig; @@ -181,7 +180,7 @@ private T CreateOrReportAndExit(Func factory, string reportMessage) } } - private void HandleRequest(ITracer tracer, string request, NamedPipeServer.Connection connection) + private void HandleRequest(ITracer requestTracer, string request, NamedPipeServer.Connection connection) { NamedPipeMessages.Message message = NamedPipeMessages.Message.FromString(request); @@ -258,7 +257,6 @@ private void HandleUnmountRequest(NamedPipeServer.Connection connection) this.currentState = MountState.Unmounting; connection.TrySendResponse(NamedPipeMessages.Unmount.Acknowledged); - this.UnmountAndStopWorkingDirectoryCallbacks(); connection.TrySendResponse(NamedPipeMessages.Unmount.Completed); this.unmountEvent.Set(); @@ -286,8 +284,6 @@ private void MountAndStartWorkingDirectoryCallbacks(CacheServerInfo cache) GitObjectsHttpRequestor objectRequestor = new GitObjectsHttpRequestor(this.context.Tracer, this.context.Enlistment, cache, this.retryConfig); this.gitObjects = new ScalarGitObjects(this.context, objectRequestor); - this.maintenanceScheduler = this.CreateOrReportAndExit(() => new GitMaintenanceScheduler(this.context, this.gitObjects), "Failed to start maintenance scheduler"); - int majorVersion; int minorVersion; if (!RepoMetadata.Instance.TryGetOnDiskLayoutVersion(out majorVersion, out minorVersion, out error)) @@ -303,14 +299,5 @@ private void MountAndStartWorkingDirectoryCallbacks(CacheServerInfo cache) ScalarPlatform.Instance.DiskLayoutUpgrade.Version.CurrentMajorVersion); } } - - private void UnmountAndStopWorkingDirectoryCallbacks() - { - if (this.maintenanceScheduler != null) - { - this.maintenanceScheduler.Dispose(); - this.maintenanceScheduler = null; - } - } } } diff --git a/Scalar.Service/IRepoMounter.cs b/Scalar.Service/IRepoMounter.cs deleted file mode 100644 index 675260a2c9..0000000000 --- a/Scalar.Service/IRepoMounter.cs +++ /dev/null @@ -1,7 +0,0 @@ -namespace Scalar.Service -{ - public interface IRepoMounter - { - bool MountRepository(string repoRoot, int sessionId); - } -} diff --git a/Scalar.Service/IRepoRegistry.cs b/Scalar.Service/IRepoRegistry.cs index fd07f716ee..3ec97b3086 100644 --- a/Scalar.Service/IRepoRegistry.cs +++ b/Scalar.Service/IRepoRegistry.cs @@ -8,7 +8,7 @@ public interface IRepoRegistry bool TryDeactivateRepo(string repoRoot, out string errorMessage); bool TryGetActiveRepos(out List repoList, out string errorMessage); bool TryRemoveRepo(string repoRoot, out string errorMessage); - void AutoMountRepos(string userId, int sessionId); + void RunMaintenanceTaskForRepos(string task, string userId, int sessionId); void TraceStatus(); } } diff --git a/Scalar.Service/IScalarVerbRunner.cs b/Scalar.Service/IScalarVerbRunner.cs new file mode 100644 index 0000000000..769ba527dd --- /dev/null +++ b/Scalar.Service/IScalarVerbRunner.cs @@ -0,0 +1,7 @@ +namespace Scalar.Service +{ + public interface IScalarVerbRunner + { + bool CallMaintenance(string task, string repoRoot, int sessionId); + } +} diff --git a/Scalar.Service/IServiceTask.cs b/Scalar.Service/IServiceTask.cs new file mode 100644 index 0000000000..3349eae95c --- /dev/null +++ b/Scalar.Service/IServiceTask.cs @@ -0,0 +1,8 @@ +namespace Scalar.Service +{ + public interface IServiceTask + { + void Execute(); + void Stop(); + } +} diff --git a/Scalar.Service/MaintenanceTaskScheduler.cs b/Scalar.Service/MaintenanceTaskScheduler.cs new file mode 100644 index 0000000000..754db86c98 --- /dev/null +++ b/Scalar.Service/MaintenanceTaskScheduler.cs @@ -0,0 +1,178 @@ +using Scalar.Common; +using Scalar.Common.Tracing; +using System; +using System.Collections.Generic; +using System.Threading; + +namespace Scalar.Service +{ + public class MaintenanceTaskScheduler : IDisposable + { + private readonly TimeSpan looseObjectsDueTime = TimeSpan.FromMinutes(5); + private readonly TimeSpan looseObjectsPeriod = TimeSpan.FromHours(6); + + private readonly TimeSpan packfileDueTime = TimeSpan.FromMinutes(30); + private readonly TimeSpan packfilePeriod = TimeSpan.FromHours(12); + + private readonly TimeSpan commitGraphDueTime = TimeSpan.FromMinutes(15); + private readonly TimeSpan commitGraphPeriod = TimeSpan.FromHours(1); + + private readonly TimeSpan fetchCommitsAndTreesPeriod = TimeSpan.FromMinutes(15); + + private readonly ITracer tracer; + private readonly ServiceTaskQueue taskQueue; + private List stepTimers; + private UserAndSession registeredUser; + + public MaintenanceTaskScheduler(ITracer tracer, IRepoRegistry repoRegistry) + { + this.tracer = tracer; + this.stepTimers = new List(); + this.taskQueue = new ServiceTaskQueue(this.tracer); + this.ScheduleRecurringSteps(repoRegistry); + } + + public void RegisterUser(string userId, int sessionId) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add(nameof(userId), userId); + metadata.Add(nameof(sessionId), sessionId); + metadata.Add( + TracingConstants.MessageKey.InfoMessage, + $"{nameof(MaintenanceTaskScheduler)}: Registering user"); + this.tracer.RelatedEvent(EventLevel.Informational, nameof(this.RegisterUser), metadata); + + this.registeredUser = new UserAndSession(userId, sessionId); + } + + public void Dispose() + { + this.taskQueue.Stop(); + + foreach (Timer timer in this.stepTimers) + { + timer?.Dispose(); + } + + this.stepTimers = null; + } + + private void ScheduleRecurringSteps(IRepoRegistry repoRegistry) + { + if (ScalarEnlistment.IsUnattended(this.tracer)) + { + this.tracer.RelatedInfo($"{nameof(this.ScheduleRecurringSteps)}: Skipping maintenance tasks due to running unattended"); + return; + } + + Func getRegisteredUser = () => { return this.registeredUser; }; + + this.stepTimers.Add(new Timer( + (state) => this.taskQueue.TryEnqueue( + new MaintenanceTask( + this.tracer, + repoRegistry, + getRegisteredUser, + ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName)), + state: null, + dueTime: this.fetchCommitsAndTreesPeriod, + period: this.fetchCommitsAndTreesPeriod)); + + this.stepTimers.Add(new Timer( + (state) => this.taskQueue.TryEnqueue( + new MaintenanceTask( + this.tracer, + repoRegistry, + getRegisteredUser, + ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName)), + state: null, + dueTime: this.looseObjectsDueTime, + period: this.looseObjectsPeriod)); + + this.stepTimers.Add(new Timer( + (state) => this.taskQueue.TryEnqueue( + new MaintenanceTask( + this.tracer, + repoRegistry, + getRegisteredUser, + ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName)), + state: null, + dueTime: this.packfileDueTime, + period: this.packfilePeriod)); + + this.stepTimers.Add(new Timer( + (state) => this.taskQueue.TryEnqueue( + new MaintenanceTask( + this.tracer, + repoRegistry, + getRegisteredUser, + ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName)), + state: null, + dueTime: this.commitGraphDueTime, + period: this.commitGraphPeriod)); + } + + private class MaintenanceTask : IServiceTask + { + private readonly string task; + private readonly IRepoRegistry repoRegistry; + private readonly ITracer tracer; + private readonly Func getRegisteredUser; + + public MaintenanceTask( + ITracer tracer, + IRepoRegistry repoRegistry, + Func getRegisteredUser, + string task) + { + this.tracer = tracer; + this.repoRegistry = repoRegistry; + this.getRegisteredUser = getRegisteredUser; + this.task = task; + } + + public void Execute() + { + UserAndSession registeredUser = this.getRegisteredUser(); + if (registeredUser != null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add(nameof(registeredUser.UserId), registeredUser.UserId); + metadata.Add(nameof(registeredUser.SessionId), registeredUser.SessionId); + metadata.Add(nameof(this.task), this.task); + metadata.Add(TracingConstants.MessageKey.InfoMessage, "Executing maintenance task"); + this.tracer.RelatedEvent( + EventLevel.Informational, + $"{nameof(MaintenanceTaskScheduler)}.{nameof(this.Execute)}", + metadata); + + this.repoRegistry.RunMaintenanceTaskForRepos( + this.task, + registeredUser.UserId, + registeredUser.SessionId); + } + else + { + this.tracer.RelatedInfo($"{nameof(MaintenanceTask)}: Skipping '{this.task}', no registered user"); + } + } + + public void Stop() + { + // TODO: #185 - Kill the currently running maintenance verb + } + } + + private class UserAndSession + { + public UserAndSession(string userId, int sessionId) + { + this.UserId = userId; + this.SessionId = sessionId; + } + + public string UserId { get; } + public int SessionId { get; } + } + } +} diff --git a/Scalar.Service/Program.Mac.cs b/Scalar.Service/Program.Mac.cs index 685b7717fa..ef5f7901a3 100644 --- a/Scalar.Service/Program.Mac.cs +++ b/Scalar.Service/Program.Mac.cs @@ -52,8 +52,7 @@ private static ScalarService CreateService(JsonTracer tracer, string[] args) tracer, new PhysicalFileSystem(), serviceDataLocation, - new ScalarMountProcess(tracer), - new NotificationHandler(tracer)); + new ScalarVerbRunner(tracer)); return new ScalarService(tracer, serviceName, repoRegistry); } diff --git a/Scalar.Service/RepoRegistry.cs b/Scalar.Service/RepoRegistry.cs index 0055733e90..4051a05c27 100644 --- a/Scalar.Service/RepoRegistry.cs +++ b/Scalar.Service/RepoRegistry.cs @@ -1,5 +1,4 @@ using Scalar.Common.FileSystem; -using Scalar.Common.NamedPipes; using Scalar.Common.Tracing; using Scalar.Service.Handlers; using System; @@ -20,21 +19,18 @@ public class RepoRegistry : IRepoRegistry private ITracer tracer; private PhysicalFileSystem fileSystem; private object repoLock = new object(); - private IRepoMounter repoMounter; - private INotificationHandler notificationHandler; + private IScalarVerbRunner scalarVerb; public RepoRegistry( ITracer tracer, PhysicalFileSystem fileSystem, string serviceDataLocation, - IRepoMounter repoMounter, - INotificationHandler notificationHandler) + IScalarVerbRunner repoMounter) { this.tracer = tracer; this.fileSystem = fileSystem; this.registryParentFolderPath = serviceDataLocation; - this.repoMounter = repoMounter; - this.notificationHandler = notificationHandler; + this.scalarVerb = repoMounter; EventMetadata metadata = new EventMetadata(); metadata.Add("Area", EtwArea); @@ -43,16 +39,6 @@ public RepoRegistry( this.tracer.RelatedEvent(EventLevel.Informational, "RepoRegistry_Created", metadata); } - public void Upgrade() - { - // Version 1 to Version 2, added OwnerSID - Dictionary allRepos = this.ReadRegistry(); - if (allRepos.Any()) - { - this.WriteRegistry(allRepos); - } - } - public bool TryRegisterRepo(string repoRoot, string ownerSID, out string errorMessage) { errorMessage = null; @@ -170,29 +156,39 @@ public bool TryRemoveRepo(string repoRoot, out string errorMessage) return false; } - public void AutoMountRepos(string userId, int sessionId) + public void RunMaintenanceTaskForRepos(string task, string userId, int sessionId) { - using (ITracer activity = this.tracer.StartActivity("AutoMount", EventLevel.Informational)) + EventMetadata metadata = CreateEventMetadata(); + metadata.Add(nameof(task), task); + metadata.Add(nameof(userId), userId); + metadata.Add(nameof(sessionId), sessionId); + + List activeRepos = this.GetActiveReposForUser(userId); + if (activeRepos.Count == 0) { - List activeRepos = this.GetActiveReposForUser(userId); - if (activeRepos.Count > 0) - { - this.SendNotification( - sessionId, - NamedPipeMessages.Notification.Request.Identifier.AutomountStart, - enlistment: null, - enlistmentCount: activeRepos.Count); - } + metadata.Add(TracingConstants.MessageKey.InfoMessage, "No active repos for user"); + this.tracer.RelatedEvent( + EventLevel.Informational, + $"{nameof(this.RunMaintenanceTaskForRepos)}_NoRepos", + metadata); + } + else + { + metadata.Add(TracingConstants.MessageKey.InfoMessage, "Calling maintenance verb"); foreach (RepoRegistration repo in activeRepos) { - // TODO #1089: We need to respect the elevation level of the original mount - if (!this.repoMounter.MountRepository(repo.EnlistmentRoot, sessionId)) + metadata[nameof(repo.EnlistmentRoot)] = repo.EnlistmentRoot; + metadata[nameof(task)] = task; + this.tracer.RelatedEvent( + EventLevel.Informational, + $"{nameof(this.RunMaintenanceTaskForRepos)}_CallingMaintenance", + metadata); + + if (!this.scalarVerb.CallMaintenance(task, repo.EnlistmentRoot, sessionId)) { - this.SendNotification( - sessionId, - NamedPipeMessages.Notification.Request.Identifier.MountFailure, - repo.EnlistmentRoot); + // TODO: #111 - If the maintenance verb failed because the repo is no longer + // on disk, it should be removed from the registry } } } @@ -218,8 +214,7 @@ public Dictionary ReadRegistry() { if (versionString != null) { - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", EtwArea); + EventMetadata metadata = CreateEventMetadata(); metadata.Add("OnDiskVersion", versionString); metadata.Add("ExpectedVersion", versionString); this.tracer.RelatedError(metadata, "ReadRegistry: Unsupported version"); @@ -243,7 +238,7 @@ public Dictionary ReadRegistry() { if (!normalizedEnlistmentRootPath.Equals(registration.EnlistmentRoot, StringComparison.OrdinalIgnoreCase)) { - EventMetadata metadata = new EventMetadata(); + EventMetadata metadata = CreateEventMetadata(); metadata.Add("registration.EnlistmentRoot", registration.EnlistmentRoot); metadata.Add(nameof(normalizedEnlistmentRootPath), normalizedEnlistmentRootPath); metadata.Add(TracingConstants.MessageKey.InfoMessage, $"{nameof(this.ReadRegistry)}: Mapping registered enlistment root to final path"); @@ -252,7 +247,7 @@ public Dictionary ReadRegistry() } else { - EventMetadata metadata = new EventMetadata(); + EventMetadata metadata = CreateEventMetadata(); metadata.Add("registration.EnlistmentRoot", registration.EnlistmentRoot); metadata.Add("NormalizedEnlistmentRootPath", normalizedEnlistmentRootPath); metadata.Add("ErrorMessage", errorMessage); @@ -266,10 +261,8 @@ public Dictionary ReadRegistry() } catch (Exception e) { - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", EtwArea); + EventMetadata metadata = CreateEventMetadata(e); metadata.Add("entry", entry); - metadata.Add("Exception", e.ToString()); this.tracer.RelatedError(metadata, "ReadRegistry: Failed to read entry"); } } @@ -304,6 +297,18 @@ public bool TryGetActiveRepos(out List repoList, out string er } } + private static EventMetadata CreateEventMetadata(Exception e = null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", EtwArea); + if (e != null) + { + metadata.Add("Exception", e.ToString()); + } + + return metadata; + } + private List GetActiveReposForUser(string ownerSID) { lock (this.repoLock) @@ -325,20 +330,6 @@ private List GetActiveReposForUser(string ownerSID) } } - private void SendNotification( - int sessionId, - NamedPipeMessages.Notification.Request.Identifier requestId, - string enlistment = null, - int enlistmentCount = 0) - { - NamedPipeMessages.Notification.Request request = new NamedPipeMessages.Notification.Request(); - request.Id = requestId; - request.Enlistment = enlistment; - request.EnlistmentCount = enlistmentCount; - - this.notificationHandler.SendNotification(request); - } - private void WriteRegistry(Dictionary registry) { string tempFilePath = Path.Combine(this.registryParentFolderPath, RegistryTempName); diff --git a/Scalar.Service/Scalar.Service.Mac.csproj b/Scalar.Service/Scalar.Service.Mac.csproj index 86e7893ab9..15c2e9920d 100644 --- a/Scalar.Service/Scalar.Service.Mac.csproj +++ b/Scalar.Service/Scalar.Service.Mac.csproj @@ -21,7 +21,7 @@ - + @@ -30,11 +30,14 @@ - + + + + diff --git a/Scalar.Service/Scalar.Service.Windows.csproj b/Scalar.Service/Scalar.Service.Windows.csproj index af9ea5ea5d..1da0c5905c 100644 --- a/Scalar.Service/Scalar.Service.Windows.csproj +++ b/Scalar.Service/Scalar.Service.Windows.csproj @@ -56,10 +56,13 @@ - + + + Component + @@ -67,7 +70,7 @@ - + diff --git a/Scalar.Service/ScalarMountProcess.Mac.cs b/Scalar.Service/ScalarMountProcess.Mac.cs deleted file mode 100644 index f760740da5..0000000000 --- a/Scalar.Service/ScalarMountProcess.Mac.cs +++ /dev/null @@ -1,87 +0,0 @@ -using Scalar.Common; -using Scalar.Common.Tracing; -using System.Diagnostics; -using System.IO; - -namespace Scalar.Service -{ - public class ScalarMountProcess : IRepoMounter - { - private const string ExecutablePath = "/bin/launchctl"; - - private MountLauncher processLauncher; - private ITracer tracer; - - public ScalarMountProcess(ITracer tracer, MountLauncher processLauncher = null) - { - this.tracer = tracer; - this.processLauncher = processLauncher ?? new MountLauncher(tracer); - } - - public bool MountRepository(string repoRoot, int sessionId) - { - string arguments = string.Format( - "asuser {0} {1} mount {2}", - sessionId, - Path.Combine(ScalarPlatform.Instance.Constants.ScalarBinDirectoryPath, ScalarPlatform.Instance.Constants.ScalarExecutableName), - repoRoot); - - if (!this.processLauncher.LaunchProcess(ExecutablePath, arguments, repoRoot)) - { - this.tracer.RelatedError($"{nameof(this.MountRepository)}: Unable to start the Scalar process."); - return false; - } - - string errorMessage; - if (!this.processLauncher.WaitUntilMounted(this.tracer, repoRoot, false, out errorMessage)) - { - this.tracer.RelatedError(errorMessage); - return false; - } - - return true; - } - - public class MountLauncher - { - private ITracer tracer; - - public MountLauncher(ITracer tracer) - { - this.tracer = tracer; - } - - public virtual bool LaunchProcess(string executablePath, string arguments, string workingDirectory) - { - ProcessStartInfo processInfo = new ProcessStartInfo(executablePath); - processInfo.Arguments = arguments; - processInfo.WindowStyle = ProcessWindowStyle.Hidden; - processInfo.WorkingDirectory = workingDirectory; - processInfo.UseShellExecute = false; - processInfo.RedirectStandardOutput = true; - - ProcessResult result = ProcessHelper.Run(processInfo); - if (result.ExitCode != 0) - { - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", nameof(ScalarMountProcess)); - metadata.Add(nameof(executablePath), executablePath); - metadata.Add(nameof(arguments), arguments); - metadata.Add(nameof(workingDirectory), workingDirectory); - metadata.Add(nameof(result.ExitCode), result.ExitCode); - metadata.Add(nameof(result.Errors), result.Errors); - - this.tracer.RelatedError(metadata, $"{nameof(this.LaunchProcess)} ERROR: Could not launch {executablePath}"); - return false; - } - - return true; - } - - public virtual bool WaitUntilMounted(ITracer tracer, string enlistmentRoot, bool unattended, out string errorMessage) - { - return ScalarEnlistment.WaitUntilMounted(tracer, enlistmentRoot, unattended: false, errorMessage: out errorMessage); - } - } - } -} diff --git a/Scalar.Service/ScalarMountProcess.Windows.cs b/Scalar.Service/ScalarMountProcess.Windows.cs deleted file mode 100644 index 09e8b1303b..0000000000 --- a/Scalar.Service/ScalarMountProcess.Windows.cs +++ /dev/null @@ -1,46 +0,0 @@ -using Scalar.Common; -using Scalar.Common.Tracing; -using Scalar.Platform.Windows; -using Scalar.Service.Handlers; - -namespace Scalar.Service -{ - public class ScalarMountProcess : IRepoMounter - { - private readonly ITracer tracer; - - public ScalarMountProcess(ITracer tracer) - { - this.tracer = tracer; - } - - public bool MountRepository(string repoRoot, int sessionId) - { - using (CurrentUser currentUser = new CurrentUser(this.tracer, sessionId)) - { - if (!this.CallScalarMount(repoRoot, currentUser)) - { - this.tracer.RelatedError($"{nameof(this.MountRepository)}: Unable to start the Scalar.exe process."); - return false; - } - - string errorMessage; - if (!ScalarEnlistment.WaitUntilMounted(this.tracer, repoRoot, false, out errorMessage)) - { - this.tracer.RelatedError(errorMessage); - return false; - } - } - - return true; - } - - private bool CallScalarMount(string repoRoot, CurrentUser currentUser) - { - InternalVerbParameters mountInternal = new InternalVerbParameters(startedByService: true); - return currentUser.RunAs( - Configuration.Instance.ScalarLocation, - $"mount {repoRoot} --{ScalarConstants.VerbParameters.InternalUseOnly} {mountInternal.ToJson()}"); - } - } -} diff --git a/Scalar.Service/ScalarService.Mac.cs b/Scalar.Service/ScalarService.Mac.cs index e7fb24aa8f..f3d601fd58 100644 --- a/Scalar.Service/ScalarService.Mac.cs +++ b/Scalar.Service/ScalarService.Mac.cs @@ -19,6 +19,7 @@ public class ScalarService private string serviceName; private IRepoRegistry repoRegistry; private RequestHandler requestHandler; + private MaintenanceTaskScheduler maintenanceTaskScheduler; public ScalarService( ITracer tracer, @@ -38,8 +39,6 @@ public void Run() { try { - this.AutoMountReposForUser(); - if (!string.IsNullOrEmpty(this.serviceName)) { string pipeName = ScalarPlatform.Instance.GetScalarServiceNamedPipeName(this.serviceName); @@ -65,16 +64,61 @@ public void Run() } } + private static EventMetadata CreateEventMetadata(Exception e = null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", EtwArea); + if (e != null) + { + metadata.Add("Exception", e.ToString()); + } + + return metadata; + } + private void ServiceThreadMain() { try { + string currentUser = ScalarPlatform.Instance.GetCurrentUser(); + EventMetadata metadata = new EventMetadata(); metadata.Add("Version", ProcessHelper.GetCurrentProcessVersion()); + metadata.Add(nameof(currentUser), currentUser); this.tracer.RelatedEvent(EventLevel.Informational, $"{nameof(ScalarService)}_{nameof(this.ServiceThreadMain)}", metadata); + if (int.TryParse(currentUser, out int sessionId)) + { + try + { + this.maintenanceTaskScheduler = new MaintenanceTaskScheduler(this.tracer, this.repoRegistry); + + // On Mac, there is no separate session Id. currentUser is used as sessionId + this.maintenanceTaskScheduler.RegisterUser(currentUser, sessionId); + } + catch (Exception e) + { + this.tracer.RelatedError(CreateEventMetadata(e), "Failed to start maintenance scheduler"); + } + } + else + { + EventMetadata errorMetadata = CreateEventMetadata(); + errorMetadata.Add(nameof(currentUser), currentUser); + this.tracer.RelatedError( + errorMetadata, + $"{nameof(this.ServiceThreadMain)}: Failed to parse current user as int."); + } + this.serviceStopped.WaitOne(); this.serviceStopped.Dispose(); + this.serviceStopped = null; + + if (this.maintenanceTaskScheduler != null) + { + this.maintenanceTaskScheduler.Dispose(); + this.maintenanceTaskScheduler = null; + } } catch (Exception e) { @@ -82,27 +126,9 @@ private void ServiceThreadMain() } } - private void AutoMountReposForUser() - { - string currentUser = ScalarPlatform.Instance.GetCurrentUser(); - if (int.TryParse(currentUser, out int sessionId)) - { - // On Mac, there is no separate session Id. currentUser is used as sessionId - this.repoRegistry.AutoMountRepos(currentUser, sessionId); - this.repoRegistry.TraceStatus(); - } - else - { - this.tracer.RelatedError($"{nameof(this.AutoMountReposForUser)} Error: could not parse current user({currentUser}) info from RepoRegistry."); - } - } - private void LogExceptionAndExit(Exception e, string method) { - EventMetadata metadata = new EventMetadata(); - metadata.Add("Area", EtwArea); - metadata.Add("Exception", e.ToString()); - this.tracer.RelatedError(metadata, "Unhandled exception in " + method); + this.tracer.RelatedError(CreateEventMetadata(e), "Unhandled exception in " + method); Environment.Exit((int)ReturnCode.GenericError); } } diff --git a/Scalar.Service/ScalarService.Windows.cs b/Scalar.Service/ScalarService.Windows.cs index ccf4f87a28..7416201246 100644 --- a/Scalar.Service/ScalarService.Windows.cs +++ b/Scalar.Service/ScalarService.Windows.cs @@ -27,6 +27,7 @@ public class ScalarService : ServiceBase private RepoRegistry repoRegistry; private ProductUpgradeTimer productUpgradeTimer; private RequestHandler requestHandler; + private MaintenanceTaskScheduler maintenanceTaskScheduler; private INotificationHandler notificationHandler; public ScalarService(JsonTracer tracer) @@ -50,9 +51,10 @@ public void Run() this.tracer, new PhysicalFileSystem(), this.serviceDataLocation, - new ScalarMountProcess(this.tracer), - this.notificationHandler); - this.repoRegistry.Upgrade(); + new ScalarVerbRunner(this.tracer)); + + this.maintenanceTaskScheduler = new MaintenanceTaskScheduler(this.tracer, this.repoRegistry); + this.requestHandler = new RequestHandler(this.tracer, EtwArea, this.repoRegistry); string pipeName = ScalarPlatform.Instance.GetScalarServiceNamedPipeName(this.serviceName); @@ -132,15 +134,14 @@ protected override void OnSessionChange(SessionChangeDescription changeDescripti using (ITracer activity = this.tracer.StartActivity("LogonAutomount", EventLevel.Informational)) { - this.repoRegistry.AutoMountRepos( + this.maintenanceTaskScheduler.RegisterUser( ScalarPlatform.Instance.GetUserIdFromLoginSessionId(changeDescription.SessionId, this.tracer), changeDescription.SessionId); - this.repoRegistry.TraceStatus(); } } else if (changeDescription.Reason == SessionChangeReason.SessionLogoff) { - this.tracer.RelatedInfo("SessionLogoff detected"); + this.tracer.RelatedInfo($"SessionLogoff detected {changeDescription.SessionId}"); } } } @@ -262,7 +263,7 @@ private void CreateAndConfigureProgramDataDirectories() Directory.SetAccessControl(serviceDataRootPath, serviceDataRootSecurity); // Special rules for the upgrader logs, as non-elevated users need to be be able to write - this.CreateAndConfigureLogDirectory(ProductUpgraderInfo.GetLogDirectoryPath()); + this.CreateAndConfigureLogDirectory(ProductUpgraderInfo.GetLogDirectoryPath()); this.CreateAndConfigureLogDirectory(ScalarPlatform.Instance.GetDataRootForScalarComponent(ScalarConstants.Service.UIName)); } diff --git a/Scalar.Service/ScalarVerbRunner.Mac.cs b/Scalar.Service/ScalarVerbRunner.Mac.cs new file mode 100644 index 0000000000..1ec9127ba3 --- /dev/null +++ b/Scalar.Service/ScalarVerbRunner.Mac.cs @@ -0,0 +1,76 @@ +using Scalar.Common; +using Scalar.Common.Tracing; +using System.Diagnostics; +using System.IO; + +namespace Scalar.Service +{ + public class ScalarVerbRunner : IScalarVerbRunner + { + private const string ExecutablePath = "/bin/launchctl"; + + private readonly string scalarBinPath; + private readonly string internalVerbJson; + + private ScalarProcessLauncher processLauncher; + private ITracer tracer; + + public ScalarVerbRunner(ITracer tracer, ScalarProcessLauncher processLauncher = null) + { + this.tracer = tracer; + this.processLauncher = processLauncher ?? new ScalarProcessLauncher(tracer); + + this.scalarBinPath = Path.Combine( + ScalarPlatform.Instance.Constants.ScalarBinDirectoryPath, + ScalarPlatform.Instance.Constants.ScalarExecutableName); + + InternalVerbParameters internalParams = new InternalVerbParameters(startedByService: true); + this.internalVerbJson = internalParams.ToJson(); + } + + public bool CallMaintenance(string task, string repoRoot, int sessionId) + { + string arguments = + $"asuser {sessionId} {this.scalarBinPath} maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"; + + ProcessResult result = this.processLauncher.LaunchProcess(ExecutablePath, arguments, repoRoot); + if (result.ExitCode != 0) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", nameof(ScalarVerbRunner)); + metadata.Add(nameof(ExecutablePath), ExecutablePath); + metadata.Add(nameof(arguments), arguments); + metadata.Add(nameof(repoRoot), repoRoot); + metadata.Add(nameof(result.ExitCode), result.ExitCode); + metadata.Add(nameof(result.Errors), result.Errors); + + this.tracer.RelatedError(metadata, $"{nameof(this.CallMaintenance)}: Maintenance verb failed"); + return false; + } + + return true; + } + + public class ScalarProcessLauncher + { + private ITracer tracer; + + public ScalarProcessLauncher(ITracer tracer) + { + this.tracer = tracer; + } + + public virtual ProcessResult LaunchProcess(string executablePath, string arguments, string workingDirectory) + { + ProcessStartInfo processInfo = new ProcessStartInfo(executablePath); + processInfo.Arguments = arguments; + processInfo.WindowStyle = ProcessWindowStyle.Hidden; + processInfo.WorkingDirectory = workingDirectory; + processInfo.UseShellExecute = false; + processInfo.RedirectStandardOutput = true; + + return ProcessHelper.Run(processInfo); + } + } + } +} diff --git a/Scalar.Service/ScalarVerbRunner.Windows.cs b/Scalar.Service/ScalarVerbRunner.Windows.cs new file mode 100644 index 0000000000..cf68fe1047 --- /dev/null +++ b/Scalar.Service/ScalarVerbRunner.Windows.cs @@ -0,0 +1,41 @@ +using Scalar.Common; +using Scalar.Common.Tracing; +using Scalar.Platform.Windows; + +namespace Scalar.Service +{ + public class ScalarVerbRunner : IScalarVerbRunner + { + private readonly ITracer tracer; + private readonly string internalVerbJson; + + public ScalarVerbRunner(ITracer tracer) + { + this.tracer = tracer; + + InternalVerbParameters internalParams = new InternalVerbParameters(startedByService: true); + this.internalVerbJson = internalParams.ToJson(); + } + + public bool CallMaintenance(string task, string repoRoot, int sessionId) + { + using (CurrentUser currentUser = new CurrentUser(this.tracer, sessionId)) + { + if (!this.CallScalarMaintenance(task, repoRoot, currentUser)) + { + this.tracer.RelatedError($"{nameof(this.CallMaintenance)}: Unable to start the Scalar.exe process."); + return false; + } + } + + return true; + } + + private bool CallScalarMaintenance(string task, string repoRoot, CurrentUser currentUser) + { + return currentUser.RunAs( + Configuration.Instance.ScalarLocation, + $"maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"); + } + } +} diff --git a/Scalar.Service/ServiceTaskQueue.cs b/Scalar.Service/ServiceTaskQueue.cs new file mode 100644 index 0000000000..b0c3d2411f --- /dev/null +++ b/Scalar.Service/ServiceTaskQueue.cs @@ -0,0 +1,103 @@ +using System; +using System.Collections.Concurrent; +using System.Threading; +using Scalar.Common.Tracing; + +namespace Scalar.Service +{ + public class ServiceTaskQueue + { + private readonly object queueLock = new object(); + private readonly ITracer tracer; + private BlockingCollection queue = new BlockingCollection(); + private IServiceTask currentTask; + + public ServiceTaskQueue(ITracer tracer) + { + this.tracer = tracer; + Thread worker = new Thread(() => this.RunQueue()); + worker.Name = "MaintenanceWorker"; + worker.IsBackground = true; + worker.Start(); + } + + public bool TryEnqueue(IServiceTask step) + { + try + { + lock (this.queueLock) + { + if (this.queue == null) + { + return false; + } + + this.queue.Add(step); + return true; + } + } + catch (InvalidOperationException) + { + // We called queue.CompleteAdding() + } + + return false; + } + + public void Stop() + { + lock (this.queueLock) + { + this.queue?.CompleteAdding(); + } + + this.currentTask?.Stop(); + } + + private void RunQueue() + { + while (true) + { + // We cannot take the lock here, as TryTake is blocking. + // However, this is the place to set 'this.queue' to null. + if (!this.queue.TryTake(out this.currentTask, Timeout.Infinite) + || this.queue.IsAddingCompleted) + { + lock (this.queueLock) + { + // A stop was requested + this.queue?.Dispose(); + this.queue = null; + return; + } + } + + try + { + this.currentTask.Execute(); + } + catch (Exception e) + { + EventMetadata metadata = this.CreateEventMetadata(nameof(this.RunQueue), e); + this.tracer.RelatedError( + metadata: metadata, + message: $"{nameof(this.RunQueue)}: Unexpected Exception while running service tasks: {e.Message}"); + } + } + } + + private EventMetadata CreateEventMetadata(string methodName, Exception exception = null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", nameof(ServiceTaskQueue)); + metadata.Add("Method", methodName); + if (exception != null) + { + metadata.Add("ExceptionMessage", exception.Message); + metadata.Add("StackTrace", exception.StackTrace); + } + + return metadata; + } + } +} diff --git a/Scalar.UnitTests/Service/Mac/MacServiceTests.cs b/Scalar.UnitTests/Service/Mac/MacServiceTests.cs index f495f8aef7..0970b5b97c 100644 --- a/Scalar.UnitTests/Service/Mac/MacServiceTests.cs +++ b/Scalar.UnitTests/Service/Mac/MacServiceTests.cs @@ -1,9 +1,7 @@ using Moq; using NUnit.Framework; using Scalar.Common; -using Scalar.Common.NamedPipes; using Scalar.Service; -using Scalar.Service.Handlers; using Scalar.UnitTests.Mock.Common; using Scalar.UnitTests.Mock.FileSystem; using System.IO; @@ -13,7 +11,6 @@ namespace Scalar.UnitTests.Service.Mac [TestFixture] public class MacServiceTests { - private const string ScalarServiceName = "Scalar.Service"; private const int ExpectedActiveUserId = 502; private const int ExpectedSessionId = 502; private static readonly string ExpectedActiveRepoPath = Path.Combine("mock:", "code", "repo2"); @@ -33,31 +30,12 @@ public void SetUp() } [TestCase] - public void ServiceStartTriggersAutoMountForCurrentUser() + public void RepoRegistryCallsMaintenanceVerbOnlyForRegisteredRepos() { - Mock repoRegistry = new Mock(MockBehavior.Strict); - repoRegistry.Setup(r => r.AutoMountRepos(ExpectedActiveUserId.ToString(), ExpectedSessionId)); - repoRegistry.Setup(r => r.TraceStatus()); + Mock repoMounterMock = new Mock(MockBehavior.Strict); - ScalarService service = new ScalarService( - this.tracer, - serviceName: null, - repoRegistry: repoRegistry.Object); - - service.Run(); - - repoRegistry.VerifyAll(); - } - - [TestCase] - public void RepoRegistryMountsOnlyRegisteredRepos() - { - Mock repoMounterMock = new Mock(MockBehavior.Strict); - Mock notificationHandlerMock = new Mock(MockBehavior.Strict); - - repoMounterMock.Setup(mp => mp.MountRepository(ExpectedActiveRepoPath, ExpectedActiveUserId)).Returns(true); - notificationHandlerMock.Setup(nh => nh.SendNotification( - It.Is(rq => rq.Id == NamedPipeMessages.Notification.Request.Identifier.AutomountStart))); + string task = "test-task"; + repoMounterMock.Setup(mp => mp.CallMaintenance(task, ExpectedActiveRepoPath, ExpectedActiveUserId)).Returns(true); this.CreateTestRepos(ServiceDataLocation); @@ -65,39 +43,31 @@ public void RepoRegistryMountsOnlyRegisteredRepos() this.tracer, this.fileSystem, ServiceDataLocation, - repoMounterMock.Object, - notificationHandlerMock.Object); + repoMounterMock.Object); - repoRegistry.AutoMountRepos(ExpectedActiveUserId.ToString(), ExpectedSessionId); + repoRegistry.RunMaintenanceTaskForRepos(task, ExpectedActiveUserId.ToString(), ExpectedSessionId); repoMounterMock.VerifyAll(); - notificationHandlerMock.VerifyAll(); } [TestCase] - public void MountProcessLaunchedUsingCorrectArgs() + public void MaintenanceVerbLaunchedUsingCorrectArgs() { + string task = "test-task"; string executable = @"/bin/launchctl"; string scalarBinPath = Path.Combine(this.scalarPlatform.Constants.ScalarBinDirectoryPath, this.scalarPlatform.Constants.ScalarExecutableName); - string expectedArgs = $"asuser {ExpectedActiveUserId} {scalarBinPath} mount {ExpectedActiveRepoPath}"; + string expectedArgs = + $"asuser {ExpectedActiveUserId} {scalarBinPath} maintenance \"{ExpectedActiveRepoPath}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {new InternalVerbParameters(startedByService: true).ToJson()}"; - Mock mountLauncherMock = new Mock(MockBehavior.Strict, this.tracer); + Mock mountLauncherMock = new Mock(MockBehavior.Strict, this.tracer); mountLauncherMock.Setup(mp => mp.LaunchProcess( executable, expectedArgs, ExpectedActiveRepoPath)) - .Returns(true); - - string errorString = null; - mountLauncherMock.Setup(mp => mp.WaitUntilMounted( - this.tracer, - ExpectedActiveRepoPath, - It.IsAny(), - out errorString)) - .Returns(true); + .Returns(new ProcessResult(output: string.Empty, errors: string.Empty, exitCode: 0)); - ScalarMountProcess mountProcess = new ScalarMountProcess(this.tracer, mountLauncherMock.Object); - mountProcess.MountRepository(ExpectedActiveRepoPath, ExpectedActiveUserId); + ScalarVerbRunner verbProcess = new ScalarVerbRunner(this.tracer, mountLauncherMock.Object); + verbProcess.CallMaintenance(task, ExpectedActiveRepoPath, ExpectedActiveUserId); mountLauncherMock.VerifyAll(); } diff --git a/Scalar.UnitTests/Service/RepoRegistryTests.cs b/Scalar.UnitTests/Service/RepoRegistryTests.cs index e0ee25399f..eaecda4040 100644 --- a/Scalar.UnitTests/Service/RepoRegistryTests.cs +++ b/Scalar.UnitTests/Service/RepoRegistryTests.cs @@ -1,7 +1,6 @@ using Moq; using NUnit.Framework; using Scalar.Service; -using Scalar.Service.Handlers; using Scalar.Tests.Should; using Scalar.UnitTests.Mock.Common; using Scalar.UnitTests.Mock.FileSystem; @@ -15,21 +14,18 @@ namespace Scalar.UnitTests.Service [TestFixture] public class RepoRegistryTests { - private Mock mockRepoMounter; - private Mock mockNotificationHandler; + private Mock mockRepoMounter; [SetUp] public void Setup() { - this.mockRepoMounter = new Mock(MockBehavior.Strict); - this.mockNotificationHandler = new Mock(MockBehavior.Strict); + this.mockRepoMounter = new Mock(MockBehavior.Strict); } [TearDown] public void TearDown() { this.mockRepoMounter.VerifyAll(); - this.mockNotificationHandler.VerifyAll(); } [TestCase] @@ -42,8 +38,7 @@ public void TryRegisterRepo_EmptyRegistry() new MockTracer(), fileSystem, dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); + this.mockRepoMounter.Object); string repoRoot = Path.Combine("c:", "test"); string ownerSID = Guid.NewGuid().ToString(); @@ -56,55 +51,6 @@ public void TryRegisterRepo_EmptyRegistry() this.VerifyRepo(verifiableRegistry[repoRoot], ownerSID, expectedIsActive: true); } - [TestCase] - public void ReadRegistry_Upgrade_ExistingVersion1() - { - string dataLocation = Path.Combine("mock:", "registryDataFolder"); - MockFileSystem fileSystem = new MockFileSystem(new MockDirectory(dataLocation, null, null)); - - string repo1 = Path.Combine("mock:", "code", "repo1"); - string repo2 = Path.Combine("mock:", "code", "repo2"); - - // Create a version 1 registry file - fileSystem.WriteAllText( - Path.Combine(dataLocation, RepoRegistry.RegistryName), -$@"1 -{{""EnlistmentRoot"":""{repo1.Replace("\\", "\\\\")}"",""IsActive"":false}} -{{""EnlistmentRoot"":""{repo2.Replace("\\", "\\\\")}"",""IsActive"":true}} -"); - - RepoRegistry registry = new RepoRegistry( - new MockTracer(), - fileSystem, - dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); - registry.Upgrade(); - - Dictionary repos = registry.ReadRegistry(); - repos.Count.ShouldEqual(2); - - this.VerifyRepo(repos[repo1], expectedOwnerSID: null, expectedIsActive: false); - this.VerifyRepo(repos[repo2], expectedOwnerSID: null, expectedIsActive: true); - } - - [TestCase] - public void ReadRegistry_Upgrade_NoRegistry() - { - string dataLocation = Path.Combine("mock:", "registryDataFolder"); - MockFileSystem fileSystem = new MockFileSystem(new MockDirectory(dataLocation, null, null)); - RepoRegistry registry = new RepoRegistry( - new MockTracer(), - fileSystem, - dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); - registry.Upgrade(); - - Dictionary repos = registry.ReadRegistry(); - repos.Count.ShouldEqual(0); - } - [TestCase] public void TryGetActiveRepos_BeforeAndAfterActivateAndDeactivate() { @@ -114,8 +60,7 @@ public void TryGetActiveRepos_BeforeAndAfterActivateAndDeactivate() new MockTracer(), fileSystem, dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); + this.mockRepoMounter.Object); string repo1Root = Path.Combine("mock:", "test", "repo1"); string owner1SID = Guid.NewGuid().ToString(); @@ -167,8 +112,7 @@ public void TryDeactivateRepo() new MockTracer(), fileSystem, dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); + this.mockRepoMounter.Object); string repo1Root = Path.Combine("mock:", "test", "repo1"); string owner1SID = Guid.NewGuid().ToString(); @@ -211,8 +155,7 @@ public void TraceStatus() tracer, fileSystem, dataLocation, - this.mockRepoMounter.Object, - this.mockNotificationHandler.Object); + this.mockRepoMounter.Object); string repo1Root = Path.Combine("mock:", "test", "repo1"); string owner1SID = Guid.NewGuid().ToString(); diff --git a/Scalar.UnitTests/Maintenance/GitMaintenanceQueueTests.cs b/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs similarity index 56% rename from Scalar.UnitTests/Maintenance/GitMaintenanceQueueTests.cs rename to Scalar.UnitTests/Service/ServiceTaskQueueTests.cs index b2bec9009e..a259bdc077 100644 --- a/Scalar.UnitTests/Maintenance/GitMaintenanceQueueTests.cs +++ b/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs @@ -1,18 +1,19 @@ -using NUnit.Framework; +using NUnit.Framework; using Scalar.Common; using Scalar.Common.FileSystem; using Scalar.Common.Git; using Scalar.Common.Maintenance; using Scalar.Common.Tracing; +using Scalar.Service; using Scalar.Tests.Should; using Scalar.UnitTests.Mock.Common; using System.Collections.Generic; using System.Threading; -namespace Scalar.UnitTests.Maintenance +namespace Scalar.UnitTests.Service { [TestFixture] - public class GitMaintenanceQueueTests + public class ServiceTaskQueueTests { private int maxWaitTime = 500; private ReadyFileSystem fileSystem; @@ -21,37 +22,14 @@ public class GitMaintenanceQueueTests private GitObjects gitObjects; [TestCase] - public void GitMaintenanceQueueEnlistmentRootReady() + public void ServiceTaskQueueHandlesTwoJobs() { this.TestSetup(); - GitMaintenanceQueue queue = new GitMaintenanceQueue(this.context); - queue.EnlistmentRootReady().ShouldBeTrue(); + TestServiceTask step1 = new TestServiceTask(); + TestServiceTask step2 = new TestServiceTask(); - this.fileSystem.Paths.Remove(this.enlistment.EnlistmentRoot); - queue.EnlistmentRootReady().ShouldBeFalse(); - - this.fileSystem.Paths.Remove(this.enlistment.GitObjectsRoot); - queue.EnlistmentRootReady().ShouldBeFalse(); - - this.fileSystem.Paths.Add(this.enlistment.EnlistmentRoot); - queue.EnlistmentRootReady().ShouldBeFalse(); - - this.fileSystem.Paths.Add(this.enlistment.GitObjectsRoot); - queue.EnlistmentRootReady().ShouldBeTrue(); - - queue.Stop(); - } - - [TestCase] - public void GitMaintenanceQueueHandlesTwoJobs() - { - this.TestSetup(); - - TestGitMaintenanceStep step1 = new TestGitMaintenanceStep(this.context); - TestGitMaintenanceStep step2 = new TestGitMaintenanceStep(this.context); - - GitMaintenanceQueue queue = new GitMaintenanceQueue(this.context); + ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); queue.TryEnqueue(step1); queue.TryEnqueue(step2); @@ -66,39 +44,39 @@ public void GitMaintenanceQueueHandlesTwoJobs() } [TestCase] - public void GitMaintenanceQueueStopSuceedsWhenQueueIsEmpty() + public void ServiceTaskQueueStopSuceedsWhenQueueIsEmpty() { this.TestSetup(); - GitMaintenanceQueue queue = new GitMaintenanceQueue(this.context); + ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); queue.Stop(); - TestGitMaintenanceStep step = new TestGitMaintenanceStep(this.context); + TestServiceTask step = new TestServiceTask(); queue.TryEnqueue(step).ShouldEqual(false); } [TestCase] - public void GitMaintenanceQueueStopsJob() + public void ServiceTaskQueueStopsJob() { this.TestSetup(); - GitMaintenanceQueue queue = new GitMaintenanceQueue(this.context); + ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); // This step stops the queue after the step is started, // then checks if Stop() was called. - WatchForStopStep watchForStop = new WatchForStopStep(queue, this.context); + WatchForStopTask watchForStop = new WatchForStopTask(queue); queue.TryEnqueue(watchForStop); - Assert.IsTrue(watchForStop.EventTriggered.WaitOne(this.maxWaitTime)); + watchForStop.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); watchForStop.SawStopping.ShouldBeTrue(); // Ensure we don't start a job after the Stop() call - TestGitMaintenanceStep watchForStart = new TestGitMaintenanceStep(this.context); + TestServiceTask watchForStart = new TestServiceTask(); queue.TryEnqueue(watchForStart).ShouldBeFalse(); // This only ensures the event didn't happen within maxWaitTime - Assert.IsFalse(watchForStart.EventTriggered.WaitOne(this.maxWaitTime)); + watchForStart.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeFalse(); queue.Stop(); } @@ -134,10 +112,9 @@ public override bool DirectoryExists(string path) } } - public class TestGitMaintenanceStep : GitMaintenanceStep + public class TestServiceTask : IServiceTask { - public TestGitMaintenanceStep(ScalarContext context) - : base(context, requireObjectCacheLock: true) + public TestServiceTask() { this.EventTriggered = new ManualResetEvent(initialState: false); } @@ -145,40 +122,41 @@ public TestGitMaintenanceStep(ScalarContext context) public ManualResetEvent EventTriggered { get; set; } public int NumberOfExecutions { get; set; } - public override string Area => "TestGitMaintenanceStep"; - - protected override void PerformMaintenance() + public void Execute() { this.NumberOfExecutions++; this.EventTriggered.Set(); } + + public void Stop() + { + } } - private class WatchForStopStep : GitMaintenanceStep + private class WatchForStopTask : IServiceTask { - public WatchForStopStep(GitMaintenanceQueue queue, ScalarContext context) - : base(context, requireObjectCacheLock: true) + public WatchForStopTask(ServiceTaskQueue queue) { this.Queue = queue; this.EventTriggered = new ManualResetEvent(false); } - public GitMaintenanceQueue Queue { get; set; } + public ServiceTaskQueue Queue { get; set; } public bool SawStopping { get; private set; } public ManualResetEvent EventTriggered { get; private set; } - public override string Area => "WatchForStopStep"; - - protected override void PerformMaintenance() + public void Execute() { this.Queue.Stop(); - - this.SawStopping = this.Stopping; - this.EventTriggered.Set(); } + + public void Stop() + { + this.SawStopping = true; + } } } } diff --git a/Scalar/CommandLine/CloneVerb.cs b/Scalar/CommandLine/CloneVerb.cs index 81a89171f2..042ad61657 100644 --- a/Scalar/CommandLine/CloneVerb.cs +++ b/Scalar/CommandLine/CloneVerb.cs @@ -263,7 +263,7 @@ private Result DoClone(string fullEnlistmentRootPathParameter, string normalized this.enlistment, verb => { - verb.MaintenanceTask = MaintenanceVerb.FetchCommitsAndTreesTaskName; + verb.MaintenanceTask = ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName; verb.SkipVersionCheck = true; verb.ResolvedCacheServer = this.cacheServer; verb.ServerScalarConfig = this.serverScalarConfig; diff --git a/Scalar/CommandLine/MaintenanceVerb.cs b/Scalar/CommandLine/MaintenanceVerb.cs index d5a2c3f998..e014a5917d 100644 --- a/Scalar/CommandLine/MaintenanceVerb.cs +++ b/Scalar/CommandLine/MaintenanceVerb.cs @@ -12,32 +12,24 @@ namespace Scalar.CommandLine [Verb(MaintenanceVerb.MaintenanceVerbName, HelpText = "Perform a maintenance task in a Scalar repo")] public class MaintenanceVerb : ScalarVerb.ForExistingEnlistment { - public const string FetchCommitsAndTreesTaskName = "fetch-commits-and-trees"; - private const string MaintenanceVerbName = "maintenance"; - private const string LooseObjectsTaskName = "loose-objects"; - private const string PackfilesTaskName = "pack-files"; - private const string CommitGraphTaskName = "commit-graph"; - - private const string BatchSizeOptionName = "batch-size"; - [Option( 't', "task", Required = true, Default = "", HelpText = "Maintenance task to run. Allowed values are '" - + LooseObjectsTaskName + "', '" - + PackfilesTaskName + "', '" - + CommitGraphTaskName + "'")] + + ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName + "', '" + + ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName + "', '" + + ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName + "'")] public string MaintenanceTask { get; set; } [Option( - BatchSizeOptionName, + ScalarConstants.VerbParameters.Maintenance.BatchSizeOptionName, Required = false, Default = "", - HelpText = "Batch size. This option can only be used with the '" + PackfilesTaskName + "' task")] + HelpText = "Batch size. This option can only be used with the '" + ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName + "' task")] public string PackfileMaintenanceBatchSize { get; set; } public bool SkipVersionCheck { get; set; } @@ -69,6 +61,7 @@ protected override void Execute(ScalarEnlistment enlistment) { nameof(this.MaintenanceTask), this.MaintenanceTask }, { nameof(this.PackfileMaintenanceBatchSize), this.PackfileMaintenanceBatchSize }, { nameof(this.EnlistmentRootPathParameter), this.EnlistmentRootPathParameter }, + { nameof(this.StartedByService), this.StartedByService }, })); this.InitializeLocalCacheAndObjectsPaths(tracer, enlistment, retryConfig: null, serverScalarConfig: null, cacheServer: null); @@ -79,12 +72,12 @@ protected override void Execute(ScalarEnlistment enlistment) { switch (this.MaintenanceTask) { - case LooseObjectsTaskName: + case ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName: this.FailIfBatchSizeSet(tracer); (new LooseObjectsStep(context, forceRun: true)).Execute(); return; - case PackfilesTaskName: + case ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName: (new PackfileMaintenanceStep( context, forceRun: true, @@ -93,12 +86,12 @@ protected override void Execute(ScalarEnlistment enlistment) this.PackfileMaintenanceBatchSize)).Execute(); return; - case FetchCommitsAndTreesTaskName: + case ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName: this.FailIfBatchSizeSet(tracer); this.FetchCommitsAndTrees(tracer, enlistment, cacheServerUrl); return; - case CommitGraphTaskName: + case ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName: this.FailIfBatchSizeSet(tracer); (new CommitGraphStep(context, requireObjectCacheLock: false)).Execute(); return; @@ -155,7 +148,7 @@ private void FailIfBatchSizeSet(ITracer tracer) this.ReportErrorAndExit( tracer, ReturnCode.UnsupportedOption, - $"--{BatchSizeOptionName} can only be used with the '{PackfilesTaskName}' task"); + $"--{ScalarConstants.VerbParameters.Maintenance.BatchSizeOptionName} can only be used with the '{ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName}' task"); } } From c4597a47bc3201f2755b7f4754192911beb5376f Mon Sep 17 00:00:00 2001 From: William Baker Date: Thu, 24 Oct 2019 15:00:23 -0700 Subject: [PATCH 2/4] scalar maintenance: simplify maintenance task management - Add a new IRegisteredUserStore and implement it with the MaintenanceTaskSchedule to avoid having to create/pass a Func to the maintenance task. - Remove the unnecessary lock from the ServiceTaskQueue - Simplify the creation of the timers in MaintenanceTaskScheduler --- Scalar.Service/IRegisteredUserStore.cs | 7 + Scalar.Service/MaintenanceTaskScheduler.cs | 126 +++++++++--------- Scalar.Service/RepoRegistry.cs | 5 +- Scalar.Service/Scalar.Service.Mac.csproj | 2 + Scalar.Service/Scalar.Service.Windows.csproj | 2 + Scalar.Service/ScalarService.Mac.cs | 2 +- Scalar.Service/ScalarService.Windows.cs | 11 +- Scalar.Service/ServiceTaskQueue.cs | 61 ++++----- Scalar.Service/UserAndSession.cs | 14 ++ .../Service/ServiceTaskQueueTests.cs | 102 ++++---------- 10 files changed, 154 insertions(+), 178 deletions(-) create mode 100644 Scalar.Service/IRegisteredUserStore.cs create mode 100644 Scalar.Service/UserAndSession.cs diff --git a/Scalar.Service/IRegisteredUserStore.cs b/Scalar.Service/IRegisteredUserStore.cs new file mode 100644 index 0000000000..677badf077 --- /dev/null +++ b/Scalar.Service/IRegisteredUserStore.cs @@ -0,0 +1,7 @@ +namespace Scalar.Service +{ + public interface IRegisteredUserStore + { + UserAndSession RegisteredUser { get; } + } +} diff --git a/Scalar.Service/MaintenanceTaskScheduler.cs b/Scalar.Service/MaintenanceTaskScheduler.cs index 754db86c98..5d63089d2f 100644 --- a/Scalar.Service/MaintenanceTaskScheduler.cs +++ b/Scalar.Service/MaintenanceTaskScheduler.cs @@ -6,7 +6,7 @@ namespace Scalar.Service { - public class MaintenanceTaskScheduler : IDisposable + public class MaintenanceTaskScheduler : IDisposable, IRegisteredUserStore { private readonly TimeSpan looseObjectsDueTime = TimeSpan.FromMinutes(5); private readonly TimeSpan looseObjectsPeriod = TimeSpan.FromHours(6); @@ -20,9 +20,8 @@ public class MaintenanceTaskScheduler : IDisposable private readonly TimeSpan fetchCommitsAndTreesPeriod = TimeSpan.FromMinutes(15); private readonly ITracer tracer; - private readonly ServiceTaskQueue taskQueue; + private ServiceTaskQueue taskQueue; private List stepTimers; - private UserAndSession registeredUser; public MaintenanceTaskScheduler(ITracer tracer, IRepoRegistry repoRegistry) { @@ -32,28 +31,35 @@ public MaintenanceTaskScheduler(ITracer tracer, IRepoRegistry repoRegistry) this.ScheduleRecurringSteps(repoRegistry); } - public void RegisterUser(string userId, int sessionId) + public UserAndSession RegisteredUser { get; private set; } + + public void RegisterUser(UserAndSession user) { EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(userId), userId); - metadata.Add(nameof(sessionId), sessionId); + metadata.Add(nameof(user.UserId), user.UserId); + metadata.Add(nameof(user.SessionId), user.SessionId); metadata.Add( TracingConstants.MessageKey.InfoMessage, $"{nameof(MaintenanceTaskScheduler)}: Registering user"); this.tracer.RelatedEvent(EventLevel.Informational, nameof(this.RegisterUser), metadata); - this.registeredUser = new UserAndSession(userId, sessionId); + this.RegisteredUser = user; } public void Dispose() { this.taskQueue.Stop(); - foreach (Timer timer in this.stepTimers) { - timer?.Dispose(); + using (ManualResetEvent timerDisposed = new ManualResetEvent(initialState: false)) + { + timer.Dispose(timerDisposed); + timerDisposed.WaitOne(); + } } + this.taskQueue.Dispose(); + this.taskQueue = null; this.stepTimers = null; } @@ -65,51 +71,53 @@ private void ScheduleRecurringSteps(IRepoRegistry repoRegistry) return; } - Func getRegisteredUser = () => { return this.registeredUser; }; - - this.stepTimers.Add(new Timer( - (state) => this.taskQueue.TryEnqueue( - new MaintenanceTask( - this.tracer, - repoRegistry, - getRegisteredUser, - ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName)), - state: null, - dueTime: this.fetchCommitsAndTreesPeriod, - period: this.fetchCommitsAndTreesPeriod)); - - this.stepTimers.Add(new Timer( + List tasks = new List() + { + new TaskTiming( + ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName, + dueTime: this.fetchCommitsAndTreesPeriod, + period: this.fetchCommitsAndTreesPeriod), + new TaskTiming( + ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName, + dueTime: this.looseObjectsDueTime, + period: this.looseObjectsPeriod), + new TaskTiming( + ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName, + dueTime: this.packfileDueTime, + period: this.packfilePeriod), + new TaskTiming( + ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName, + dueTime: this.commitGraphDueTime, + period: this.commitGraphPeriod), + }; + + foreach (TaskTiming task in tasks) + { + this.stepTimers.Add(new Timer( (state) => this.taskQueue.TryEnqueue( new MaintenanceTask( this.tracer, repoRegistry, - getRegisteredUser, - ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName)), + this, + task.Name)), state: null, - dueTime: this.looseObjectsDueTime, - period: this.looseObjectsPeriod)); + dueTime: task.DueTime, + period: task.Period)); + } + } - this.stepTimers.Add(new Timer( - (state) => this.taskQueue.TryEnqueue( - new MaintenanceTask( - this.tracer, - repoRegistry, - getRegisteredUser, - ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName)), - state: null, - dueTime: this.packfileDueTime, - period: this.packfilePeriod)); + private class TaskTiming + { + public TaskTiming(string name, TimeSpan dueTime, TimeSpan period) + { + this.Name = name; + this.DueTime = dueTime; + this.Period = period; + } - this.stepTimers.Add(new Timer( - (state) => this.taskQueue.TryEnqueue( - new MaintenanceTask( - this.tracer, - repoRegistry, - getRegisteredUser, - ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName)), - state: null, - dueTime: this.commitGraphDueTime, - period: this.commitGraphPeriod)); + public string Name { get; } + public TimeSpan DueTime { get; } + public TimeSpan Period { get; } } private class MaintenanceTask : IServiceTask @@ -117,23 +125,23 @@ private class MaintenanceTask : IServiceTask private readonly string task; private readonly IRepoRegistry repoRegistry; private readonly ITracer tracer; - private readonly Func getRegisteredUser; + private readonly IRegisteredUserStore registeredUserStore; public MaintenanceTask( ITracer tracer, IRepoRegistry repoRegistry, - Func getRegisteredUser, + IRegisteredUserStore registeredUserStore, string task) { this.tracer = tracer; this.repoRegistry = repoRegistry; - this.getRegisteredUser = getRegisteredUser; + this.registeredUserStore = registeredUserStore; this.task = task; } public void Execute() { - UserAndSession registeredUser = this.getRegisteredUser(); + UserAndSession registeredUser = this.registeredUserStore.RegisteredUser; if (registeredUser != null) { EventMetadata metadata = new EventMetadata(); @@ -148,8 +156,8 @@ public void Execute() this.repoRegistry.RunMaintenanceTaskForRepos( this.task, - registeredUser.UserId, - registeredUser.SessionId); + this.registeredUserStore.RegisteredUser.UserId, + this.registeredUserStore.RegisteredUser.SessionId); } else { @@ -162,17 +170,5 @@ public void Stop() // TODO: #185 - Kill the currently running maintenance verb } } - - private class UserAndSession - { - public UserAndSession(string userId, int sessionId) - { - this.UserId = userId; - this.SessionId = sessionId; - } - - public string UserId { get; } - public int SessionId { get; } - } } } diff --git a/Scalar.Service/RepoRegistry.cs b/Scalar.Service/RepoRegistry.cs index 4051a05c27..1bef51213c 100644 --- a/Scalar.Service/RepoRegistry.cs +++ b/Scalar.Service/RepoRegistry.cs @@ -1,6 +1,5 @@ using Scalar.Common.FileSystem; using Scalar.Common.Tracing; -using Scalar.Service.Handlers; using System; using System.Collections.Generic; using System.IO; @@ -25,12 +24,12 @@ public RepoRegistry( ITracer tracer, PhysicalFileSystem fileSystem, string serviceDataLocation, - IScalarVerbRunner repoMounter) + IScalarVerbRunner scalarVerbRunner) { this.tracer = tracer; this.fileSystem = fileSystem; this.registryParentFolderPath = serviceDataLocation; - this.scalarVerb = repoMounter; + this.scalarVerb = scalarVerbRunner; EventMetadata metadata = new EventMetadata(); metadata.Add("Area", EtwArea); diff --git a/Scalar.Service/Scalar.Service.Mac.csproj b/Scalar.Service/Scalar.Service.Mac.csproj index 15c2e9920d..7c0ee5df85 100644 --- a/Scalar.Service/Scalar.Service.Mac.csproj +++ b/Scalar.Service/Scalar.Service.Mac.csproj @@ -38,6 +38,8 @@ + + diff --git a/Scalar.Service/Scalar.Service.Windows.csproj b/Scalar.Service/Scalar.Service.Windows.csproj index 1da0c5905c..f15c2e7922 100644 --- a/Scalar.Service/Scalar.Service.Windows.csproj +++ b/Scalar.Service/Scalar.Service.Windows.csproj @@ -71,12 +71,14 @@ + + diff --git a/Scalar.Service/ScalarService.Mac.cs b/Scalar.Service/ScalarService.Mac.cs index f3d601fd58..390b16c51e 100644 --- a/Scalar.Service/ScalarService.Mac.cs +++ b/Scalar.Service/ScalarService.Mac.cs @@ -94,7 +94,7 @@ private void ServiceThreadMain() this.maintenanceTaskScheduler = new MaintenanceTaskScheduler(this.tracer, this.repoRegistry); // On Mac, there is no separate session Id. currentUser is used as sessionId - this.maintenanceTaskScheduler.RegisterUser(currentUser, sessionId); + this.maintenanceTaskScheduler.RegisterUser(new UserAndSession(currentUser, sessionId)); } catch (Exception e) { diff --git a/Scalar.Service/ScalarService.Windows.cs b/Scalar.Service/ScalarService.Windows.cs index 7416201246..53037c8a24 100644 --- a/Scalar.Service/ScalarService.Windows.cs +++ b/Scalar.Service/ScalarService.Windows.cs @@ -105,6 +105,12 @@ public void StopRunning() this.serviceThread.Join(); this.serviceThread = null; + if (this.maintenanceTaskScheduler != null) + { + this.maintenanceTaskScheduler.Dispose(); + this.maintenanceTaskScheduler = null; + } + if (this.serviceStopped != null) { this.serviceStopped.Dispose(); @@ -135,8 +141,9 @@ protected override void OnSessionChange(SessionChangeDescription changeDescripti using (ITracer activity = this.tracer.StartActivity("LogonAutomount", EventLevel.Informational)) { this.maintenanceTaskScheduler.RegisterUser( - ScalarPlatform.Instance.GetUserIdFromLoginSessionId(changeDescription.SessionId, this.tracer), - changeDescription.SessionId); + new UserAndSession( + ScalarPlatform.Instance.GetUserIdFromLoginSessionId(changeDescription.SessionId, this.tracer), + changeDescription.SessionId)); } } else if (changeDescription.Reason == SessionChangeReason.SessionLogoff) diff --git a/Scalar.Service/ServiceTaskQueue.cs b/Scalar.Service/ServiceTaskQueue.cs index b0c3d2411f..b2a010b1e6 100644 --- a/Scalar.Service/ServiceTaskQueue.cs +++ b/Scalar.Service/ServiceTaskQueue.cs @@ -5,36 +5,28 @@ namespace Scalar.Service { - public class ServiceTaskQueue + public class ServiceTaskQueue : IDisposable { - private readonly object queueLock = new object(); private readonly ITracer tracer; private BlockingCollection queue = new BlockingCollection(); private IServiceTask currentTask; + private Thread workerThread; public ServiceTaskQueue(ITracer tracer) { this.tracer = tracer; - Thread worker = new Thread(() => this.RunQueue()); - worker.Name = "MaintenanceWorker"; - worker.IsBackground = true; - worker.Start(); + this.workerThread = new Thread(() => this.RunQueue()); + this.workerThread.Name = nameof(ServiceTaskQueue); + this.workerThread.IsBackground = true; + this.workerThread.Start(); } public bool TryEnqueue(IServiceTask step) { try { - lock (this.queueLock) - { - if (this.queue == null) - { - return false; - } - - this.queue.Add(step); - return true; - } + this.queue.Add(step); + return true; } catch (InvalidOperationException) { @@ -46,32 +38,33 @@ public bool TryEnqueue(IServiceTask step) public void Stop() { - lock (this.queueLock) + this.queue.CompleteAdding(); + this.currentTask?.Stop(); + } + + public void Dispose() + { + if (this.workerThread != null) { - this.queue?.CompleteAdding(); + // Wait for the working thread to finish before setting queue + // to null to ensure that the worker does not hit a null + // reference exception trying to access the queue + this.workerThread.Join(); + this.workerThread = null; } - this.currentTask?.Stop(); + if (this.queue != null) + { + this.queue.Dispose(); + this.queue = null; + } } private void RunQueue() { - while (true) + while (this.queue.TryTake(out this.currentTask, Timeout.Infinite) && + !this.queue.IsAddingCompleted) { - // We cannot take the lock here, as TryTake is blocking. - // However, this is the place to set 'this.queue' to null. - if (!this.queue.TryTake(out this.currentTask, Timeout.Infinite) - || this.queue.IsAddingCompleted) - { - lock (this.queueLock) - { - // A stop was requested - this.queue?.Dispose(); - this.queue = null; - return; - } - } - try { this.currentTask.Execute(); diff --git a/Scalar.Service/UserAndSession.cs b/Scalar.Service/UserAndSession.cs new file mode 100644 index 0000000000..22506b732c --- /dev/null +++ b/Scalar.Service/UserAndSession.cs @@ -0,0 +1,14 @@ +namespace Scalar.Service +{ + public class UserAndSession + { + public UserAndSession(string userId, int sessionId) + { + this.UserId = userId; + this.SessionId = sessionId; + } + + public string UserId { get; } + public int SessionId { get; } + } +} diff --git a/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs b/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs index a259bdc077..285dbbaadb 100644 --- a/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs +++ b/Scalar.UnitTests/Service/ServiceTaskQueueTests.cs @@ -1,13 +1,7 @@ using NUnit.Framework; -using Scalar.Common; -using Scalar.Common.FileSystem; -using Scalar.Common.Git; -using Scalar.Common.Maintenance; -using Scalar.Common.Tracing; using Scalar.Service; using Scalar.Tests.Should; using Scalar.UnitTests.Mock.Common; -using System.Collections.Generic; using System.Threading; namespace Scalar.UnitTests.Service @@ -16,99 +10,61 @@ namespace Scalar.UnitTests.Service public class ServiceTaskQueueTests { private int maxWaitTime = 500; - private ReadyFileSystem fileSystem; - private ScalarEnlistment enlistment; - private ScalarContext context; - private GitObjects gitObjects; [TestCase] public void ServiceTaskQueueHandlesTwoJobs() { - this.TestSetup(); - TestServiceTask step1 = new TestServiceTask(); TestServiceTask step2 = new TestServiceTask(); - ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); - - queue.TryEnqueue(step1); - queue.TryEnqueue(step2); + using (ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer())) + { + queue.TryEnqueue(step1); + queue.TryEnqueue(step2); - step1.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); - step2.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); + step1.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); + step2.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); - queue.Stop(); + queue.Stop(); - step1.NumberOfExecutions.ShouldEqual(1); - step2.NumberOfExecutions.ShouldEqual(1); + step1.NumberOfExecutions.ShouldEqual(1); + step2.NumberOfExecutions.ShouldEqual(1); + } } [TestCase] public void ServiceTaskQueueStopSuceedsWhenQueueIsEmpty() { - this.TestSetup(); - - ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); - - queue.Stop(); + using (ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer())) + { + queue.Stop(); - TestServiceTask step = new TestServiceTask(); - queue.TryEnqueue(step).ShouldEqual(false); + TestServiceTask step = new TestServiceTask(); + queue.TryEnqueue(step).ShouldEqual(false); + } } [TestCase] public void ServiceTaskQueueStopsJob() { - this.TestSetup(); - - ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer()); - - // This step stops the queue after the step is started, - // then checks if Stop() was called. - WatchForStopTask watchForStop = new WatchForStopTask(queue); - - queue.TryEnqueue(watchForStop); - watchForStop.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); - watchForStop.SawStopping.ShouldBeTrue(); - - // Ensure we don't start a job after the Stop() call - TestServiceTask watchForStart = new TestServiceTask(); - queue.TryEnqueue(watchForStart).ShouldBeFalse(); - - // This only ensures the event didn't happen within maxWaitTime - watchForStart.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeFalse(); - - queue.Stop(); - } - - private void TestSetup() - { - ITracer tracer = new MockTracer(); - this.enlistment = new MockScalarEnlistment(); - - // We need to have the EnlistmentRoot and GitObjectsRoot available for jobs to run - this.fileSystem = new ReadyFileSystem(new string[] + using (ServiceTaskQueue queue = new ServiceTaskQueue(new MockTracer())) { - this.enlistment.EnlistmentRoot, - this.enlistment.GitObjectsRoot - }); + // This step stops the queue after the step is started, + // then checks if Stop() was called. + WatchForStopTask watchForStop = new WatchForStopTask(queue); - this.context = new ScalarContext(tracer, this.fileSystem, this.enlistment); - this.gitObjects = new MockPhysicalGitObjects(tracer, this.fileSystem, this.enlistment, null); - } + queue.TryEnqueue(watchForStop).ShouldBeTrue(); + watchForStop.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeTrue(); + watchForStop.SawStopping.ShouldBeTrue(); - public class ReadyFileSystem : PhysicalFileSystem - { - public ReadyFileSystem(IEnumerable paths) - { - this.Paths = new HashSet(paths); - } + // Ensure we don't start a job after the Stop() call + TestServiceTask watchForStart = new TestServiceTask(); + queue.TryEnqueue(watchForStart).ShouldBeFalse(); - public HashSet Paths { get; } + // This only ensures the event didn't happen within maxWaitTime + watchForStart.EventTriggered.WaitOne(this.maxWaitTime).ShouldBeFalse(); - public override bool DirectoryExists(string path) - { - return this.Paths.Contains(path); + queue.Stop(); } } From 05ef53a047d853251635f4962b066db40883f6ab Mon Sep 17 00:00:00 2001 From: William Baker Date: Thu, 24 Oct 2019 16:31:52 -0700 Subject: [PATCH 3/4] fixup! scalar maintenance: simplify maintenance task management --- Scalar.Service/MaintenanceTaskScheduler.cs | 4 ++-- Scalar.Service/ServiceTaskQueue.cs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Scalar.Service/MaintenanceTaskScheduler.cs b/Scalar.Service/MaintenanceTaskScheduler.cs index 5d63089d2f..bf38023093 100644 --- a/Scalar.Service/MaintenanceTaskScheduler.cs +++ b/Scalar.Service/MaintenanceTaskScheduler.cs @@ -156,8 +156,8 @@ public void Execute() this.repoRegistry.RunMaintenanceTaskForRepos( this.task, - this.registeredUserStore.RegisteredUser.UserId, - this.registeredUserStore.RegisteredUser.SessionId); + registeredUser.UserId, + registeredUser.SessionId); } else { diff --git a/Scalar.Service/ServiceTaskQueue.cs b/Scalar.Service/ServiceTaskQueue.cs index b2a010b1e6..093a958893 100644 --- a/Scalar.Service/ServiceTaskQueue.cs +++ b/Scalar.Service/ServiceTaskQueue.cs @@ -15,7 +15,7 @@ public class ServiceTaskQueue : IDisposable public ServiceTaskQueue(ITracer tracer) { this.tracer = tracer; - this.workerThread = new Thread(() => this.RunQueue()); + this.workerThread = new Thread(this.ExecuteTasksOnWorkerThread); this.workerThread.Name = nameof(ServiceTaskQueue); this.workerThread.IsBackground = true; this.workerThread.Start(); @@ -60,7 +60,7 @@ public void Dispose() } } - private void RunQueue() + private void ExecuteTasksOnWorkerThread() { while (this.queue.TryTake(out this.currentTask, Timeout.Infinite) && !this.queue.IsAddingCompleted) @@ -71,10 +71,10 @@ private void RunQueue() } catch (Exception e) { - EventMetadata metadata = this.CreateEventMetadata(nameof(this.RunQueue), e); + EventMetadata metadata = this.CreateEventMetadata(nameof(this.ExecuteTasksOnWorkerThread), e); this.tracer.RelatedError( metadata: metadata, - message: $"{nameof(this.RunQueue)}: Unexpected Exception while running service tasks: {e.Message}"); + message: $"{nameof(this.ExecuteTasksOnWorkerThread)}: Unexpected Exception while running service tasks: {e.Message}"); } } } From 68a37b45ce6d1b94309d406dc6964c82da688c59 Mon Sep 17 00:00:00 2001 From: William Baker Date: Thu, 24 Oct 2019 16:56:39 -0700 Subject: [PATCH 4/4] scalar service: use an enum to for maintenance tasks Rather than passing around a string representation of the task to run, update the code to pass around an enum instead. Additionally, update legacy code that was using "step" in its name to use "task" instead. --- Scalar.Common/Maintenance/MaintenanceTasks.cs | 33 +++++++++++ Scalar.Service/IRepoRegistry.cs | 3 +- Scalar.Service/IScalarVerbRunner.cs | 4 +- Scalar.Service/MaintenanceTaskScheduler.cs | 55 ++++++++++--------- Scalar.Service/RepoRegistry.cs | 5 +- Scalar.Service/ScalarVerbRunner.Mac.cs | 6 +- Scalar.Service/ScalarVerbRunner.Windows.cs | 9 ++- .../Service/Mac/MacServiceTests.cs | 8 ++- 8 files changed, 84 insertions(+), 39 deletions(-) create mode 100644 Scalar.Common/Maintenance/MaintenanceTasks.cs diff --git a/Scalar.Common/Maintenance/MaintenanceTasks.cs b/Scalar.Common/Maintenance/MaintenanceTasks.cs new file mode 100644 index 0000000000..a5387b7638 --- /dev/null +++ b/Scalar.Common/Maintenance/MaintenanceTasks.cs @@ -0,0 +1,33 @@ +using System; + +namespace Scalar.Common.Maintenance +{ + public static class MaintenanceTasks + { + public enum Task + { + Invalid = 0, + FetchCommitsAndTrees, + LooseObjects, + PackFiles, + CommitGraph, + } + + public static string GetVerbTaskName(Task task) + { + switch (task) + { + case Task.FetchCommitsAndTrees: + return ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName; + case Task.LooseObjects: + return ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName; + case Task.PackFiles: + return ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName; + case Task.CommitGraph: + return ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName; + default: + throw new ArgumentException($"Invalid or unknown task {task.ToString()}", nameof(task)); + } + } + } +} diff --git a/Scalar.Service/IRepoRegistry.cs b/Scalar.Service/IRepoRegistry.cs index 3ec97b3086..38595863ca 100644 --- a/Scalar.Service/IRepoRegistry.cs +++ b/Scalar.Service/IRepoRegistry.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using Scalar.Common.Maintenance; namespace Scalar.Service { @@ -8,7 +9,7 @@ public interface IRepoRegistry bool TryDeactivateRepo(string repoRoot, out string errorMessage); bool TryGetActiveRepos(out List repoList, out string errorMessage); bool TryRemoveRepo(string repoRoot, out string errorMessage); - void RunMaintenanceTaskForRepos(string task, string userId, int sessionId); + void RunMaintenanceTaskForRepos(MaintenanceTasks.Task task, string userId, int sessionId); void TraceStatus(); } } diff --git a/Scalar.Service/IScalarVerbRunner.cs b/Scalar.Service/IScalarVerbRunner.cs index 769ba527dd..8d7c56d7b9 100644 --- a/Scalar.Service/IScalarVerbRunner.cs +++ b/Scalar.Service/IScalarVerbRunner.cs @@ -1,7 +1,9 @@ +using Scalar.Common.Maintenance; + namespace Scalar.Service { public interface IScalarVerbRunner { - bool CallMaintenance(string task, string repoRoot, int sessionId); + bool CallMaintenance(MaintenanceTasks.Task task, string repoRoot, int sessionId); } } diff --git a/Scalar.Service/MaintenanceTaskScheduler.cs b/Scalar.Service/MaintenanceTaskScheduler.cs index bf38023093..eeb8b93b23 100644 --- a/Scalar.Service/MaintenanceTaskScheduler.cs +++ b/Scalar.Service/MaintenanceTaskScheduler.cs @@ -1,4 +1,5 @@ using Scalar.Common; +using Scalar.Common.Maintenance; using Scalar.Common.Tracing; using System; using System.Collections.Generic; @@ -21,14 +22,14 @@ public class MaintenanceTaskScheduler : IDisposable, IRegisteredUserStore private readonly ITracer tracer; private ServiceTaskQueue taskQueue; - private List stepTimers; + private List taskTimers; public MaintenanceTaskScheduler(ITracer tracer, IRepoRegistry repoRegistry) { this.tracer = tracer; - this.stepTimers = new List(); + this.taskTimers = new List(); this.taskQueue = new ServiceTaskQueue(this.tracer); - this.ScheduleRecurringSteps(repoRegistry); + this.ScheduleRecurringTasks(repoRegistry); } public UserAndSession RegisteredUser { get; private set; } @@ -49,7 +50,7 @@ public void RegisterUser(UserAndSession user) public void Dispose() { this.taskQueue.Stop(); - foreach (Timer timer in this.stepTimers) + foreach (Timer timer in this.taskTimers) { using (ManualResetEvent timerDisposed = new ManualResetEvent(initialState: false)) { @@ -60,69 +61,69 @@ public void Dispose() this.taskQueue.Dispose(); this.taskQueue = null; - this.stepTimers = null; + this.taskTimers = null; } - private void ScheduleRecurringSteps(IRepoRegistry repoRegistry) + private void ScheduleRecurringTasks(IRepoRegistry repoRegistry) { if (ScalarEnlistment.IsUnattended(this.tracer)) { - this.tracer.RelatedInfo($"{nameof(this.ScheduleRecurringSteps)}: Skipping maintenance tasks due to running unattended"); + this.tracer.RelatedInfo($"{nameof(this.ScheduleRecurringTasks)}: Skipping maintenance tasks due to running unattended"); return; } - List tasks = new List() + List taskSchedules = new List() { - new TaskTiming( - ScalarConstants.VerbParameters.Maintenance.FetchCommitsAndTreesTaskName, + new MaintenanceSchedule( + MaintenanceTasks.Task.FetchCommitsAndTrees, dueTime: this.fetchCommitsAndTreesPeriod, period: this.fetchCommitsAndTreesPeriod), - new TaskTiming( - ScalarConstants.VerbParameters.Maintenance.LooseObjectsTaskName, + new MaintenanceSchedule( + MaintenanceTasks.Task.LooseObjects, dueTime: this.looseObjectsDueTime, period: this.looseObjectsPeriod), - new TaskTiming( - ScalarConstants.VerbParameters.Maintenance.PackFilesTaskName, + new MaintenanceSchedule( + MaintenanceTasks.Task.PackFiles, dueTime: this.packfileDueTime, period: this.packfilePeriod), - new TaskTiming( - ScalarConstants.VerbParameters.Maintenance.CommitGraphTaskName, + new MaintenanceSchedule( + MaintenanceTasks.Task.CommitGraph, dueTime: this.commitGraphDueTime, period: this.commitGraphPeriod), }; - foreach (TaskTiming task in tasks) + foreach (MaintenanceSchedule schedule in taskSchedules) { - this.stepTimers.Add(new Timer( + this.taskTimers.Add(new Timer( (state) => this.taskQueue.TryEnqueue( new MaintenanceTask( this.tracer, repoRegistry, this, - task.Name)), + schedule.Task)), state: null, - dueTime: task.DueTime, - period: task.Period)); + dueTime: schedule.DueTime, + period: schedule.Period)); } } - private class TaskTiming + private class MaintenanceSchedule { - public TaskTiming(string name, TimeSpan dueTime, TimeSpan period) + public MaintenanceSchedule(MaintenanceTasks.Task task, TimeSpan dueTime, TimeSpan period) { - this.Name = name; + this.Task = task; this.DueTime = dueTime; this.Period = period; } - public string Name { get; } + public MaintenanceTasks.Task Task { get; } public TimeSpan DueTime { get; } public TimeSpan Period { get; } } private class MaintenanceTask : IServiceTask { - private readonly string task; + private readonly MaintenanceTasks.Task task; private readonly IRepoRegistry repoRegistry; private readonly ITracer tracer; private readonly IRegisteredUserStore registeredUserStore; @@ -131,7 +132,7 @@ public MaintenanceTask( ITracer tracer, IRepoRegistry repoRegistry, IRegisteredUserStore registeredUserStore, - string task) + MaintenanceTasks.Task task) { this.tracer = tracer; this.repoRegistry = repoRegistry; diff --git a/Scalar.Service/RepoRegistry.cs b/Scalar.Service/RepoRegistry.cs index 1bef51213c..ac21f8c905 100644 --- a/Scalar.Service/RepoRegistry.cs +++ b/Scalar.Service/RepoRegistry.cs @@ -1,4 +1,5 @@ using Scalar.Common.FileSystem; +using Scalar.Common.Maintenance; using Scalar.Common.Tracing; using System; using System.Collections.Generic; @@ -155,10 +156,10 @@ public bool TryRemoveRepo(string repoRoot, out string errorMessage) return false; } - public void RunMaintenanceTaskForRepos(string task, string userId, int sessionId) + public void RunMaintenanceTaskForRepos(MaintenanceTasks.Task task, string userId, int sessionId) { EventMetadata metadata = CreateEventMetadata(); - metadata.Add(nameof(task), task); + metadata.Add(nameof(task), MaintenanceTasks.GetVerbTaskName(task)); metadata.Add(nameof(userId), userId); metadata.Add(nameof(sessionId), sessionId); diff --git a/Scalar.Service/ScalarVerbRunner.Mac.cs b/Scalar.Service/ScalarVerbRunner.Mac.cs index 1ec9127ba3..ee8faf4bf2 100644 --- a/Scalar.Service/ScalarVerbRunner.Mac.cs +++ b/Scalar.Service/ScalarVerbRunner.Mac.cs @@ -1,4 +1,5 @@ using Scalar.Common; +using Scalar.Common.Maintenance; using Scalar.Common.Tracing; using System.Diagnostics; using System.IO; @@ -28,10 +29,11 @@ public ScalarVerbRunner(ITracer tracer, ScalarProcessLauncher processLauncher = this.internalVerbJson = internalParams.ToJson(); } - public bool CallMaintenance(string task, string repoRoot, int sessionId) + public bool CallMaintenance(MaintenanceTasks.Task task, string repoRoot, int sessionId) { + string taskVerbName = MaintenanceTasks.GetVerbTaskName(task); string arguments = - $"asuser {sessionId} {this.scalarBinPath} maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"; + $"asuser {sessionId} {this.scalarBinPath} maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {taskVerbName} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"; ProcessResult result = this.processLauncher.LaunchProcess(ExecutablePath, arguments, repoRoot); if (result.ExitCode != 0) diff --git a/Scalar.Service/ScalarVerbRunner.Windows.cs b/Scalar.Service/ScalarVerbRunner.Windows.cs index cf68fe1047..caec693836 100644 --- a/Scalar.Service/ScalarVerbRunner.Windows.cs +++ b/Scalar.Service/ScalarVerbRunner.Windows.cs @@ -1,4 +1,5 @@ using Scalar.Common; +using Scalar.Common.Maintenance; using Scalar.Common.Tracing; using Scalar.Platform.Windows; @@ -17,7 +18,7 @@ public ScalarVerbRunner(ITracer tracer) this.internalVerbJson = internalParams.ToJson(); } - public bool CallMaintenance(string task, string repoRoot, int sessionId) + public bool CallMaintenance(MaintenanceTasks.Task task, string repoRoot, int sessionId) { using (CurrentUser currentUser = new CurrentUser(this.tracer, sessionId)) { @@ -31,11 +32,13 @@ public bool CallMaintenance(string task, string repoRoot, int sessionId) return true; } - private bool CallScalarMaintenance(string task, string repoRoot, CurrentUser currentUser) + private bool CallScalarMaintenance(MaintenanceTasks.Task task, string repoRoot, CurrentUser currentUser) { + string taskVerbName = MaintenanceTasks.GetVerbTaskName(task); + return currentUser.RunAs( Configuration.Instance.ScalarLocation, - $"maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"); + $"maintenance \"{repoRoot}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {taskVerbName} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"); } } } diff --git a/Scalar.UnitTests/Service/Mac/MacServiceTests.cs b/Scalar.UnitTests/Service/Mac/MacServiceTests.cs index 0970b5b97c..22fb996414 100644 --- a/Scalar.UnitTests/Service/Mac/MacServiceTests.cs +++ b/Scalar.UnitTests/Service/Mac/MacServiceTests.cs @@ -1,6 +1,7 @@ using Moq; using NUnit.Framework; using Scalar.Common; +using Scalar.Common.Maintenance; using Scalar.Service; using Scalar.UnitTests.Mock.Common; using Scalar.UnitTests.Mock.FileSystem; @@ -34,7 +35,7 @@ public void RepoRegistryCallsMaintenanceVerbOnlyForRegisteredRepos() { Mock repoMounterMock = new Mock(MockBehavior.Strict); - string task = "test-task"; + MaintenanceTasks.Task task = MaintenanceTasks.Task.FetchCommitsAndTrees; repoMounterMock.Setup(mp => mp.CallMaintenance(task, ExpectedActiveRepoPath, ExpectedActiveUserId)).Returns(true); this.CreateTestRepos(ServiceDataLocation); @@ -53,11 +54,12 @@ public void RepoRegistryCallsMaintenanceVerbOnlyForRegisteredRepos() [TestCase] public void MaintenanceVerbLaunchedUsingCorrectArgs() { - string task = "test-task"; + MaintenanceTasks.Task task = MaintenanceTasks.Task.FetchCommitsAndTrees; + string taskVerbName = MaintenanceTasks.GetVerbTaskName(task); string executable = @"/bin/launchctl"; string scalarBinPath = Path.Combine(this.scalarPlatform.Constants.ScalarBinDirectoryPath, this.scalarPlatform.Constants.ScalarExecutableName); string expectedArgs = - $"asuser {ExpectedActiveUserId} {scalarBinPath} maintenance \"{ExpectedActiveRepoPath}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {new InternalVerbParameters(startedByService: true).ToJson()}"; + $"asuser {ExpectedActiveUserId} {scalarBinPath} maintenance \"{ExpectedActiveRepoPath}\" --{ScalarConstants.VerbParameters.Maintenance.Task} {taskVerbName} --{ScalarConstants.VerbParameters.InternalUseOnly} {new InternalVerbParameters(startedByService: true).ToJson()}"; Mock mountLauncherMock = new Mock(MockBehavior.Strict, this.tracer); mountLauncherMock.Setup(mp => mp.LaunchProcess(