public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Dominik Csapak <d.csapak@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Hannes Laimer <h.laimer@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore
Date: Fri, 1 Oct 2021 10:18:01 +0200	[thread overview]
Message-ID: <0a1f9d39-b658-5265-e59c-ca86376a61a5@proxmox.com> (raw)
In-Reply-To: <20210928100548.4873-1-h.laimer@proxmox.com>

high level comment here, rest in the individual patches following

i like the idea, but i find some things a bit clunky:

* what gets saved is the maintenance mode (which is okay) but the
interface to check it is confusing since the user has to give the mode
that is *not* enough. IMHO it would be better if the check would require
the least mode it requires instead. this could either be a "none" (or
"writable") mode of the enum, or making the parameter an Option where
None represents needing write access.

* the check requiring an addition call to the storage seems like a thing
one can easily forget. instead we could make our intent clear on the
lookup part, which could do the check for us. if we then need elevated
access, we can still have the check function. this way we cannot even
request a datastore if its in offline mode anywhere in the code.

i know this would require more code changes possibly, but i think
this would provide a safer interface for this feature




  parent reply	other threads:[~2021-10-01  8:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-28 10:05 Hannes Laimer
2021-09-28 10:05 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-api-types: add maintenance type and msg to ds config Hannes Laimer
2021-10-01  8:23   ` Dominik Csapak
2021-10-01 12:00     ` Wolfgang Bumiller
2021-09-28 10:05 ` [pbs-devel] [PATCH proxmox-backup 2/5] pbs-datastore: add check_maintenence function Hannes Laimer
2021-10-01 12:06   ` Wolfgang Bumiller
2021-09-28 10:05 ` [pbs-devel] [PATCH proxmox-backup 3/5] api2: make maintenance type and msg updatable/deletable Hannes Laimer
2021-09-28 10:05 ` [pbs-devel] [PATCH proxmox-backup 4/5] jobs/api2: add checks for maintenance Hannes Laimer
2021-10-01  8:28   ` Dominik Csapak
2021-09-28 10:05 ` [pbs-devel] [PATCH proxmox-backup 5/5] ui: add maintenance to datastore options Hannes Laimer
2021-10-01  8:36   ` Dominik Csapak
2021-10-01  8:18 ` Dominik Csapak [this message]
2021-10-01  9:36 ` [pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore Dominik Csapak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0a1f9d39-b658-5265-e59c-ca86376a61a5@proxmox.com \
    --to=d.csapak@proxmox.com \
    --cc=h.laimer@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal