Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

Make the workspace path relative to the manifest file #444

Closed
wants to merge 3 commits into from

Conversation

mengelmann
Copy link
Contributor

Make the workspace path relative to the manifest file (resolves #424).

I had to modify minimize_size plugin.

I left individual commits to show the process flow.

Copy link
Owner

@andsens andsens left a comment

Choose a reason for hiding this comment

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

Nice job! I'm pretty sure the add_mount() call will not work as expected though. See my comment.

@@ -22,7 +22,9 @@ def __init__(self, manifest=None, debug=False):

# Define the path to our workspace
import os.path
self.workspace = os.path.join(manifest.bootstrapper['workspace'], self.run_id)
from bootstrapvz.common.tools import rel_path
self.workspace_root = rel_path(manifest.path, os.path.join(manifest.bootstrapper['workspace']))
Copy link
Owner

Choose a reason for hiding this comment

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

Nice, this adds some semantic info to the code, I like it.

@@ -22,7 +22,7 @@ def run(cls, info):

full_path = os.path.join(info.root, folder)
os.chmod(temp_path, os.stat(full_path).st_mode)
info.volume.partition_map.root.add_mount(temp_path, full_path, ['--bind'])
info.volume.partition_map.root.add_mount(temp_path, folder, ['--bind'])
Copy link
Owner

Choose a reason for hiding this comment

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

The mount call here is not executed in the chroot, we need the full path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

full path is not required as it is possible to mount directory with relative paths, like this:

service:/tmp/mount-test$ mkdir src dst
service:/tmp/mount-test$ touch src/test
service:/tmp/mount-test$ sudo mount --bind src dst
service:/tmp/mount-test$ ls -1 dst
test

Actually with full_path I got errors like this:

[89365.6539917] INFO: Mounting folders for writing temporary and cache data
[89365.9210205] DEBUG: Executing: mount --bind dist/6287dc50/minimize_size/tmp dist/6287dc50/root/dist/6287dc50/root/tmp
[89369.1380024] ERROR: mount: mount point dist/6287dc50/root/dist/6287dc50/root/tmp does not exist
[89469.5329666] ERROR: Command 'mount --bind dist/6287dc50/minimize_size/tmp dist/6287dc50/root/dist/6287dc50/root/tmp' returned non-zero exit status 32
Traceback (most recent call last):
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/base/main.py", line 111, in run
    tasklist.run(info=bootstrap_info, dry_run=dry_run)
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/base/tasklist.py", line 44, in run
    task.run(info)
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/plugins/minimize_size/tasks/mounts.py", line 27, in run
    info.volume.partition_map.root.add_mount(temp_path, full_path, ['--bind'])
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/base/fs/partitions/abstract.py", line 123, in add_mount
    mount.mount(self.mount_dir)
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/base/fs/partitions/mount.py", line 29, in mount
    log_check_call(['mount'] + self.opts + [self.source, mount_dir])
  File "/home/users/mengelmann/diskimage-builder-imagin/bootstrap-vz/bootstrapvz/common/tools.py", line 13, in log_check_call
    raise e
CalledProcessError: Command 'mount --bind dist/6287dc50/minimize_size/tmp dist/6287dc50/root/dist/6287dc50/root/tmp' returned non-zero exit status 32
[89470.1631069] ERROR: Rolling back

Copy link
Owner

Choose a reason for hiding this comment

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

Wait. This suggests info.root is not absolute any longer. It's set here:

class CreateMountDir(Task):
description = 'Creating mountpoint for the root partition'
phase = phases.volume_mounting
@classmethod
def run(cls, info):
import os
info.root = os.path.join(info.workspace, 'root')
os.makedirs(info.root)

Are you sure info.workspace is still absolute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, info.root is not absolute any longer, it becomes relative to the manifest file. Should it be absolute path?

Copy link
Owner

Choose a reason for hiding this comment

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

It should be, yes. Can you check if this was the case before the change?

@mengelmann
Copy link
Contributor Author

Just quick update - I'll get back to this PR next week.

@andsens
Copy link
Owner

andsens commented May 2, 2018

Closing for now, feel free to reopen when you got the changes ready.

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.

Provide an absolute path to a debootstrap tarball
2 participants