[pbs-devel] [PATCH proxmox-backup 1/2] mount zpools created via API under /mnt/datastore

[pbs-devel] [PATCH proxmox-backup 1/2] mount zpools created via API under /mnt/datastore

[Fabian Ebner]

as we do for other file systems

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 docs/storage.rst           |  2 +-
 src/api2/node/disks/zfs.rs | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/docs/storage.rst b/docs/storage.rst
index 8541237b..022d951e 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -57,7 +57,7 @@ create a datastore at the location ``/mnt/datastore/store1``:
 You can also create a ``zpool`` with various raid levels from **Administration
 -> Disks -> Zpool** in the web interface, or by using ``zpool create``. The command
 below creates a mirrored ``zpool`` using two disks (``sdb`` & ``sdc``) and
-mounts it on the root directory (default):
+mounts it under ``/mnt/datastore/zpool1``:
 
 .. code-block:: console
 
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 79094012..ef7b7c67 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -243,7 +243,7 @@ pub fn zpool_details(
         permission: &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false),
     },
 )]
-/// Create a new ZFS pool.
+/// Create a new ZFS pool. Will be mounted under '/mnt/datastore/<name>'.
 pub fn create_zpool(
     name: String,
     devices: String,
@@ -303,10 +303,11 @@ pub fn create_zpool(
         bail!("{:?} needs at least {} disks.", raidlevel, min_disks);
     }
 
+    let mount_point = format!("/mnt/datastore/{}", &name);
+
     // check if the default path does exist already and bail if it does
-    // otherwise we get an error on mounting
-    let mut default_path = std::path::PathBuf::from("/");
-    default_path.push(&name);
+    // otherwise 'zpool create' aborts after partitioning, but before creating the pool
+    let default_path = std::path::PathBuf::from(&mount_point);
 
     match std::fs::metadata(&default_path) {
         Err(_) => {}, // path does not exist
@@ -322,7 +323,7 @@ pub fn create_zpool(
 
 
             let mut command = std::process::Command::new("zpool");
-            command.args(&["create", "-o", &format!("ashift={}", ashift), &name]);
+            command.args(&["create", "-o", &format!("ashift={}", ashift), "-m", &mount_point, &name]);
 
             match raidlevel {
                 ZfsRaidLevel::Single => {
@@ -371,7 +372,6 @@ pub fn create_zpool(
             }
 
             if add_datastore {
-                let mount_point = format!("/{}", name);
                 crate::api2::config::datastore::create_datastore(json!({ "name": name, "path": mount_point }))?
             }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/2] bail if mount point already exists for directories

[Fabian Ebner]

similar to what we do for zfs. By bailing before partitioning, the disk is
still considered unused after a failed attempt.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

There is no other caller for create_datastore_mount_unit, so the check from
the not yet applied https://lists.proxmox.com/pipermail/pbs-devel/2020-November/001297.html
is superseded by this.

 src/api2/node/disks/directory.rs | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 2a4780f2..da16e753 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -142,6 +142,18 @@ pub fn create_datastore_disk(
         bail!("disk '{}' is already in use.", disk);
     }
 
+    let mount_point = format!("/mnt/datastore/{}", &name);
+
+    // check if the default path does exist already and bail if it does
+    let default_path = std::path::PathBuf::from(&mount_point);
+
+    match std::fs::metadata(&default_path) {
+        Err(_) => {}, // path does not exist
+        Ok(_) => {
+            bail!("path {:?} already exists", default_path);
+        }
+    }
+
     let upid_str = WorkerTask::new_thread(
         "dircreate", Some(name.clone()), auth_id, to_stdout, move |worker|
         {
@@ -160,7 +172,7 @@ pub fn create_datastore_disk(
             let uuid = get_fs_uuid(&partition)?;
             let uuid_path = format!("/dev/disk/by-uuid/{}", uuid);
 
-            let (mount_unit_name, mount_point) = create_datastore_mount_unit(&name, filesystem, &uuid_path)?;
+            let mount_unit_name = create_datastore_mount_unit(&name, &mount_point, filesystem, &uuid_path)?;
 
             systemd::reload_daemon()?;
             systemd::enable_unit(&mount_unit_name)?;
@@ -243,11 +255,11 @@ pub const ROUTER: Router = Router::new()
 
 fn create_datastore_mount_unit(
     datastore_name: &str,
+    mount_point: &str,
     fs_type: FileSystemType,
     what: &str,
-) -> Result<(String, String), Error> {
+) -> Result<String, Error> {
 
-    let mount_point = format!("/mnt/datastore/{}", datastore_name);
     let mut mount_unit_name = systemd::escape_unit(&mount_point, true);
     mount_unit_name.push_str(".mount");
 
@@ -265,7 +277,7 @@ fn create_datastore_mount_unit(
 
     let mount = SystemdMountSection {
         What: what.to_string(),
-        Where: mount_point.clone(),
+        Where: mount_point.to_string(),
         Type: Some(fs_type.to_string()),
         Options: Some(String::from("defaults")),
         ..Default::default()
@@ -278,5 +290,5 @@ fn create_datastore_mount_unit(
 
     systemd::config::save_systemd_mount(&mount_unit_path, &config)?;
 
-    Ok((mount_unit_name, mount_point))
+    Ok(mount_unit_name)
 }
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup 1/2] mount zpools created via API under /mnt/datastore

[Thomas Lamprecht]

On 05.11.20 14:36, Fabian Ebner wrote:
> as we do for other file systems
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  docs/storage.rst           |  2 +-
>  src/api2/node/disks/zfs.rs | 12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
>

applied, thanks!

[pbs-devel] applied: [PATCH proxmox-backup 2/2] bail if mount point already exists for directories

[Thomas Lamprecht]

On 05.11.20 14:36, Fabian Ebner wrote:
> similar to what we do for zfs. By bailing before partitioning, the disk is
> still considered unused after a failed attempt.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> There is no other caller for create_datastore_mount_unit, so the check from
> the not yet applied https://lists.proxmox.com/pipermail/pbs-devel/2020-November/001297.html
> is superseded by this.
> 
>  src/api2/node/disks/directory.rs | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

maybe we should loosen this check a bit and also allow existing but empty
directories?  But for now OK, this is in a pretty specific namespace anyway.