Add `resume` command and support saving the argument state. #3508

ryfu-msft · 2023-08-07T18:10:01Z

Related to:
#3165

Changes:

Add a new Resume command that takes in a --resume-id; -g argument that points to a resume state.
Created a Checkpoint Index that creates/interacts with the following tables:

CheckpointMetadataTable (Responsible for tracking important information about a resume i.e. command, args, clientVersion)
CheckpointContextTable (Records the checkpoint and all relevant context data. Will be extended in a future PR to handle more complex context data)

The resume command will create a brand new context and initialize the CheckpointManager with the provided --resume-id argument. The CheckpointManager will be utilized to interface with the checkpointIndex and retrieve information for calling the correct command.
Added a Resume function that an individual command can override if they have command-specific steps for resuming.

Tests:
Add unit tests to verify the following scenarios:

Invalid resume id
Guid resume state does not exist
Resume state is empty and does not have any values.
Successful install should clean up index.
Failed install should not clean up index.

Added E2E tests to verify:

Successful install cleans up checkpoint index after completion.
Failed install leaves behind a checkpoint save file that can be called using the resume command to replicate the same outcome.

Microsoft Reviewers: codeflow:open?pullrequest=#3508

JohnMcPMS

We should probably talk again.

src/AppInstallerCLICore/Argument.cpp

src/AppInstallerCLICore/CheckpointManager.cpp

src/AppInstallerCLICore/Commands/InstallCommand.cpp

Trenly

Will be extended in a future PR to handle more complex context data

I presume this relates to sub-contexts, multi-installs, etc?

src/AppInstallerCLICore/Argument.cpp

Trenly · 2023-08-07T18:43:25Z

src/AppInstallerCLICore/Argument.cpp

@@ -345,6 +349,8 @@ namespace AppInstaller::CLI
            return Argument{ type, Resource::String::DownloadDirectoryArgumentDescription, ArgumentType::Standard, Argument::Visibility::Help, false };
        case Args::Type::InstallerType:
            return Argument{ type, Resource::String::InstallerTypeArgumentDescription, ArgumentType::Standard, Argument::Visibility::Help, false };
+        case Args::Type::ResumeGuid:
+            return Argument{ type, Resource::String::ResumeGuidArgumentDescription, ArgumentType::Standard, true };


I'd like to understand the design of this feature a bit more; What was the reason for making this a standard argument and not a positional argument? What happens if the argument is not provided?

src/AppInstallerCLICore/Command.cpp

Trenly · 2023-08-07T19:01:32Z

src/AppInstallerCLICore/Commands/ResumeCommand.cpp

+
+        checkpointManager.Checkpoint(resumeContext, Execution::CheckpointFlag::CommandArguments);
+
+        commandToResume->Execute(resumeContext);


Because the command to be resumed is actually calling the command itself, I presume this already handles a change in state on the machine? i.e - User started a command with experimental feature for dependencies enabled, first dependency spawned a reboot, upon reboot, dependencies experimental feature was magically disabled by some script that ran during the reboot, and then the winget resume happens; Would it fail due to a change in state? Proceed but just not install further dependencies? What if an argument was used originally that was behind an experimental feature and then was disabled? I think that would cause it to error out and show the "need to enable experimental feature for this", but would that also remove the resume from the checkpoint index?

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

src/AppInstallerCommonCore/Public/AppInstallerRuntime.h

Trenly · 2023-08-07T19:27:47Z

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointArgumentsTable.cpp

+    using namespace SQLite;
+    using namespace std::string_view_literals;
+
+    static constexpr std::string_view s_CheckpointArgumentsTable_Table_Name = "CheckpointArguments"sv;


Instead of having a column for each argument type, wouldn't it make more sense to have an ArgumentName table which associates a unique numerical ID to each argument type, then use a many-to-one architecture with dynamic typing for storing the arguments? It would reduce the amount of space needed in memory to not have to have empty columns for every argument, especially when arguments aren't supported by certain commands

microsoft-github-policy-service · 2023-08-14T23:19:36Z

Hello @ryfu-msft,

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Template: msftbot/noRecentActivity

ryfu-msft · 2023-08-15T20:58:48Z

/azp run

.github/actions/spelling/allow.txt

JohnMcPMS · 2023-09-14T17:38:23Z

src/AppInstallerCLICore/Checkpoint.h

+        }
+
+        // Sets a single value for the a data type.
+        void Set(T dataType, std::string value)


I think we should force all fields to have a name considered. The caller can still use empty string as the field name, although I would definitely make a comment on that PR.

src/AppInstallerCLICore/Checkpoint.h

JohnMcPMS · 2023-09-14T17:42:13Z

src/AppInstallerCLICore/Checkpoint.h

+        }
+
+        // Gets a single value for a data type.
+        std::string Get(T dataType)


Field naming comment from above applies here as well. I'm assuming that this is due to using unique enum values per field in the automatic data case. I think it better to have those pass in empty strings as they are special cases rather than expose the standard use cases to "should I name the field or not?".

JohnMcPMS · 2023-09-14T17:53:48Z

src/AppInstallerCLICore/Checkpoint.h

+
+    // A representation of a row in the Checkpoint table. 
+    template <typename T>
+    struct Checkpoint


This should be in the repository core, created by the database object. It can have a templated function to retrieve one.

Moved to repository core

src/AppInstallerCLICore/Commands/ResumeCommand.cpp

JohnMcPMS · 2023-09-14T20:45:36Z

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointTable.cpp

+
+        StatementBuilder createTableBuilder;
+        createTableBuilder.CreateTable(s_CheckpointTable_Table_Name).BeginColumns();
+        createTableBuilder.Column(ColumnBuilder(s_CheckpointTable_Name_Column, Type::Text).Unique());


Any time that you are going to use the row id as a foreign key in another table, you must specify it explicitly as a column or it can be changed on you when the table is rebalanced. See the index code for the way that you do it (it has to be one of a few specific names and with specific properties).

Removed Guid and just made the resume id a string. Also removed the argument validation so that it simply checks if we can find the checkpoint database with the provided resume id string.

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointTable.cpp

JohnMcPMS · 2023-09-14T20:55:21Z

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp

+        createTableBuilder.Column(ColumnBuilder(s_CheckpointDataTable_ContextData_Column, Type::Int));
+        createTableBuilder.Column(ColumnBuilder(s_CheckpointDataTable_Name_Column, Type::Text));
+        createTableBuilder.Column(ColumnBuilder(s_CheckpointDataTable_Value_Column, Type::Text));
+        createTableBuilder.Column(ColumnBuilder(s_CheckpointDataTable_Index_Column, Type::Int));


Table should probably have a primary key on { checkpoint, data, name, index }. Also, I don't think you want to allow nulls in any of the columns except for value, and then I think it would be better to have the design be that you simply don't insert the value rather than putting in a null one.

Changed to suggested

… resilience

ryfu-msft · 2023-09-21T16:32:46Z

/azp run

azure-pipelines · 2023-09-21T16:32:57Z

Azure Pipelines successfully started running 1 pipeline(s).

ryfu-msft · 2023-09-25T14:31:41Z

/azp run

azure-pipelines · 2023-09-25T14:31:52Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS · 2023-10-09T17:07:10Z

src/AppInstallerCLICore/CheckpointManager.cpp

+            }
+            else
+            {
+                THROW_HR_IF(ERROR_CANNOT_MAKE, !std::filesystem::is_directory(checkpointsDirectory));


Suggested change

THROW_HR_IF(ERROR_CANNOT_MAKE, !std::filesystem::is_directory(checkpointsDirectory));

THROW_HR_IF(MAKE_HRESULT_FROM_WIN32(ERROR_CANNOT_MAKE), !std::filesystem::is_directory(checkpointsDirectory));

JohnMcPMS · 2023-10-09T17:07:32Z

src/AppInstallerCLICore/CheckpointManager.cpp

+
+    std::filesystem::path CheckpointManager::GetCheckpointDatabasePath(const std::string_view& resumeId, bool createCheckpointDirectory)
+    {
+


nit: extre newline

JohnMcPMS · 2023-10-09T17:16:44Z

src/AppInstallerCLICore/CheckpointManager.cpp

+        auto checkpointIds = m_checkpointDatabase->GetCheckpointIds();
+
+        // Remove the last checkpoint (automatic)
+        checkpointIds.pop_back();


In GetAutomaticCheckpoint you check for empty, but not here.

JohnMcPMS · 2023-10-09T17:17:51Z

src/AppInstallerCLICore/CheckpointManager.cpp

+                }
+
+                const auto& checkpointDatabaseParentDirectory = checkpointDatabasePath.parent_path();
+                if (std::filesystem::remove(checkpointDatabaseParentDirectory, error))


I feel like I've left this comment before, but why not simply delete the directory with a recursive delete method?

JohnMcPMS · 2023-10-09T17:25:01Z

src/AppInstallerCLICore/Commands/ResumeCommand.cpp

+        AICLI_LOG(CLI, Info, << "Resuming command: " << checkpointCommand);
+        std::unique_ptr<Command> commandToResume = FindCommandToResume(checkpointCommand);
+
+        for (const auto& fieldName : automaticCheckpoint.GetFieldNames(AutomaticCheckpointData::Arguments))


For future maintainability, it would be better if this code that reads the values out were next to the code that writes the values in. Even if that means a standalone helper, I would much rather have them in the same file (and even have the functions next to each other).

JohnMcPMS · 2023-10-09T17:29:16Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -2581,4 +2581,27 @@ Please specify one of them using the --source option to proceed.</value>
  <data name="WINGET_CONFIG_ERROR_UNIT_SETTING_CONFIG_ROOT" xml:space="preserve">
    <value>A unit contains a setting that requires the config root.</value>
  </data>
+  <data name="ResumeCommandLongDescription" xml:space="preserve">
+    <value>Resumes execution of a previously saved command by passing in the unique guid identifier of the saved command. This is used to resume an executed command that may have been terminated due to a reboot.</value>


Suggested change

<value>Resumes execution of a previously saved command by passing in the unique guid identifier of the saved command. This is used to resume an executed command that may have been terminated due to a reboot.</value>

<value>Resumes execution of a previously saved command by passing in the unique identifier of the saved command. This is used to resume an executed command that may have been terminated due to a reboot.</value>

JohnMcPMS · 2023-10-09T17:30:45Z

src/AppInstallerCLICore/Command.cpp

+    void Command::Resume(Execution::Context& context) const
+    {
+        context.Reporter.Error() << Resource::String::CommandDoesNotSupportResumeMessage << std::endl;
+        THROW_HR(E_NOTIMPL);


Terminate the context rather than throwing I think...

JohnMcPMS · 2023-10-09T17:34:22Z

src/AppInstallerCommonCore/Runtime.cpp

@@ -31,6 +31,7 @@ namespace AppInstaller::Runtime
        constexpr std::string_view s_PortablePackageRoot = "WinGet"sv;
        constexpr std::string_view s_PortablePackagesDirectory = "Packages"sv;
        constexpr std::string_view s_LinksDirectory = "Links"sv;
+        constexpr std::string_view s_CheckpointsDirectory = "Checkpoints"sv;


At some point I think we will need to have something going through and cleaning out old resume context data. Doesn't need to be now.

JohnMcPMS · 2023-10-09T17:35:12Z

src/AppInstallerSharedLib/Public/AppInstallerStrings.h

@@ -266,4 +266,6 @@ namespace AppInstaller::Utility
    // Converts the given boolean value to a string.
    std::string_view ConvertBoolToString(bool value);

+    // Converts the given GUID value to a string.
+    std::string ConvertGuidToString(GUID value);


Suggested change

std::string ConvertGuidToString(GUID value);

std::string ConvertGuidToString(const GUID& value);

Ryan Fu added 12 commits July 14, 2023 15:51

save work

291ded4

it builds!

c6adb1a

save work

952a799

add checkpoint manager

2ed8d2c

retrieve args from index

166b843

save work

cbc34db

clean up index functions

4786194

resolve merge conflicts

8982105

add initial unit tests

0f5ce84

save tests

e3786aa

fix e2E test and cleanup

940599f

fix CLIcore filter

ba3bcb6

ryfu-msft requested a review from a team as a code owner August 7, 2023 18:10

resolve merge conflicts and fix spelling

dace07e

JohnMcPMS requested changes Aug 7, 2023

View reviewed changes

microsoft-github-policy-service bot assigned ryfu-msft Aug 7, 2023

microsoft-github-policy-service bot added the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 7, 2023

Trenly reviewed Aug 7, 2023

View reviewed changes

Ryan Fu added 5 commits August 9, 2023 17:24

save work

8d9a3b4

savework

e15a142

save work

22840a5

it builds

35b772f

capture context data

9f7f3f5

microsoft-github-policy-service bot added the No-Recent-Activity Issue has no recent activity label Aug 14, 2023

denelon removed Needs-Author-Feedback Issue needs attention from issue or PR author No-Recent-Activity Issue has no recent activity labels Aug 15, 2023

simplify work and update tests

e939e90

This comment has been minimized.

Sign in to view

Ryan Fu added 3 commits September 12, 2023 08:22

fix tests and fix spelling

6076097

fix path for checkpoints directory

5107a85

remove all references to index

16ac252

ryfu-msft requested a review from JohnMcPMS September 12, 2023 18:23

actually fix e2e tests

1022bc2

JohnMcPMS reviewed Sep 14, 2023

View reviewed changes

rename checkpointRecord to checkpointDatabase

e04f6bb

This comment has been minimized.

Sign in to view

save work

38d3f83

This comment has been minimized.

Sign in to view

address rest of comments

1d1250b

This comment has been minimized.

Sign in to view

ryfu-msft added 3 commits September 20, 2023 13:22

resolve merge conflicts

e57f23e

fix path

d73ef7d

Merge branch 'master' of https://github.com/ryfu-msft/winget-cli into…

3540ba0

… resilience

ryfu-msft added 2 commits September 21, 2023 11:49

fix path recursive call

ae2dda0

minor fix

a3e72c7

fix resume e2e test

c133d52

JohnMcPMS reviewed Oct 9, 2023

View reviewed changes

ryfu-msft added 3 commits October 10, 2023 11:54

respond to PR feedback

8a2da5d

fix error code

792c465

resolve merge conflicts

a8f0ced

JohnMcPMS approved these changes Oct 12, 2023

View reviewed changes

ryfu-msft merged commit 85951ae into microsoft:master Oct 12, 2023
8 checks passed

ryfu-msft deleted the resilience branch October 12, 2023 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `resume` command and support saving the argument state. #3508

Add `resume` command and support saving the argument state. #3508

ryfu-msft commented Aug 7, 2023 •

edited

Loading

JohnMcPMS left a comment

Trenly left a comment

Trenly Aug 7, 2023

Trenly Aug 7, 2023

Trenly Aug 7, 2023

microsoft-github-policy-service bot commented Aug 14, 2023

This comment has been minimized.

ryfu-msft commented Aug 15, 2023

This comment has been minimized.

JohnMcPMS Sep 14, 2023

JohnMcPMS Sep 14, 2023

JohnMcPMS Sep 14, 2023

ryfu-msft Sep 25, 2023

JohnMcPMS Sep 14, 2023

ryfu-msft Sep 25, 2023

JohnMcPMS Sep 14, 2023

ryfu-msft Sep 25, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ryfu-msft commented Sep 21, 2023

azure-pipelines bot commented Sep 21, 2023

ryfu-msft commented Sep 25, 2023

azure-pipelines bot commented Sep 25, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023

JohnMcPMS Oct 9, 2023


		checkpointManager.Checkpoint(resumeContext, Execution::CheckpointFlag::CommandArguments);

		commandToResume->Execute(resumeContext);

	THROW_HR_IF(ERROR_CANNOT_MAKE, !std::filesystem::is_directory(checkpointsDirectory));
	THROW_HR_IF(MAKE_HRESULT_FROM_WIN32(ERROR_CANNOT_MAKE), !std::filesystem::is_directory(checkpointsDirectory));


		std::filesystem::path CheckpointManager::GetCheckpointDatabasePath(const std::string_view& resumeId, bool createCheckpointDirectory)
		{

	<value>Resumes execution of a previously saved command by passing in the unique guid identifier of the saved command. This is used to resume an executed command that may have been terminated due to a reboot.</value>
	<value>Resumes execution of a previously saved command by passing in the unique identifier of the saved command. This is used to resume an executed command that may have been terminated due to a reboot.</value>

	std::string ConvertGuidToString(GUID value);
	std::string ConvertGuidToString(const GUID& value);

Add resume command and support saving the argument state. #3508

Add resume command and support saving the argument state. #3508

Conversation

ryfu-msft commented Aug 7, 2023 • edited Loading

Microsoft Reviewers: codeflow:open?pullrequest=#3508

JohnMcPMS left a comment

Choose a reason for hiding this comment

Trenly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microsoft-github-policy-service bot commented Aug 14, 2023

This comment has been minimized.

ryfu-msft commented Aug 15, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ryfu-msft commented Sep 21, 2023

azure-pipelines bot commented Sep 21, 2023

ryfu-msft commented Sep 25, 2023

azure-pipelines bot commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `resume` command and support saving the argument state. #3508

Add `resume` command and support saving the argument state. #3508

ryfu-msft commented Aug 7, 2023 •

edited

Loading