Support Acquire-By-Hash for index files #664

sliverc · 2017-11-02T13:33:58Z

Fixes #536

Description of the Change

This follows up on #551 beside small fixes adds following:

Add s3 and swift support
Add tests
Rename of option to -acquire-by-hash for consistency with other flags

Checklist

unit-test added (if change is algorithm)
functional test added/updated (if change is functional)
man page updated (if applicable)
bash completion updated (if applicable)
documentation updated
author name in AUTHORS

sliverc · 2017-11-02T13:54:11Z

Added documentation in PR aptly-dev/www.aptly.info#55

smira · 2017-11-20T21:26:20Z

bash_completion.d/aptly

@@ -499,7 +499,7 @@ _aptly()
          "snapshot"|"repo")
            if [[ $numargs -eq 0 ]]; then
              if [[ "$cur" == -* ]]; then
-                COMPREPLY=($(compgen -W "-batch -force-overwrite -distribution= -component= -gpg-key= -keyring= -label= -origin= -notautomatic= -butautomaticupgrades= -passphrase= -passphrase-file= -secret-keyring= -skip-contents -skip-signing" -- ${cur}))
+                COMPREPLY=($(compgen -W "-batch -force-overwrite -distribution= -component= -gpg-key= -keyring= -label= -origin= -notautomatic= -butautomaticupgrades= -passphrase= -passphrase-file= -secret-keyring= -skip-contents -skip-signing -acquire-by-hash" -- ${cur}))


Could you please keep options sorted here, so that it's easier to find if something is there in the list or not?

Make sense. There were some other options which were not in order either so reordered whole list.

smira

This looks like really awesome work, thanks!

I left some comments which are more or less small nits

smira · 2017-11-20T21:28:37Z

deb/index_files.go

+	hashs := []string{}
+	if file.acquireByHash {
+		hashs = append(hashs, "MD5Sum", "SHA1", "SHA256", "SHA512")
+		for _, hash := range hashs {


I think this can be straight for _, hash := range []string{"MD5Sum", "SHA1", ....} {

smira · 2017-11-20T21:29:08Z

deb/index_files.go

+		if file.acquireByHash {
+			sums := file.parent.generatedFiles[file.relativePath+ext]
+
+			err = packageIndexByHash(file, ext, "SHA512", sums.SHA512)


I think we can rewrite this in a loop similar line 107?

Used map to accomplish this.

smira · 2017-11-20T21:29:33Z

deb/index_files.go

+
+			err = packageIndexByHash(file, ext, "SHA512", sums.SHA512)
+			if err != nil {
+				fmt.Printf("%s\n", err)


We should return an error here? Unless we want to ignore some errors (I would prefer error to be returned)

Agree. Changed.

smira · 2017-11-20T21:32:52Z

files/public.go

+
+// FileExists returns true if path exists
+func (storage *PublishedStorage) FileExists(path string) bool {
+	list, err := storage.Filelist(filepath.Dir(path))


Should we use os.Lstat (https://godoc.org/os#Lstat) + os.IsNotExist (https://godoc.org/os#IsNotExist) here? Listing directory seems to be really resource-consuming for this simple task?

Definitely. Changed as suggested.

There were also some unit tests missing which I added now.

smira · 2017-11-20T21:34:19Z

s3/public.go

+
+	params := &s3.CopyObjectInput{
+		Bucket:     aws.String(storage.bucket),
+		CopySource: aws.String(src),


I think we need to do filepath.Join(storage.prefix, src) here as well

smira · 2017-11-20T21:35:03Z

s3/public.go

+		Key:    aws.String(filepath.Join(storage.prefix, path)),
+	}
+	_, err := storage.s3.HeadObject(params)
+	return err == nil


I think we should distinguish "not found" error vs. some other AWS error (which would be an error, and it's better for FileExists to return bool, error here)

Make sense. Adjusted interface and also implemented difference between file existence and error in other storage types as well.

sliverc · 2017-11-21T11:49:15Z

@smira Adjusted PR according to your comments. Let me know if there is anything else.

The added "aptly publish repo" option "-access-by-hash" publishes the index files (Packages*, Sources*) also as hardlinked hashes. Example: /dists/yakkety/main/binary-amd64/by-hash/SHA512/31833ec39acc... The Release files indicate this with the option "Acquire-By-Hash: yes" This is used by apt >= 1.2.0 and prevents the "Hash sum mismatch" race condition between a server side "aptly publish repo" and "apt-get update" on a client. See: http://www.chiark.greenend.org.uk/~cjwatson/blog/no-more-hash-sum-mismatch-errors.html This implementation uses symlinks in the by-hash/*/ directory for keeping only two versions of the index files and deleting older files automatically. Note: this only works with aptly.FileSystemPublishedStorage Closes: aptly-dev#536 Signed-off-by: André Roth <[email protected]>

Signed-off-by: André Roth <[email protected]>

This makes it easier maintainable.

…rmance

…tence

smira

thanks, this looks good to me!

I was a bit concerned at first with index and index.old symlinks, but they seem to work perfectly fine!

erickeller · 2017-11-30T22:47:06Z

Great news... Thanks

sliverc mentioned this pull request Nov 2, 2017

Document new option acquire by hash aptly-dev/www.aptly.info#55

Merged

smira added 1.2.0 feature labels Nov 3, 2017

sliverc force-pushed the acquire-by-hash branch from 384300c to 34ae8bb Compare November 17, 2017 12:24

smira reviewed Nov 20, 2017

View reviewed changes

This was referenced Nov 20, 2017

Support Acquire-By-Hash for index files #551

Closed

support APT 'by-hash' indexes #381

Closed

sliverc force-pushed the acquire-by-hash branch 2 times, most recently from 1b9b9bf to 46cce5a Compare November 21, 2017 11:04

neolynx and others added 15 commits November 30, 2017 09:46

Extend PublishedStorage interface for Acquire-By-Hash

e079127

Signed-off-by: André Roth <[email protected]>

Extend s3 storage with link and file exists methods

7498fd8

Extend swift storage with link and file exists methods

438e206

Rename AccessByHash to AcquireByHash for consistency with other flags

092a7ed

Extend publish snapshot test with acquire by hash

2bd0b78

Fix linting errors

d6b4b79

Avoid exception when failing tests doesn't have a doc string

4718625

Add acquire by hash when updating publish

b4f3573

Update man page and bash completion

f5e1e19

Order publish parameters in bash completion

674a0e8

This makes it easier maintainable.

Simplify packaging indexing by hash and stop when there is an error

2e48860

Implement FileExists in files storage as simple stat to improve perfo…

3efa105

…rmance

Build src path on basis of storage prefix when symlinking

e504fdc

Adjust FileExists to differentiate between error and actual file exis…

b2bf4f7

…tence

sliverc force-pushed the acquire-by-hash branch from 46cce5a to b2bf4f7 Compare November 30, 2017 08:46

smira approved these changes Nov 30, 2017

View reviewed changes

smira merged commit 565fcf4 into aptly-dev:master Nov 30, 2017

sliverc mentioned this pull request Jun 18, 2018

apt-get update failing when mirrors are being updated #313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Acquire-By-Hash for index files #664

Support Acquire-By-Hash for index files #664

sliverc commented Nov 2, 2017 •

edited

Loading

sliverc commented Nov 2, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

smira left a comment

smira Nov 20, 2017

sliverc Nov 21, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

sliverc Nov 21, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

smira Nov 20, 2017

sliverc Nov 21, 2017

sliverc commented Nov 21, 2017

smira left a comment

erickeller commented Nov 30, 2017

Support Acquire-By-Hash for index files #664

Support Acquire-By-Hash for index files #664

Conversation

sliverc commented Nov 2, 2017 • edited Loading

Description of the Change

Checklist

sliverc commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smira 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

sliverc commented Nov 21, 2017

smira left a comment

Choose a reason for hiding this comment

erickeller commented Nov 30, 2017

sliverc commented Nov 2, 2017 •

edited

Loading