Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup files.PublishedStorage after dropping a publish #1381

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cleanup files.PublishedStorage after dropping a publish #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions deb/publish.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/aptly-dev/aptly/aptly"
"github.com/aptly-dev/aptly/database"
"github.com/aptly-dev/aptly/files"
"github.com/aptly-dev/aptly/pgp"
"github.com/aptly-dev/aptly/utils"
)
Expand Down Expand Up @@ -1323,14 +1324,20 @@ func (collection *PublishedRepoCollection) Remove(publishedStorageProvider aptly
collection.list[len(collection.list)-1], collection.list[repoPosition], collection.list =
nil, collection.list[len(collection.list)-1], collection.list[:len(collection.list)-1]

if !skipCleanup && len(cleanComponents) > 0 {
err = collection.CleanupPrefixComponentFiles(repo.Prefix, cleanComponents,
publishedStorageProvider.GetPublishedStorage(storage), collectionFactory, progress)
if err != nil {
if !force {
return fmt.Errorf("cleanup failed, use -force-drop to override: %s", err)
if !skipCleanup {
neolynx marked this conversation as resolved.
Show resolved Hide resolved
publishedStorage := publishedStorageProvider.GetPublishedStorage(storage)
if len(cleanComponents) > 0 {
err = collection.CleanupPrefixComponentFiles(repo.Prefix, cleanComponents, publishedStorage, collectionFactory, progress)
if err != nil {
if !force {
return fmt.Errorf("cleanup failed, use -force-drop to override: %s", err)
}
}
}

if localStorage, ok := publishedStorage.(*files.PublishedStorage); ok {
neolynx marked this conversation as resolved.
Show resolved Hide resolved
localStorage.CleanupPublishTree(repo.Prefix, progress)
}
}

batch := collection.db.CreateBatch()
Expand Down
32 changes: 32 additions & 0 deletions files/public.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,38 @@ func (storage *PublishedStorage) RemoveDirs(path string, progress aptly.Progress
return os.RemoveAll(filepath)
}

// Cleans up PublishedStorage tree towards the publish storage root up to first non-empty directory
func (storage *PublishedStorage) CleanupPublishTree(path string, progress aptly.Progress) {
path = strings.TrimLeft(path, "./")
if path == "" {
return
}
/*
Say, `path` is "a/b/c/d", let's remove all empty dirs:
storage.rootPath + "a/b/c/d"
storage.rootPath + "a/b/c"
storage.rootPath + "a/b"
... and so on, up to storage.rootPath, first non-empty directory or any other os.Remove error, whichever comes first
*/

segments := strings.Split(path, "/")

for segmentsCount := len(segments); segmentsCount > 0; segmentsCount-- {
fullPath := filepath.Join(storage.rootPath, strings.Join(segments[:segmentsCount], "/"))

if err := os.Remove(fullPath); err != nil {
// We stop crawling the tree on any os.Remove error.
// Among all of the return values there might be 'directory not empty',
// which means, we reached non-empty dir, which is okay.
// Any other errors are logged for informational reasons.
if progress != nil {
progress.Printf("CleanupPublishTree: %s. '%s' not removed\n", err, fullPath)
}
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please log the error. we canna see if we have werid permissions like 000 or other behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, log.Printf will suffice? Or should I bring "github.com/rs/zerolog/log" which is used in other files there?

Copy link
Member

@neolynx neolynx Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in line 117 progress.Printf is used in RemoveDirs. I think we should follow that pattern... or return an fmt.Errorf as in line 160. what do you think ?

Copy link
Contributor Author

@aol-nnov aol-nnov Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or return an fmt.Errorf

and what should be done then? It's not the error, that needs any further handling.

Implemented it via progress.Printf as you proposed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a (misconfigured) remote file server, there might be permission issues when removing directories. such errors should be logged... this is why I would still like to have a check for emptyness, bcs then we do not get an error.

do we have a check where a directory is not removed bcs not empty ?

another little thing: progress logs in aptly do not have punctiations, could you make the log similar to the others ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, too much moving parts here..

should be logged

but strings printed by aptly.Progress are seen only during cli operation

remote file server

In api mode all logging is handled by gin or zerolog, I do not know for sure, but plain fmt.Println from gin controller in aptly outputs nothing to stdout.

Your intentions regarding localStorage.CleanupPublishTree() "errors" as you call them is not clear.

Imagine, we will return an error object from it, handle it in func (collection *PublishedRepoCollection) Remove() like

		if localStorage, ok := publishedStorage.(*files.PublishedStorage); ok {
			if err := localStorage.CleanupPublishTree(repo.Prefix, progress); err != nil {
				return fmt.Errorf("")
			}
		}
	}

	batch := collection.db.CreateBatch()
	batch.Delete(repo.Key())

	for _, component := range repo.Components() {
		batch.Delete(repo.RefKey(component))
	}

	return batch.Write()
}

But is that really an error of a scale, that we should terminate the whole process and to not update database at this point?!

Keep in mind, that by the time localStorage.CleanupPublishTree() is executed, all files concerned the publish being Remove()-ed are long ago gone. So, if we terminate the execution of Remove() at this point, we'll find ourselves in an inconsistent state!

To my understanding, silently leaving directories intact if we can not remove them is the best bet in this situation.
Okay, we'll log this in cli via progress, but how about api mode?

Also, as I mentioned earlier, I was unable log anything to console by plain printf from gin controller.

}
}
}

// LinkFromPool links package file from pool to dist's pool location
//
// publishedPrefix is desired prefix for the location in the pool.
Expand Down
5 changes: 5 additions & 0 deletions system/t06_publish/PublishDrop21Test_gold
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Removing ${HOME}/.aptly/public/ppa/smira/dists...
Removing ${HOME}/.aptly/public/ppa/smira/pool...
CleanupPublishTree: remove ${HOME}/.aptly/public/ppa: directory not empty. '${HOME}/.aptly/public/ppa' not removed

Published repository has been removed successfully.
34 changes: 30 additions & 4 deletions system/t06_publish/drop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def check(self):
class PublishDrop2Test(BaseTest):
"""
publish drop: under prefix
if remaining tree is empty, it is now removed
"""
requiresGPG2 = True
fixtureDB = True
Expand All @@ -39,9 +40,32 @@ class PublishDrop2Test(BaseTest):
def check(self):
super(PublishDrop2Test, self).check()

self.check_not_exists('public/ppa/smira/dists/')
self.check_not_exists('public/ppa/smira/pool/')
self.check_exists('public/ppa/smira/')
self.check_not_exists('public/ppa/')
self.check_exists('public/')


class PublishDrop21Test(BaseTest):
"""
publish drop: under prefix
if some publishes have common part of the prefix, only empty parts are cleared
"""
requiresGPG2 = True
fixtureDB = True
fixturePool = True
fixtureCmds = [
"aptly snapshot create snap1 from mirror gnuplot-maverick",
"aptly snapshot create snap2 from mirror gnuplot-maverick",
"aptly publish snapshot -keyring=${files}/aptly.pub -secret-keyring=${files}/aptly.sec snap1 ppa/smira",
"aptly publish snapshot -keyring=${files}/aptly.pub -secret-keyring=${files}/aptly.sec snap2 ppa/another",
]
runCmd = "aptly publish drop maverick ppa/smira"
gold_processor = BaseTest.expand_environ

def check(self):
super(PublishDrop21Test, self).check()

self.check_not_exists('public/ppa/smira/')
self.check_exists('public/ppa/another')


class PublishDrop3Test(BaseTest):
Expand Down Expand Up @@ -132,6 +156,7 @@ class PublishDrop6Test(BaseTest):
class PublishDrop7Test(BaseTest):
"""
publish drop: under prefix with trailing & leading slashes
see comment in PublishDrop2Test
"""
requiresGPG2 = True
fixtureDB = True
Expand All @@ -148,7 +173,8 @@ def check(self):

self.check_not_exists('public/ppa/smira/dists/')
self.check_not_exists('public/ppa/smira/pool/')
self.check_exists('public/ppa/smira/')
self.check_not_exists('public/ppa/smira/')
self.check_exists('public/')


class PublishDrop8Test(BaseTest):
Expand Down
Loading