Refactor workload set and workload install/update logic #39991

dsplaisted · 2024-04-03T15:08:52Z

Fix file not found error updating in workload set mode if no workload sets for feature band were found
Add workload set installation records when installing workload sets in file based installer
Refactor workload set install / update logic to pass around workload set version instead of using a file in the advertising manifest path to pass the version
Refactor workload update and install commands to share update logic
Refactor workload set installation logic in file based installer to share logic with workload manifest installation
Update logic mapping from workload set version to feature band, to resolve ambiguity that previously existed. Now the preview part of the workload set version will be used for the feature band if the workload set version doesn't have a fourth version number part.
Unify logic to read workload set json files into WorkloadSet.FromWorkloadSetFolder

joeloff · 2024-04-04T15:53:23Z

src/Cli/Microsoft.DotNet.Cli.Utils/PathUtility.cs

            {
                Directory.Delete(dir);
+                directoriesDeleted++;


Is there a concern that we'll get stuck in this loop?

Not really—we'll start out a finite number of directories deep, and that provides an upper limit to how many times it'll run.

I wanted to use this as part of cleaning up the workload install records, but I didn't want it to delete directories further than a certain level. It probably would have been OK to delete empty directories and they would have been recreated if needed, but it felt safer to me to limit it.

Forgind

I'm most of the way through this but want to give it another look later. Appreciate the refactoring!

Forgind · 2024-04-04T20:46:33Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+                        _workloadInstaller.RemoveManifestsFromInstallState(_sdkFeatureBand);
+                    }
+
+                    _workloadInstaller.AdjustWorkloadSetInInstallState(_sdkFeatureBand, string.IsNullOrWhiteSpace(_workloadSetVersion) ? null : _workloadSetVersion);


This means we're removing the workload set from the install state if it had been pinned previously even if we're doing an install. As in your todo, we need to leave it if the workload set version is pinned in the install state for install. I did that in my PR here, though it might be cleaner to handle this with the 'resolvedWorkloadSetVersion' you made.

resolvedWorkloadSetVersion wasn't what I'd thought it was, so ignore that part.

We should also be sure we handle global.json properly.

src/Tests/dotnet-MsiInstallation.Tests/WorkloadSetTests.cs

Forgind · 2024-04-04T21:07:57Z

src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs

-                    }
+                    //  All installation records for the manifest were removed, so we can delete the manifest
+                    _reporter.WriteLine(string.Format(LocalizableStrings.DeletingWorkloadManifest, manifestId, $"{manifestVersion}/{manifestFeatureBand}"));
+                    var manifestPath = Path.Combine(GetManifestInstallDirForFeatureBand(manifestFeatureBand.ToString()), manifestId.ToString(), manifestVersion.ToString());


Why don't we have to delete the install records anymore? Wouldn't leaving old ones behind mean we wouldn't be able to delete it if it's reinstalled later?

Forgind · 2024-04-04T21:13:06Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

                    {
-                        manifestsToUpdate = InstallWorkloadSet(context);
+                        var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
+                        if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && File.Exists(installStateFilePath) && InstallStateContents.FromString(File.ReadAllText(installStateFilePath)).Manifests is not null)


Suggested change

if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && File.Exists(installStateFilePath) && InstallStateContents.FromString(File.ReadAllText(installStateFilePath)).Manifests is not null)

if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && InstallStateContents.FromPath(installStateFilePath)?.Manifests is not null)

This is what FromPath is for 🙂

Forgind · 2024-04-04T21:17:55Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

-                    }
+                        if (_skipManifestUpdate)
+                        {
+                            _workloadInstaller.InstallWorkloads(workloadIds, _sdkFeatureBand, context, offlineCache);


I think this raises an interesting question:
If you ask for a workload set (via global.json, say) and you also specify to skip manifest updates, what should our response be? I don't think "install the latest version" makes sense because that would violate our workload set, but we can't update other manifests, so there's no point in aligning with the workload set. Perhaps that should be an error?

Forgind · 2024-04-04T21:19:43Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

+                        }
+
+                        //  Write workload installation records
+                        var recordRepo = _workloadInstaller.GetWorkloadInstallationRecordRepository();


We don't need to do this part before we install more workloads? Wouldn't this just be empty?

Forgind · 2024-04-04T21:20:55Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

+                                }
+                            });
+
+                        TryRunGarbageCollection(_workloadInstaller, Reporter, Verbosity, workloadSetVersion => _workloadResolverFactory.CreateForWorkloadSet(_dotnetPath, _sdkVersion.ToString(), _userProfileDir, workloadSetVersion), offlineCache);


I think we need to refresh workload manifests before we run GC.

Forgind · 2024-04-04T21:23:43Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+            string[] sections = setVersion.Split(new char[] { '-', '+' }, 2);
+            string versionCore = sections[0];
+            string preReleaseOrBuild = sections.Length > 1 ? sections[1] : null;
+


Why are you doing all this manually instead of using NugetVersion's parsing? I'm having a little trouble figuring out what exactly you're doing here.

/cc: @joeloff

Forgind · 2024-04-04T21:25:26Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

@@ -194,6 +214,17 @@ public static void AdvertiseWorkloadUpdates()
            }
        }

+        public string GetAdvertisedWorkloadSetVersion()
+        {
+            var advertisedPath = GetAdvertisingManifestPath(_sdkFeatureBand, new ManifestId(WorkloadSetManifestId));


There's only ever one advertising manifest for workload sets? I don't remember for sure.

Forgind · 2024-04-04T21:32:25Z

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/ManifestVersionUpdate.cs

@@ -3,6 +3,7 @@

 namespace Microsoft.NET.Sdk.WorkloadManifestReader
 {
+    //  TODO: Do we need this class, or the existing version information anymore now that workload manifest are side by side?


The only time I can think of that it would be helpful is if we're rolling back to a pinned version of something, but I don't think we handle that 100% correctly now, so we can probably dispense with it.

Forgind · 2024-04-05T21:05:09Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+            {
+                manifestsToUpdate = InstallWorkloadSet(context, resolvedWorkloadSetVersion);
+            }
+            else


You could probably make this whole thing a ternary; I only didn't because the return value was different for workload sets.

Forgind · 2024-04-05T21:06:23Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+                        _workloadInstaller.RemoveManifestsFromInstallState(_sdkFeatureBand);
+                    }
+
+                    _workloadInstaller.AdjustWorkloadSetInInstallState(_sdkFeatureBand, string.IsNullOrWhiteSpace(_workloadSetVersion) ? null : _workloadSetVersion);


resolvedWorkloadSetVersion wasn't what I'd thought it was, so ignore that part.

Forgind · 2024-04-05T21:06:45Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+                        _workloadInstaller.RemoveManifestsFromInstallState(_sdkFeatureBand);
+                    }
+
+                    _workloadInstaller.AdjustWorkloadSetInInstallState(_sdkFeatureBand, string.IsNullOrWhiteSpace(_workloadSetVersion) ? null : _workloadSetVersion);


We should also be sure we handle global.json properly.

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadSet.cs

Forgind · 2024-04-05T21:22:29Z

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadSet.cs

+                workloadSet.IsBaselineWorkloadSet = true;
+            }
+
+            workloadSet.Version = workloadSetVersion;


It feels a little weird to assert that this random workload set we found is the version specified in the argument to this method...maybe move this to be next to where we actually figured out what workload set version we wanted? Note that I think if we request version x from NuGet, we might get something else that's close.

…'t found

…oadCommand

…all state

Forgind

I haven't reviewed tests yet

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

