improvement(setup): improve caching of local tarballs #557

juliayakovlev · 2024-01-28T11:05:38Z

If the test version is unstable (not release, maybe private branch) and installation folder exists, we want to check if this version was downloaded nd installed already in the past and was changed.
In this case the version should be downloaded and installed again. Compare hash of saved version (it is saved in the 'scylla-core-package/source.txt' file) and hash of a new package.
If it is same - skip the download. If not - remove existing folder and download again.

Tests:

https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/dtest-byo/133/ (branch-5.4)
https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/dtest-byo/134/ (master)
https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/dtest-byo/135/ (using ARTIFACT_WEB_URL)
local run

ccmlib/scylla_repository.py

fruch · 2024-01-31T08:41:50Z

ccmlib/utils/download.py

@@ -172,7 +172,8 @@ def get_url_hash(url: str) -> str:
    """

    if os.path.exists(url):  # if file/dir is local, hash based on the path
-        return hashlib.md5(url).hexdigest()
+        # Strings must be encoded before hashing
+        return hashlib.md5(url.encode('utf-8')).hexdigest()


it might worth adding a unit test for this case (we have test for downloading, but not for using local file)

tchaikov · 2024-02-01T08:23:24Z

ccmlib/common.py

+        # get hash from file
+        for line in lines:
+            if line.startswith("hash="):
+                current_hash = line.replace("hash=", "").strip()


is it possible that the source_file does not contain a line of "hash=.."? is it possible that it contains multiple lines of "hash=.."? if both answers are "no", i'd suggest put something like:

if not source_file.exists(): return "" with open(source_file, 'r') as f: for line in f: if line.startswith("hash="): return line.replace("hash=", "").strip() throw ArgumentError(f"{source_file} does not contains the hash")

less indent this way. and less tolerant for sure.

Few lines with "hash" is not possible because we replace whole file and not add the lines.

No "hash": it is possible. Due to this I added the comment:

https://github.com/scylladb/scylla-ccm/pull/557/files#:~:text=%23%20Current%20hash%20may,start%20downloading%20again

# Current hash may be None. It may be due to uncompleted downloading and installation. # Or because of it is old installation that was not saved the hash yet. # In any case the new download and installation should be performed. # For this goal we need to remove existing folder and start downloading again

It means that if hash is not in the source.txt file - version will be downloaded.

i see. then still good to return early in that case.

Why do you think we need to return and stop the test? Maybe there is the reason that I did not think?

I think @tchaikov is talking more on the style, i.e. readability of it, nothing logical you are missing here.
i.e. no need for exception, but the style suggestion still can be relevert

@tchaikov I changed it, let me know if it is more clear

thanks. indeed. i was concerned about the readability. the code was and still correct.

tchaikov

left a couple comments.

tchaikov · 2024-02-01T08:26:05Z

ccmlib/scylla_repository.py

+    if validate_by_hash and packages:
+        """
+        Validate if packages hash was changed and the new package(s) have to be downloaded 
+        """


nit, i'd suggest use a comment instead of a multi-line string for comment. as it cannot be used as doc-string.

yep, if we had a linter of any kind on this project, it would have complain about this

tchaikov · 2024-02-01T08:36:15Z

ccmlib/scylla_repository.py

+            if not (new_hash and current_hash) or new_hash != current_hash:
+                # remove version_dir


IIUC, new_hash should always be a non-empty string, right? so, probably we can simplify this a little bit. how about:

for package in zip(packages._fields, packages): # ... if current_hash is None or new_hash != current_hash: rmdirs(version_dir) version_dir = None break

Actually, I do not know, may be from some reason it will be empty. I added this validation to be sure. If it will be None (I can not let you example now), the comparison new_hash != current_hash will fail

i see. the ! and not in the condition expression makes my eyes hurt. hence this suggestion. if we can trust the 'ETag' returned by S3, then it should not be empty.

if get_url_hash always returns string, there no need to check for emptiness of it return value.

also get_installed_scylla_package_hash doesn't return None (at least not now), it always return a string, could be empty, so using is None wouldn't work as one might expect.

so current_hash and new_hash != current_hash should be suffice
if get_url_hash might return None
so current_hash and new_hash and new_hash != current_hash should do the trick.

I am not sure if we can trust. 'ETag' is not mandatory. https://docs.aws.amazon.com/AmazonS3/latest/API/API_Object.html

@fruch please, let you opinion

@tchaikov I changed it, let me know if it is more clear

fruch · 2024-02-01T08:44:10Z

tests/test_scylla_repository.py

+
+    def test_get_local_tarball_hash(self):
+        this_path = Path().resolve()
+        url_hash = get_url_hash(url=str(this_path / "tests" / "test_data" / "scylla_unified_master_2023_04_03.tar.gz"))


where is that file ? in the the source code ?

let make sure it's not a full sized release, the a minimal empty tar.gz

and yeah, CI show there no such file:

FAILED tests/test_scylla_repository.py::TestScyllaRepositoryRelease::test_get_local_tarball_hash - requests.exceptions.MissingSchema: Invalid URL '/home/runner/work/scylla-ccm/scylla-ccm/tests/test_data/scylla_unified_master_2023_04_03.tar.gz': No scheme supplied. Perhaps you meant https:///home/runner/work/scylla-ccm/scylla-ccm/tests/test_data/scylla_unified_master_2023_04_03.tar.gz?

I took is from unstable. It's size 166M

and yeah, CI show there no such file:

FAILED tests/test_scylla_repository.py::TestScyllaRepositoryRelease::test_get_local_tarball_hash - requests.exceptions.MissingSchema: Invalid URL '/home/runner/work/scylla-ccm/scylla-ccm/tests/test_data/scylla_unified_master_2023_04_03.tar.gz': No scheme supplied. Perhaps you meant https:///home/runner/work/scylla-ccm/scylla-ccm/tests/test_data/scylla_unified_master_2023_04_03.tar.gz?

Yeah, I did not added it. fixing

@fruch
Oh, it is too big. It's is allowed 100m. I'll take any tarball. It is not must to be scylla package, right?

fruch · 2024-02-01T09:32:30Z

tests/tests/test_data/scylla_unified_master_2023_04_03.tar.gz

@juliayakovlev as I've said, if we can make it even smaller, I would rather not had 24mb binary on a git repository

and if we need something at this size for some reason, that it would fetch or generated during the test

@juliayakovlev as I've said, if we can make it even smaller, I would rather not had 24mb binary on a git repository

and if we need something at this size for some reason, that it would fetch or generated during the test

replaced it with small 131K. is it ok?

it's better than 24Mb

tchaikov

much better. thanks!

If the test version is unstable (not release, maybe private branch) and installation folder exists, we want to check if this version was downloaded nd installed already in the past and was changed. In this case the version should be downloaded and installed again. Compare hash of saved version (it is saved in the 'scylla-core-package/source.txt' file) and hash of a new package. If it is same - skip the download. If not - remove existing folder and download again.

juliayakovlev · 2024-02-06T11:22:55Z

@fruch @bhalevy can you review, please?

fruch

LGTM

@juliayakovlev can you also update dtest readme, and remove the comment about this issue?

juliayakovlev requested a review from fruch January 28, 2024 11:05

juliayakovlev commented Jan 28, 2024

View reviewed changes

ccmlib/scylla_repository.py Show resolved Hide resolved

juliayakovlev commented Jan 28, 2024

View reviewed changes

ccmlib/scylla_repository.py Outdated Show resolved Hide resolved

juliayakovlev force-pushed the improve_local_tarballs_caching branch 3 times, most recently from 22be9ac to 2c32b7a Compare January 28, 2024 17:40

juliayakovlev marked this pull request as ready for review January 29, 2024 07:56

juliayakovlev requested a review from bhalevy January 29, 2024 07:56

juliayakovlev self-assigned this Jan 29, 2024

fruch requested a review from tchaikov January 31, 2024 08:26

fruch reviewed Jan 31, 2024

View reviewed changes

juliayakovlev force-pushed the improve_local_tarballs_caching branch 3 times, most recently from 2d5f7dd to 262b1ed Compare January 31, 2024 17:29

juliayakovlev requested a review from fruch January 31, 2024 17:30

juliayakovlev force-pushed the improve_local_tarballs_caching branch 2 times, most recently from 9823d09 to 64970e2 Compare February 1, 2024 08:01

tchaikov reviewed Feb 1, 2024

View reviewed changes

juliayakovlev force-pushed the improve_local_tarballs_caching branch from 64970e2 to ae4bc72 Compare February 1, 2024 08:25

tchaikov reviewed Feb 1, 2024

View reviewed changes

fruch reviewed Feb 1, 2024

View reviewed changes

juliayakovlev force-pushed the improve_local_tarballs_caching branch from ae4bc72 to 52be412 Compare February 1, 2024 09:14

fruch reviewed Feb 1, 2024

View reviewed changes

juliayakovlev force-pushed the improve_local_tarballs_caching branch 4 times, most recently from 15e000f to 35aba32 Compare February 1, 2024 12:25

tchaikov approved these changes Feb 1, 2024

View reviewed changes

juliayakovlev force-pushed the improve_local_tarballs_caching branch 2 times, most recently from bf32b6e to 7a4d007 Compare February 1, 2024 13:25

juliayakovlev force-pushed the improve_local_tarballs_caching branch 2 times, most recently from ad3a94e to 61671b9 Compare February 1, 2024 14:47

juliayakovlev requested a review from fruch February 1, 2024 15:06

juliayakovlev force-pushed the improve_local_tarballs_caching branch from 61671b9 to bdf4ea1 Compare February 1, 2024 15:30

fruch approved these changes Feb 6, 2024

View reviewed changes

fruch merged commit cf94a58 into scylladb:next Feb 6, 2024
4 checks passed

fruch mentioned this pull request Feb 7, 2024

Improve caching of local tarballs #521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement(setup): improve caching of local tarballs #557

improvement(setup): improve caching of local tarballs #557

juliayakovlev commented Jan 28, 2024 •

edited

Loading

fruch Jan 31, 2024

juliayakovlev Jan 31, 2024

tchaikov Feb 1, 2024

juliayakovlev Feb 1, 2024 •

edited

Loading

tchaikov Feb 1, 2024

juliayakovlev Feb 1, 2024

fruch Feb 1, 2024

juliayakovlev Feb 1, 2024

tchaikov Feb 1, 2024

tchaikov left a comment

tchaikov Feb 1, 2024

fruch Feb 1, 2024

juliayakovlev Feb 1, 2024

tchaikov Feb 1, 2024

juliayakovlev Feb 1, 2024

tchaikov Feb 1, 2024 •

edited

Loading

fruch Feb 1, 2024

juliayakovlev Feb 1, 2024

juliayakovlev Feb 1, 2024

fruch Feb 1, 2024

fruch Feb 1, 2024

juliayakovlev Feb 1, 2024

juliayakovlev Feb 1, 2024

juliayakovlev Feb 1, 2024

juliayakovlev Feb 1, 2024

fruch Feb 1, 2024

juliayakovlev Feb 1, 2024

fruch Feb 1, 2024

tchaikov left a comment

juliayakovlev commented Feb 6, 2024 •

edited

Loading

fruch left a comment

		if not (new_hash and current_hash) or new_hash != current_hash:
		# remove version_dir

improvement(setup): improve caching of local tarballs #557

improvement(setup): improve caching of local tarballs #557

Conversation

juliayakovlev commented Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliayakovlev Feb 1, 2024 • edited Loading

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

tchaikov 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

tchaikov Feb 1, 2024 • edited Loading

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

tchaikov left a comment

Choose a reason for hiding this comment

juliayakovlev commented Feb 6, 2024 • edited Loading

fruch left a comment

Choose a reason for hiding this comment

juliayakovlev commented Jan 28, 2024 •

edited

Loading

juliayakovlev Feb 1, 2024 •

edited

Loading

tchaikov Feb 1, 2024 •

edited

Loading

juliayakovlev commented Feb 6, 2024 •

edited

Loading