Re: [pbs-devel] [PATCH proxmox-backup v12 13/26] datastore: handle deletion of removable datastore properly

["Hannes Laimer" <h.laimer@proxmox.com>]

On Mon Oct 14, 2024 at 3:42 PM CEST, Fabian Grünbichler wrote:
> On September 4, 2024 4:11 pm, Hannes Laimer wrote:
> > Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> > ---
> >  pbs-datastore/src/datastore.rs |  4 +++-
> >  src/api2/config/datastore.rs   | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> > index 29f98b37..df038d62 100644
> > --- a/pbs-datastore/src/datastore.rs
> > +++ b/pbs-datastore/src/datastore.rs
> > @@ -1510,7 +1510,9 @@ impl DataStore {
> >                          // weird, but ok
> >                      }
> >                      Err(err) if err.is_errno(nix::errno::Errno::EBUSY) => {
> > -                        warn!("Cannot delete datastore directory (is it a mount point?).")
> > +                        if datastore_config.backing_device.is_none() {
> > +                            warn!("Cannot delete datastore directory (is it a mount point?).")
> > +                        }
> >                      }
> >                      Err(err) if err.is_errno(nix::errno::Errno::ENOTEMPTY) => {
> >                          warn!("Datastore directory not empty, not deleting.")
> > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> > index c24b6e9d..c7d59c44 100644
> > --- a/src/api2/config/datastore.rs
> > +++ b/src/api2/config/datastore.rs
> > @@ -29,9 +29,11 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
> >  use crate::api2::config::verify::delete_verification_job;
> >  use pbs_config::CachedUserInfo;
> >  
> > +use pbs_datastore::is_datastore_available;
> >  use proxmox_rest_server::WorkerTask;
> >  
> >  use crate::server::jobstate;
> > +use crate::tools::disks::unmount_by_mountpoint;
> >  
> >  #[api(
> >      input: {
> > @@ -523,6 +525,14 @@ pub async fn delete_datastore(
> >          http_bail!(NOT_FOUND, "datastore '{}' does not exist.", name);
> >      }
> >  
> > +    let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
> > +    if destroy_data && !is_datastore_available(&store_config) {
> > +        http_bail!(
> > +            BAD_REQUEST,
> > +            "cannot destroy data on '{name}' unless the datastore is mounted"
> > +        );
> > +    }
> > +
> >      if !keep_job_configs {
> >          for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
> >              delete_verification_job(job.config.id, None, rpcenv)?
> > @@ -549,6 +559,22 @@ pub async fn delete_datastore(
> >  
> >      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> >      let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> > +    let name_copy = name.clone();
> > +    tokio::spawn(async move {
> > +        if let Ok(proxy_pid) =
> > +            proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> > +        {
> > +            let sock = proxmox_daemon::command_socket::path_from_pid(proxy_pid);
> > +            let _ = proxmox_daemon::command_socket::send_raw(
> > +                sock,
> > +                &format!(
> > +                    "{{\"command\":\"update-datastore-cache\",\"args\":\"{}\"}}\n",
> > +                    name_copy
> > +                ),
> > +            )
> > +            .await;
> > +        }
> > +    });
>
> why is this tokio::spawned, but the one in unmount introduced in the
> same series is not?
>

I took it from update_datastore which is not async, you're right we don't need
spawn here.

> since we now have three call sites with basically the same code, should
> we have a helper to do this?
>

yes

> >  
> >      let upid = WorkerTask::new_thread(
> >          "delete-datastore",
> > @@ -561,6 +587,12 @@ pub async fn delete_datastore(
> >              // ignore errors
> >              let _ = jobstate::remove_state_file("prune", &name);
> >              let _ = jobstate::remove_state_file("garbage_collection", &name);
> > +            if destroy_data {
> > +                if let Some(mount_point) = store_config.get_mount_point() {
> > +                    let _ = unmount_by_mountpoint(&mount_point);
> > +                    let _ = std::fs::remove_dir(&mount_point);
> > +                }
> > +            }
> >  
> >              if let Err(err) =
> >                  proxmox_async::runtime::block_on(crate::server::notify_datastore_removed())
> > -- 
> > 2.39.2
> > 
> > 
> > 
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> > 
> > 
> > 
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel