Skip to content

Commit c661390

Browse files
AnatoliBAnatoliB
authored
Don't remove the initial snapshot (#351)
* Pass removeIfEquivalentToLatest to InstallSnapshot * Don't check or remove equivalent snapshots if not required * Extract PromoteToInstalledOrRemove method * Add comments for removeIfEquivalentToLatest
1 parent 1997c6d commit c661390

7 files changed

+138
-49
lines changed

‎src/DependencyManagement/BackgroundDependencySnapshotMaintainer.cs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,17 @@ public string InstallAndPurgeSnapshots(Func<PowerShell> pwshFactory, ILogger log
7373

7474
using (var pwsh = pwshFactory())
7575
{
76-
_installer.InstallSnapshot(_dependencyManifest, nextSnapshotPath, pwsh, logger);
76+
_installer.InstallSnapshot(
77+
_dependencyManifest,
78+
nextSnapshotPath,
79+
pwsh,
80+
// If the new snapshot turns out to be equivalent to the latest one,
81+
// removing it helps us save storage space and avoid unnecessary worker restarts.
82+
// It is ok to do that during background upgrade because the current
83+
// worker already has a good enough snapshot, and nothing depends on
84+
// the new snapshot yet.
85+
removeIfEquivalentToLatest: true,
86+
logger);
7787
}
7888

7989
// Now that a new snapshot has been installed, there is a chance an old snapshot can be purged.

‎src/DependencyManagement/DependencyManager.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,18 @@ private void InstallSnapshotInForeground(PowerShell pwsh, ILogger logger)
206206
{
207207
try
208208
{
209-
_installer.InstallSnapshot(_dependenciesFromManifest, _currentSnapshotPath, pwsh, logger);
209+
_installer.InstallSnapshot(
210+
_dependenciesFromManifest,
211+
_currentSnapshotPath,
212+
pwsh,
213+
// Even if the new snapshot turns out to be equivalent to the latest one,
214+
// removing it would not be safe because the current worker already depends
215+
// on it, as it has the path to this snapshot already added to PSModulePath.
216+
// As opposed to the background upgrade case, this snapshot is *required* for
217+
// this worker to run, even though it occupies some space (until the workers
218+
// restart and the redundant snapshots are purged).
219+
removeIfEquivalentToLatest: false,
220+
logger);
210221
}
211222
catch (Exception e)
212223
{

‎src/DependencyManagement/DependencySnapshotInstaller.cs

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public void InstallSnapshot(
4040
IEnumerable<DependencyManifestEntry> dependencies,
4141
string targetPath,
4242
PowerShell pwsh,
43+
bool removeIfEquivalentToLatest,
4344
ILogger logger)
4445
{
4546
var installingPath = CreateInstallingSnapshot(targetPath);
@@ -56,30 +57,13 @@ public void InstallSnapshot(
5657
InstallModule(module, installingPath, pwsh, logger);
5758
}
5859

59-
var latestSnapshot = _storage.GetLatestInstalledSnapshot();
60-
if (latestSnapshot != null && _snapshotComparer.AreEquivalent(installingPath, latestSnapshot, logger))
60+
if (removeIfEquivalentToLatest)
6161
{
62-
logger.Log(
63-
isUserOnlyLog: false,
64-
LogLevel.Trace,
65-
string.Format(PowerShellWorkerStrings.RemovingEquivalentDependencySnapshot, installingPath, latestSnapshot));
66-
67-
// The new snapshot is not better than the latest installed snapshot,
68-
// so remove the new snapshot and update the timestamp of the latest snapshot
69-
// in order to avoid unnecessary worker restarts.
70-
_storage.RemoveSnapshot(installingPath);
71-
_storage.SetSnapshotCreationTimeToUtcNow(latestSnapshot);
62+
PromoteToInstalledOrRemove(installingPath, targetPath, logger);
7263
}
7364
else
7465
{
75-
_storage.PromoteInstallingSnapshotToInstalledAtomically(targetPath);
76-
77-
logger.Log(
78-
isUserOnlyLog: false,
79-
LogLevel.Trace,
80-
string.Format(PowerShellWorkerStrings.PromotedDependencySnapshot, installingPath, targetPath));
81-
82-
_snapshotContentLogger.LogDependencySnapshotContent(targetPath, logger);
66+
PromoteToInstalled(installingPath, targetPath, logger);
8367
}
8468
}
8569
catch (Exception e)
@@ -146,6 +130,40 @@ private void InstallModule(DependencyInfo module, string installingPath, PowerSh
146130
}
147131
}
148132

133+
private void PromoteToInstalledOrRemove(string installingPath, string installedPath, ILogger logger)
134+
{
135+
var latestSnapshot = _storage.GetLatestInstalledSnapshot();
136+
if (latestSnapshot != null && _snapshotComparer.AreEquivalent(installingPath, latestSnapshot, logger))
137+
{
138+
logger.Log(
139+
isUserOnlyLog: false,
140+
LogLevel.Trace,
141+
string.Format(PowerShellWorkerStrings.RemovingEquivalentDependencySnapshot, installingPath, latestSnapshot));
142+
143+
// The new snapshot is not better than the latest installed snapshot,
144+
// so remove the new snapshot and update the timestamp of the latest snapshot
145+
// in order to avoid unnecessary worker restarts.
146+
_storage.RemoveSnapshot(installingPath);
147+
_storage.SetSnapshotCreationTimeToUtcNow(latestSnapshot);
148+
}
149+
else
150+
{
151+
PromoteToInstalled(installingPath, installedPath, logger);
152+
}
153+
}
154+
155+
private void PromoteToInstalled(string installingPath, string installedPath, ILogger logger)
156+
{
157+
_storage.PromoteInstallingSnapshotToInstalledAtomically(installedPath);
158+
159+
logger.Log(
160+
isUserOnlyLog: false,
161+
LogLevel.Trace,
162+
string.Format(PowerShellWorkerStrings.PromotedDependencySnapshot, installingPath, installedPath));
163+
164+
_snapshotContentLogger.LogDependencySnapshotContent(installedPath, logger);
165+
}
166+
149167
/// <summary>
150168
/// Returns the string representation of the given attempt number.
151169
/// </summary>

‎src/DependencyManagement/IDependencySnapshotInstaller.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ void InstallSnapshot(
1515
IEnumerable<DependencyManifestEntry> dependencies,
1616
string targetPath,
1717
PowerShell pwsh,
18+
bool removeIfEquivalentToLatest,
1819
ILogger logger);
1920
}
2021
}

‎test/Unit/DependencyManagement/BackgroundDependencySnapshotMaintainerTests.cs

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,11 @@ public void InstallsSnapshotIfNoRecentlyInstalledSnapshotFound()
4949

5050
_mockInstaller.Setup(
5151
_ => _.InstallSnapshot(
52-
It.IsAny<DependencyManifestEntry[]>(), It.IsAny<string>(), It.IsAny<PowerShell>(), It.IsAny<ILogger>()));
52+
It.IsAny<DependencyManifestEntry[]>(),
53+
It.IsAny<string>(),
54+
It.IsAny<PowerShell>(),
55+
It.IsAny<bool>(),
56+
It.IsAny<ILogger>()));
5357

5458
using (var dummyPowerShell = PowerShell.Create())
5559
using (var maintainer = CreateMaintainerWithMocks(_minBackgroundUpgradePeriod))
@@ -62,7 +66,7 @@ public void InstallsSnapshotIfNoRecentlyInstalledSnapshotFound()
6266

6367
// ReSharper disable once AccessToDisposedClosure
6468
_mockInstaller.Verify(
65-
_ => _.InstallSnapshot(_dependencyManifest, "new snapshot path", dummyPowerShell, _mockLogger.Object),
69+
_ => _.InstallSnapshot(_dependencyManifest, "new snapshot path", dummyPowerShell, true, _mockLogger.Object),
6670
Times.Once);
6771

6872
_mockPurger.Verify(_ => _.Purge(_mockLogger.Object), Times.Exactly(2));
@@ -116,7 +120,11 @@ public void LogsWarningIfCannotInstallSnapshot()
116120

117121
_mockInstaller.Setup(
118122
_ => _.InstallSnapshot(
119-
It.IsAny<DependencyManifestEntry[]>(), It.IsAny<string>(), It.IsAny<PowerShell>(), It.IsAny<ILogger>()))
123+
It.IsAny<DependencyManifestEntry[]>(),
124+
It.IsAny<string>(),
125+
It.IsAny<PowerShell>(),
126+
It.IsAny<bool>(),
127+
It.IsAny<ILogger>()))
120128
.Throws(injectedException);
121129

122130
using (var dummyPowerShell = PowerShell.Create())

‎test/Unit/DependencyManagement/DependencyManagerTests.cs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ public void StartDependencyInstallationIfNeeded_InstallsSnapshotInForeground_Whe
137137
"NewSnapshot",
138138
// Must run on the same runspace
139139
It.Is<PowerShell>(powerShell => ReferenceEquals(firstPowerShellRunspace, powerShell)),
140+
false, // removeIfEquivalentToLatest
140141
_mockLogger.Object));
141142

142143
_mockStorage.Setup(_ => _.SnapshotExists("NewSnapshot")).Returns(false);
@@ -201,6 +202,7 @@ public void StartDependencyInstallationIfNeeded_InstallsSnapshotInForeground_Whe
201202
"NewSnapshot",
202203
// Must run on the same runspace
203204
It.Is<PowerShell>(powerShell => ReferenceEquals(firstPowerShellRunspace, powerShell)),
205+
false, // removeIfEquivalentToLatest
204206
_mockLogger.Object));
205207

206208
_mockStorage.Setup(_ => _.SnapshotExists("NewSnapshot")).Returns(false);
@@ -239,7 +241,11 @@ public void StartDependencyInstallationIfNeeded_HandlesExceptionThrownBy_Install
239241

240242
_mockInstaller.Setup(
241243
_ => _.InstallSnapshot(
242-
It.IsAny<IEnumerable<DependencyManifestEntry>>(), It.IsAny<string>(), It.IsAny<PowerShell>(), It.IsAny<ILogger>()))
244+
It.IsAny<IEnumerable<DependencyManifestEntry>>(),
245+
It.IsAny<string>(),
246+
It.IsAny<PowerShell>(),
247+
It.IsAny<bool>(),
248+
It.IsAny<ILogger>()))
243249
.Throws(injectedException);
244250

245251
_mockStorage.Setup(_ => _.SnapshotExists(dependenciesPath)).Returns(false);
@@ -264,7 +270,11 @@ private void VerifyExactlyOneSnapshotInstalled()
264270
{
265271
_mockInstaller.Verify(
266272
_ => _.InstallSnapshot(
267-
It.IsAny<IEnumerable<DependencyManifestEntry>>(), It.IsAny<string>(), It.IsAny<PowerShell>(), It.IsAny<ILogger>()),
273+
It.IsAny<IEnumerable<DependencyManifestEntry>>(),
274+
It.IsAny<string>(),
275+
It.IsAny<PowerShell>(),
276+
It.IsAny<bool>(),
277+
It.IsAny<ILogger>()),
268278
Times.Once());
269279

270280
_mockInstaller.VerifyNoOtherCalls();

0 commit comments

Comments
 (0)