Forgind · 2024-06-26T17:06:04Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

                    {
-                        RunInNewTransaction(context =>
+                        var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
+                        if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&


Shouldn't there be more conditions here for other reasons to require a manifest update?

clarification: consider adding string.isnull checks for --version inputs and global.json version inputs to be complete

Ping on this one

Are you saying that you think we should skip manifest update if --sdk-version is specified or if there's a workload version in global.json? In those cases we do want to call the UpdateWorkloadManifests method. That method is responsible for respecting the version from --sdk-version or global.json. If a version was specified in either of those ways, then it will install those versions.

So I think the logic is correct, let me know if there's a case I'm missing.

we should skip manifest update if --sdk-version is specified or if there's a workload version in global.json

The opposite: I think we should never skip manifest updates if either of those is specified. This if statement currently says that if rollback is not specified, and there is something pinned in the install state file, then we do skip manifest updates, which is wrong.

Suggested change

if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&

if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&

!SpecifiedWorkloadSetVersionOnCommandLine &&

!SpecifiedWorkloadSetVersionInGlobalJson &&

The explanation as I understand it is that forgind is saying that if we already have an install state, this logic will skip workload updates even if you have a global.json or --version specified (which could be different than what is in the install state). So I think you are agreeing with each other. Daniel, I think you're implying that UpdateWorkloadManifests would end up getting called for those cases which I think is true the first time a customer does it but any subsequent times after having an install state, I think forgind may be right unless I'm missing a code path that calls it elsewhere.

Thanks, I think I get it now. I will try to add a test to validate the behavior and then update the logic. I'm not sure how quickly I'll be able to do that though.

Should be fixed now

Forgind · 2024-06-26T17:11:43Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

+
+                    RunInNewTransaction(context =>
+                    {
+                       if (!_skipManifestUpdate)


I haven't figured out why yet, but skipping UpdateWorkloadManifests here makes me uncomfortable.

…tory copy

…ors for combinations with skip manifest update

Forgind

I still haven't gotten to tests yet, but I think the code looks good other than these 6. (I mostly just care about the fourth and sixth.)

src/Cli/dotnet/commands/dotnet-workload/install/LocalizableStrings.resx

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadGarbageCollector.cs

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadSet.cs

Forgind · 2024-06-27T21:24:10Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

                    {
-                        RunInNewTransaction(context =>
+                        var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
+                        if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&


Ping on this one

Forgind · 2024-06-27T22:10:35Z

src/Tests/dotnet-MsiInstallation.Tests/WorkloadSetTests.cs

+            throw new NotImplementedException();
+        }
+
+        [Fact]


Should we have a test that tries to install from another feature band?

I ended up testing this incidentally, as I switched to using 8.0.302 instead of an 8.0.300-preview.0 build. That's how I found the issue with cross-band workload set GC.

Some of the VM tests are now failing though because of the band mismatch (because when you do a workload set update on an 8.0.300 band SDK, it won't install 8.0.300-preview.0 workload sets, which is what the tests are expecting).

I will probably try to sort through that and get all the tests working separately from this PR.

I was just thinking perhaps we could have another test that just throws a NotImplementedException (that we'll later make real) to explicitly do cross band install/update, but if it's covered decently well by the 8.0.300-preview tests, that works for me.

…l.json or command-line specified workload set version

Forgind

Thanks! Looks good to me! 🚢

Forgind · 2024-07-01T15:25:52Z

src/Tests/dotnet-MsiInstallation.Tests/WorkloadSetTests.cs

@@ -294,6 +293,52 @@ public void InstallWithVersionAndSkipManifestUpdate()
                .And.HaveStdErrContaining("--sdk-version");
        }

+        [Fact]
+        public void InstallWithVersionWhenPinned()


This is the perfect test for testing this change. Simple, elegant, and it works. It'd be nice to make it run in CI, but this should be fine.

marcpopMSFT · 2024-07-01T18:26:38Z

Merging per offline discussions as this is now signed off. We'll get it merged into VS main for testing.

ghost added Area-Workloads untriaged Request triage from a team member labels Apr 3, 2024

dsplaisted force-pushed the refactor-workload-sets branch from 68f2114 to 43f7012 Compare April 4, 2024 03:56

build-analysis bot mentioned this pull request Apr 4, 2024

dotnet-watch tests failing #40006

Open

dsplaisted force-pushed the refactor-workload-sets branch from 43f7012 to 58bf7f6 Compare April 4, 2024 11:34

dsplaisted changed the title ~~Refactor workload sets~~ Refactor workload set and workload install/update logic Apr 4, 2024

dsplaisted marked this pull request as ready for review April 4, 2024 12:28

dsplaisted requested review from joeloff, Forgind and a team April 4, 2024 12:38

joeloff reviewed Apr 4, 2024

View reviewed changes

Forgind reviewed Apr 4, 2024

View reviewed changes

Forgind reviewed Apr 5, 2024

View reviewed changes

dsplaisted force-pushed the refactor-workload-sets branch from 58bf7f6 to 7099ad0 Compare May 14, 2024 16:40

dsplaisted requested a review from a team as a code owner May 14, 2024 16:40

dsplaisted changed the base branch from release/8.0.3xx to release/8.0.4xx May 14, 2024 17:10

dsplaisted force-pushed the refactor-workload-sets branch from 599a15a to 2438d99 Compare May 21, 2024 13:44

dsplaisted force-pushed the refactor-workload-sets branch from 8a4f1db to d6f4293 Compare May 28, 2024 22:38

marcpopMSFT mentioned this pull request Jun 5, 2024

Support for new Workload Set Versions #35455

Open

55 tasks

dsplaisted force-pushed the refactor-workload-sets branch from d5658cd to b5a26b2 Compare June 11, 2024 21:07

dsplaisted force-pushed the refactor-workload-sets branch from c30f7b4 to ce07ca3 Compare June 25, 2024 17:31

dsplaisted added 9 commits June 25, 2024 13:55

Refactor workload update and install commands

513a744

Handle different workload update / install scenarios

a571bba

Add Aspire test

c10f8df

Roll back install state changes on workload install / update failures

5c6bf09

Fix workload set version prerelease handling

b2ed34c

Move WorkloadSetVersionToWorkloadSetPackageVersion to WorkloadSet class

80557a6

Support loading workload sets from different feature bands

45d2e5b

Fix issue where on some machines, VM snapshots couldn't be found

22452a2

Fix some tests not to skip manifest updates when installing workloads

e47104b

dsplaisted added 7 commits June 25, 2024 13:55

Don't try to roll back installation of workload set package that wasn…

e2b177f

…'t found

Don't update pinned workload set on install, refactor InstallingWorkl…

d316d96

…oadCommand

Fix some cases of pinning / unpinning workload version in global inst…

768ec44

…all state

Add message for workload set installation failure

42019de

Remove unused isRollback parameter

1d87f96

Resolve TODOs in WorkloadInstallCommand

515544c

Refactor ManifestVersionUpdate

4bb9d50

dsplaisted force-pushed the refactor-workload-sets branch from ce07ca3 to 4bb9d50 Compare June 25, 2024 17:57

Forgind reviewed Jun 26, 2024

View reviewed changes

Fix tests

49a9b98

Forgind mentioned this pull request Jun 26, 2024

Error condition fix Fixes #40297 #41741

Closed

dsplaisted added 4 commits June 26, 2024 17:24

Apply code review feedback

0742587

Fix tests

e6e20ad

Add tests for --skip-manifest-update, switch to robocopy for VM direc…

708f383

…tory copy

Fix issue garbage collecting cross-feature band workload set, add err…

904eb64

…ors for combinations with skip manifest update

Forgind reviewed Jun 27, 2024

View reviewed changes

dsplaisted added 2 commits June 27, 2024 23:14

Apply code review feedback and fix test

62c9743

Fix combinations of workload install, global install state, and globa…

979ac00

…l.json or command-line specified workload set version

Forgind approved these changes Jul 1, 2024

View reviewed changes

marcpopMSFT merged commit 4f90ed8 into dotnet:release/8.0.4xx Jul 1, 2024
17 checks passed

martincostello mentioned this pull request Jul 17, 2024

[9.0-preview.7] Microsoft.DotNet.MSBuildWorkloadSdkResolver fails with dotnet workload or build on Windows #42211

Open

	if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && File.Exists(installStateFilePath) && InstallStateContents.FromString(File.ReadAllText(installStateFilePath)).Manifests is not null)
	if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && InstallStateContents.FromPath(installStateFilePath)?.Manifests is not null)

Refactor workload set and workload install/update logic #39991

Refactor workload set and workload install/update logic #39991

Uh oh!

Conversation

dsplaisted commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Forgind Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcpopMSFT Jun 28, 2024 • edited by Forgind Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dsplaisted commented Apr 3, 2024 •

edited

Loading

Forgind Jun 26, 2024 •

edited

Loading

marcpopMSFT Jun 28, 2024 •

edited by Forgind

Loading

Forgind left a comment •

edited

Loading