Add support for removing old state directories on upgrade #6017

xoriole · 2021-03-01T16:15:26Z

This PR

Asks user whether to clean up old state directories on upgrade
Adds a DATA tab in settings which allows users to selectively select and delete the old state directories.

xoriole · 2021-03-02T15:29:43Z

retest this please

ichorid

When the "should remove old dirs?" dialog window is shown, if the user waits (thinks) too long and then clicks "No", the GUI never switches from the "Loading" screen. This is reproducible and is probably a race condition between the GUI receiving the "Core started" signal and the dialog window. Also, showing the dialog window before Tribler GUI finished loading falsely indicates to the user that something is wrong.
Please, only show the window after the GUI completely loaded.

Also, please squash the commits 😉

ichorid · 2021-03-02T21:09:14Z

src/tribler-gui/tribler_gui/widgets/settingspage.py

+            version_dirs_str = "\n- ".join(selected_versions)
+            versions_info = f"Versions: \n- {version_dirs_str}"
+
+            title = "Confirm delete older versions?"


Please, change these to
"Really delete old state directories?"

I would like to avoid mentioning state directories. I think version is more understandable to users than state directories.

"Version" and "state directories" are two different things. I propose asking the guys in the dev chat 😉

To me "version" means binary files of a program, and a suggestion to delete old version I'd probably interpreted as a suggestion to delete old exe/dmg files.

As an alternative to "state directories for selected version" we can probably use "data files for selected version"

ichorid · 2021-03-02T21:10:14Z

src/tribler-gui/tribler_gui/widgets/settingspage.py

+            versions_info = f"Versions: \n- {version_dirs_str}"
+
+            title = "Confirm delete older versions?"
+            message_body = f"Are you sure to remove the selected versions? " \


Better to
"Are you sure you want to remove the state directories for selected versions?"

src/tribler-gui/tribler_gui/widgets/settingspage.py

ichorid · 2021-03-02T21:31:00Z

src/tribler-gui/tribler_gui/core_manager.py

+            storage_info += f"{format_size(dir_size)} \t {relpath(old_state_dir, root_state_dir)}\n"
+
+        # Show a question to the user asking if the user wants to remove the old data.
+        title = "Delete older version?"


"Delete state directories for old versions?"

ichorid · 2021-03-02T21:38:47Z

src/tribler-gui/tribler_gui/core_manager.py

+
+        # Show a question to the user asking if the user wants to remove the old data.
+        title = "Delete older version?"
+        message_body = f"Press 'Yes' to remove data of older versions " \


"Press 'Yes' to remove state directories for older versions of Tribler and reclaim {} of storage space."
"Tribler used those directories during upgrades from previous versions. Now those directories can be safely deleted."
"If unsure, press 'No'"
"You will be able to remove those directories from the Settinds->Data page later."

This is better 👍

ichorid · 2021-03-02T21:46:36Z

src/tribler-gui/tribler_gui/core_manager.py

+
+        do_cleanup, old_version_dirs = self.should_cleanup_old_versions(root_state_dir, version_id)
+        if do_cleanup:
+            self.events_manager.remaining_connection_attempts = 1200


This could interfere with the upgrade process and cause other kinds of trouble. Instead, the process must be initiated by the main Tribler Window class after Tribler GUI loads completely.

ichorid · 2021-03-02T22:00:38Z

src/tribler-core/tribler_core/upgrade/tests/test_version_manager.py

+
+    # Case 0: If the code version and last version is the same, no disposable directories shown at startup
+    code_version = last_version  # 8.9.2
+    disposable_dirs = get_disposable_state_directories(root_state_dir, code_version, skip_last_version=True)


the output of get_disposable_state_directories should be always the same, regardless of whether the code version is there or not. Otherwise, you're mixing decision logic with analysis logic.

Examples: (code version is 8.9.2)

8.7 8.8 8.9 disposable_dirs -> [8.7]

8.7 8.8 disposable_dirs -> []

8.3 8.7 8.8 disposable_dirs -> []

ichorid · 2021-03-02T22:04:04Z

src/tribler-core/tribler_core/upgrade/tests/test_version_manager.py

+    code_version = f"{major_versions[0] + 1}.{minor_versions[0]}.{patch_versions[0]}"  # 9.0.0
+
+    # Case 1: Skip last version is True, then those two last directories will not returned as disposable dirs.
+    disposable_dirs = get_disposable_state_directories(root_state_dir, code_version, skip_last_version=True)


Again, please don't mix decision logic with analysis logic

ichorid · 2021-03-02T22:05:12Z

src/tribler-core/tribler_core/upgrade/tests/test_version_manager.py

+    assert second_last_version_dir in disposable_dirs
+
+
+def test_installed_versions_and_removal(tmpdir_factory):


Sorry, I don't get what this test is intended to test from the name 🤕

ichorid · 2021-03-02T22:07:37Z

src/tribler-core/tribler_core/upgrade/tests/test_version_manager.py

+
+    # 3. Skip a few other versions
+    skip_versions = ['7.5', '7.6']
+    installed_versions = get_installed_versions(root_state_dir, current_version=False, skip_versions=skip_versions)


Again, mixing decision logic with analytical logic

ichorid · 2021-03-02T22:15:51Z

src/tribler-core/tribler_core/upgrade/version_manager.py

+            skip_versions = [last_version]
+
+            # Get the second last version if available and add it to the dirs to save
+            second_last_version = version_history.get_last_upgradable_version(root_state_dir, last_version)


Why do this skip_versions hack instead of just forming a full list of versions and using Python slicing to select every one but the last two?

skip_versions is used to give flexibility to choose different version from Settings/Data page.

~~If only last two version are selected, it does not clean up non-version directories eg. pre 7.4 directories~~

Full list of version should be sorted separately in the order of installation based on version history, additional logic

I would expect choosing the version to delete from the Data page to be completely independent from the process listing the versions to choose from

Full list of version should be sorted separately in the order of installation based on version history, additional logic

I was not clear enough in my request, my fault. By "the full list of versions" I mean "the ordered list of versions". By "ordered" I mean "ordered by last usage time, last usage goes last".

So, the expected logic is:

Get the list of state directories from the directory listing A)

Get the ordered list of versions from the version history file (B)

Filter list B to only contain entries from A

Get B[:-2] and show it to the user. Let the user form the list C of versions to delete.

Delete every version in list C.

I'm not sure that the B[:-2] logic ("we should suggest to delete everything except two last directories") is correct. If we currently have versions:

8.3

8.4

8.5 (currently running version)

8.6

8.7
what should we delete? In my opinion, definitely not [8.3, 8.4, 8.5]. I think the correct answer is [8.3, 8.6, 8.7].

xoriole · 2021-03-03T10:37:04Z

@ichorid

When the "should remove old dirs?" dialog window is shown, if the user waits (thinks) too long and then clicks "No", the GUI never switches from the "Loading" screen. This is reproducible and is probably a race condition between the GUI receiving the "Core started" signal and the dialog window. Also, showing the dialog window before Tribler GUI finished loading falsely indicates to the user that something is wrong.
Please, only show the window after the GUI completely loaded.

It is a good point you pointed out that the GUI could get stuck in the loading screen if user waits too long since the core can timeouts by then. That can be solved by increasing the timeout or pausing it when the dialog is shown. But, showing the dialog only after fully loaded defeats the purpose of the PR to solve the issue with not sufficient storage to upgrade. #5907

Also, please squash the commits wink

Yes, was planning after the review

ichorid · 2021-03-03T11:03:27Z

It is a good point you pointed out that the GUI could get stuck in the loading screen if user waits too long since the core can timeouts by then. That can be solved by increasing the timeout or pausing it when the dialog is shown. But, showing the dialog only after fully loaded defeats the purpose of the PR to solve the issue with not sufficient storage to upgrade. #5907

Increasing the timeout is a bad idea. Pausing will work, though. The Core should not be even run before the dialog finishes.

ghost · 2021-03-03T14:13:50Z

Congratulations 🎉. DeepCode analyzed your code in 0.431 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

src/tribler-gui/tribler_gui/core_manager.py

kozlovsky · 2021-03-08T17:52:47Z

Currently, the code suggests deletion of every unknown folder within the root directory. You can see on the screenshot some folders with underscores. These are my "backups" of old versions. Also, you can see three directories for old versions of Tribler, in which the .Tribler folder itself was the data folder: sqlite, channels, and dlcheckpoints. Also, there is a folder "tunnel--1" for tunnel helpers. I don't want to remove any of them except three folders from the ancient version. So, currently, the code suggests deleting every unknown folder inside .Tribler except several known folders (for the current version of Tribler and two previous versions). Probably instead it should only suggest deletion of known folders of previous versions. That is, instead of searching the root directory for all folders, it should construct folder names that are candidates for deletion from the version history.
I think it is better to swap buttons in the dialog to have the default (the safest) button on the right.
As choosing "Yes" lead to data deletion, it is probably better to ask for a second confirmation, like "Press Continue if you really want to delete all these old directories".

Regarding the code itself, it looks good enough for me. I think the code logic may be significantly simplified by introducing TriblerVersion class which is loaded from the version history and combines version name, version number, version directory, and some methods like directory_exists and delete. But this refactoring can be done in a separate PR.

kozlovsky · 2021-03-15T09:36:57Z

retest this please

kozlovsky · 2021-03-15T09:40:40Z

retest this please

kozlovsky · 2021-03-15T09:53:23Z

Retest this please

sonarcloud · 2021-03-15T22:39:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

xoriole requested a review from ichorid March 2, 2021 17:37

xoriole marked this pull request as ready for review March 2, 2021 17:37

ichorid suggested changes Mar 2, 2021

View reviewed changes

ichorid reviewed Mar 2, 2021

View reviewed changes

ichorid suggested changes Mar 2, 2021

View reviewed changes

xoriole marked this pull request as draft March 3, 2021 14:14

xoriole commented Mar 3, 2021

View reviewed changes

src/tribler-gui/tribler_gui/core_manager.py Show resolved Hide resolved

xoriole assigned kozlovsky Mar 10, 2021

kozlovsky force-pushed the fix/version-upgrade branch from 1c68e6d to 98c72e7 Compare March 15, 2021 09:40

kozlovsky requested review from ichorid and kozlovsky March 15, 2021 10:59

ichorid previously approved these changes Mar 15, 2021

View reviewed changes

kozlovsky dismissed ichorid’s stale review via 27b0f20 March 15, 2021 22:15

kozlovsky force-pushed the fix/version-upgrade branch 2 times, most recently from 23b30a3 to 84c0a64 Compare March 15, 2021 22:34

xoriole added 6 commits March 15, 2021 23:36

Update version to 7.9.0-GIT

ce0431b

Add popup to remove old state dirs on startup

5f4cf18

Add data tab in settings

adcec59

Add test of disposable version dir

adbc1c3

Pylint & flake8 fixes

aa5fe07

Fix checkbox deletion issue

406dee3

xoriole and others added 14 commits March 15, 2021 23:36

Refactor get_disposable_state_directories function

144637e

Fix pylint issues

03e89b9

Ignore pylint many locals issue in test

93f9b26

Skip old version cleanup in GUI tests

6592ee5

Fix issue with current version checkbox refresh

6cdf965

Update version manager tests

f5238a6

Add additional disposable dir test

b2c3bd9

Connect events manager after cleanup is done

6d2b09d

Update dialog asking to delete old state directories

70c1bac

Move version_manager.py from tribler_core to tribler_common

4c674b3

Refactor version_manager.py, add TriblerVersion class

c97d450

Make linter satisfied

517dbf6

Make code Python 3.7-compatible

2ff6838

Minor bugfixes, better test coverage

85ec94b

kozlovsky force-pushed the fix/version-upgrade branch from 84c0a64 to 85ec94b Compare March 15, 2021 22:37

kozlovsky marked this pull request as ready for review March 15, 2021 22:43

kozlovsky approved these changes Mar 15, 2021

View reviewed changes

kozlovsky merged commit 28e1bce into Tribler:main Mar 16, 2021

xoriole deleted the fix/version-upgrade branch August 4, 2021 08:48

drew2a mentioned this pull request May 16, 2023

Upgrader: Decrease Disposable Versions count #7424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for removing old state directories on upgrade #6017

Add support for removing old state directories on upgrade #6017

xoriole commented Mar 1, 2021

xoriole commented Mar 2, 2021

ichorid left a comment

ichorid Mar 2, 2021

xoriole Mar 3, 2021

ichorid Mar 3, 2021

kozlovsky Mar 4, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

xoriole Mar 3, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

ichorid Mar 2, 2021

xoriole Mar 3, 2021 •

edited

Loading

ichorid Mar 3, 2021

kozlovsky Mar 4, 2021

xoriole commented Mar 3, 2021

ichorid commented Mar 3, 2021

ghost commented Mar 3, 2021 •

edited by ghost

Loading

kozlovsky commented Mar 8, 2021 •

edited

Loading

kozlovsky commented Mar 15, 2021

kozlovsky commented Mar 15, 2021

kozlovsky commented Mar 15, 2021

sonarcloud bot commented Mar 15, 2021

		assert second_last_version_dir in disposable_dirs


		def test_installed_versions_and_removal(tmpdir_factory):

Add support for removing old state directories on upgrade #6017

Add support for removing old state directories on upgrade #6017

Conversation

xoriole commented Mar 1, 2021

xoriole commented Mar 2, 2021

ichorid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoriole Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoriole commented Mar 3, 2021

ichorid commented Mar 3, 2021

ghost commented Mar 3, 2021 • edited by ghost Loading

👉 View analysis in DeepCode’s Dashboard | Configure the bot

kozlovsky commented Mar 8, 2021 • edited Loading

kozlovsky commented Mar 15, 2021

kozlovsky commented Mar 15, 2021

kozlovsky commented Mar 15, 2021

sonarcloud bot commented Mar 15, 2021

xoriole Mar 3, 2021 •

edited

Loading

ghost commented Mar 3, 2021 •

edited by ghost

Loading

kozlovsky commented Mar 8, 2021 •

edited

Loading