From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dietmar@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id BC34793EBB
 for <pbs-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:12:47 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 972BAAB3E
 for <pbs-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:12:17 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pbs-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:12:16 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 44EC843A72
 for <pbs-devel@lists.proxmox.com>; Wed, 10 Apr 2024 11:12:16 +0200 (CEST)
Date: Wed, 10 Apr 2024 11:12:15 +0200 (CEST)
From: Dietmar Maurer <dietmar@proxmox.com>
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>, 
 Hannes Laimer <h.laimer@proxmox.com>
Message-ID: <451634062.5587.1712740335197@webmail.proxmox.com>
In-Reply-To: <20240409110012.166472-7-h.laimer@proxmox.com>
References: <20240409110012.166472-1-h.laimer@proxmox.com>
 <20240409110012.166472-7-h.laimer@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-Priority: 3
Importance: Normal
X-Mailer: Open-Xchange Mailer v7.10.6-Rev61
X-Originating-Client: open-xchange-appsuite
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.347 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 06/24] api2: admin: add
 (un)mount endpoint for removable datastores
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 10 Apr 2024 09:12:47 -0000

more comments inline:

> +pub fn do_mount_device(
> +    _lock: Option<BackupLockGuard>,
> +    mut config: SectionConfigData,
> +    mut datastore: DataStoreConfig,
> +    worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> +    if let Some(uuid) = datastore.backing_device.as_ref() {
> +        if pbs_datastore::check_if_available(&datastore).is_ok() {
> +            return Err(format_err!("device '{}' is already mounted", &uuid));
> +        }
> +        let mount_point_path = std::path::Path::new(&datastore.path);
> +        if let Some(worker) = worker {
> +            task_log!(worker, "mounting '{}' to '{}'", uuid, datastore.path);
> +        }
> +        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
> +
> +        datastore.set_maintenance_mode(None);

You change the maintenance mode, but do not notify the proxy?

> +        config.set_data(&datastore.name, "datastore", &datastore)?;
> +        pbs_config::datastore::save_config(&config)?;
> +
> +        Ok(())
> +    } else {
> +        Err(format_err!(
> +            "Datastore '{}' can't be mounted because it is not removable.",
> +            &datastore.name
> +        ))
> +    }
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        }
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> +    },
> +)]
> +/// Mount removable datastore.
> +pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (section_config, _digest) = pbs_config::datastore::config()?;
> +    let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> +    if datastore.backing_device.is_none() {
> +        bail!("datastore '{}' is not removable", &store);
> +    }
> +
> +    let lock = pbs_config::datastore::lock_config()?;
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    let upid = WorkerTask::new_thread(
> +        "mount-device",
> +        Some(store),
> +        auth_id.to_string(),
> +        to_stdout,
> +        move |worker| do_mount_device(Some(lock), section_config, datastore, Some(&worker)),
> +    )?;
> +
> +    Ok(json!(upid))
> +}
> +
> +fn do_unmount_device(
> +    force: bool,
> +    datastore: DataStoreConfig,
> +    worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> +    if force {
> +        let _ = crate::tools::disks::unmount_by_mountpoint(&datastore.path);
> +        return Ok(());
> +    }
> +
> +    let mut active_operations = task_tracking::get_active_operations(&datastore.name)?;
> +    while active_operations.read + active_operations.write > 0 {
> +        if let Some(worker) = worker {
> +            if worker.abort_requested() {
> +                bail!("aborted, due to user request");
> +            }
> +            task_log!(
> +                worker,
> +                "can't unmount yet, still {} read and {} write operations active",
> +                active_operations.read,
> +                active_operations.write
> +            );
> +        }
> +
> +        std::thread::sleep(std::time::Duration::new(5, 0));
> +        active_operations = task_tracking::get_active_operations(&datastore.name)?;
> +    }
> +    crate::tools::disks::unmount_by_mountpoint(&datastore.path)?;
> +
> +    Ok(())
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: { schema: DATASTORE_SCHEMA },
> +            force: {
> +                type: Boolean,
> +                description: "Force unmount even if there are active operations.",
> +                optional: true,
> +                default: false,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> +    }
> +)]
> +/// Unmount a removable device that is associated with the datastore
> +pub async fn unmount(
> +    store: String,
> +    force: bool,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +    let _lock = pbs_config::datastore::lock_config()?;
> +    let (mut section_config, _digest) = pbs_config::datastore::config()?;
> +    let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> +    if datastore.backing_device.is_none() {
> +        bail!("datastore '{}' is not removable", &store);
> +    }
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    datastore.set_maintenance_mode(Some(MaintenanceMode::new(MaintenanceType::Unplugged, None)));
> +    section_config.set_data(&datastore.name, "datastore", &datastore)?;
> +    pbs_config::datastore::save_config(&section_config)?;
> +    drop(_lock);
> +
> +    if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> +    {
> +        let sock = proxmox_rest_server::ctrl_sock_from_pid(proxy_pid);
> +        let _ = proxmox_rest_server::send_raw_command(
> +            sock,
> +            &format!(
> +                "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
> +                &store
> +            ),
> +        )
> +        .await;
> +    }

I found the same code in strc/api2/config/datastore.rs. I guess its is worth to factor this out.