Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

scan: optimize the OSTree case#917

Closed
giuseppe wants to merge 4 commits intoprojectatomic:masterfrom
giuseppe:scan-optimize-ostree
Closed

scan: optimize the OSTree case#917
giuseppe wants to merge 4 commits intoprojectatomic:masterfrom
giuseppe:scan-optimize-ostree

Conversation

@giuseppe
Copy link
Collaborator

create hard links instead of copying files when possible.

Closes: #916

@baude
Copy link
Member

baude commented Feb 27, 2017

@giuseppe lets talk tomorrow. im refactoring Scan right now. Would like to complete that and then we can fix the ostree mount point issues.

Atomic/mount.py Outdated
def mount(self):
def _try_ostree_mount(self, best_mountpoint_for_storage):
if best_mountpoint_for_storage:
mountpoint = os.path.join(self.syscontainers.get_ostree_repo_location(), "tmp", str(os.getpid()), self.image)
Copy link
Member

@cgwalters cgwalters Feb 27, 2017

Choose a reason for hiding this comment

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

Using getpid() in here is going to make it hard to avoid leaking them. I'd make it get_repo_location() + /tmp/atomic-mount or something. Then it's easy to have a cleanup function delete that directory. This is what we do in rpm-ostree. Now, inside that directory, maybe you do want a pid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I have pushed a fixup patch to change this to /tmp/atomic-mount.

@giuseppe
Copy link
Collaborator Author

@baude The changes to scan.py are quite limited. Are these patches going to conflict with the refactor you are working on? I am ok to resolve these conflicts on top of your work

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably f5419ec) made this pull request unmergeable. Please resolve the merge conflicts.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
It optimizes the OSTree case where we prefer to use the same file
system of the repository for the checkout so that we can use hard links
instead of copying files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
let the storage choose the best mountpoint.

Closes: projectatomic#916

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the scan-optimize-ostree branch from 97be3ef to f005964 Compare February 28, 2017 15:54
@giuseppe giuseppe changed the title [WIP] scan: optimize the OSTree case scan: optimize the OSTree case Feb 28, 2017
@giuseppe
Copy link
Collaborator Author

@baude rebased on top of your series

@giuseppe
Copy link
Collaborator Author

one issue I've seen is that if the image is present in both repositories, scan fails with an unfriendly:

ValueError: Found more than one Image with name 7968321274dc6b6171697c33df7815310468e694ac5be0ec03ff053bb135e768; please specify with --storage.

Not sure how to handle this, if mount fails, iterate over all the backends until the image is found?

@baude
Copy link
Member

baude commented Feb 28, 2017

@giuseppe ill look into that issue deeper but outside the context of this PR. Sound good?

@giuseppe
Copy link
Collaborator Author

@baude, sure just wanted to point it out so that we could discuss it

@baude
Copy link
Member

baude commented Feb 28, 2017

LGTM!

@baude
Copy link
Member

baude commented Feb 28, 2017

@rh-atomic-bot r+ f005964

@rh-atomic-bot
Copy link

⌛ Testing commit f005964 with merge 90146aa...

rh-atomic-bot pushed a commit that referenced this pull request Feb 28, 2017
It optimizes the OSTree case where we prefer to use the same file
system of the repository for the checkout so that we can use hard links
instead of copying files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #917
Approved by: baude
rh-atomic-bot pushed a commit that referenced this pull request Feb 28, 2017
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #917
Approved by: baude
rh-atomic-bot pushed a commit that referenced this pull request Feb 28, 2017
let the storage choose the best mountpoint.

Closes: #916

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #917
Approved by: baude
@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: baude
Pushing 90146aa to master...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants