idempotency versus/and user comfort

[Jakub Krysl]

Hello all,

Recently I've raised an issue about user comfort when destroying stratis fs
[1] which got rejected with idempotency as reason. I do not agree with
this, let me explain why.

First let's start with the user comfort itself. It seems to be good
practice to fail on item removal if the item user is trying to remove is
not present. This can have many reasons varying from "item is already
removed" to "user made typo in item name". Here are some examples:
device mapper:
# dmsetup table
test: 0 102400 linear 7:0 102400
# dmsetup remove test
# echo $?
0
# dmsetup remove test
device-mapper: remove ioctl on test  failed: No such device or address
Command failed.
# echo $?
1
LVM:
# pvremove /dev/loop0
  Labels on physical volume "/dev/loop0" successfully wiped.
# echo $?
0
# pvremove /dev/loop0
  No PV found on device /dev/loop0.
# echo $?
5

On the other hand Stratis CLI does not work this way:
# stratis pool create test /dev/loop0
# stratis fs create test test_fs
# echo $?
0
# stratis fs create test test_fs
Execution failure caused by:
ALREADY EXISTS: test_fs
# echo $?
1
# stratis fs destroy test test_fs
# echo $?
0
# stratis fs destroy test test_fs
# echo $?
0

Now let's get to idempotency itself. Given the response in [1], command is
idempotent only when the output is teh same. Based on the example above it
seems that stratis create is actually not idempotent, as trying to create
the same fs again fails. But I'd like to pinpoint the definition and
application of idempotency first before settling this. I like the
idempotency definition from RFC 7231 [2] (HTTP/1.1), mainly the end of the
paragraph: "repeating the request will have the same intended effect, even
if the original request succeeded, though the response might differ."
Based on this it is actually idempotent to give user different response as
long as the effect is the same, eg. item is present at the end of create
command (unless something crashed). So even if I run the command again and
the item got already created by previous command, the item still present.
The user gets different response, but the state of the server is the same,
hence idempotency stands. Same applies to any other command including
'stratis fs destroy' - idempotency is not broken as long as the fs is not
present at the end of the command. Response to user does not matter to
idempotency.

So I think based on examples on what others do (dm, LVM...), 'stratis fs
remove' should actually warn user and fail if he tries to remove something
that is not there. It does not break idempotency and makes the CLI much
nicer to user.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1688262
[2] https://tools.ietf.org/html/rfc7231#section-4.2.2

[Anne Mulhern]

Hi!

I've thought about this in a slightly different way.

I'm more used to the more stringent definition of idempotency
that I learned very-long-ago in an OS class.
"An operation is idempotent when the effect of performing it multiple times is
equivalent to performing it once."
I pulled it from a recent textbook by the professor,
http://pages.cs.wisc.edu/~remzi/OSTEP/dist-nfs.pdf, I don't
still have my notes or anything like that.

So, I do think we would be giving up the idempotency that we've been striving for in
the CLI if we make the proposed
changes (by that definition of idempotency).

But there is also the consideration of user expectation, which is important.
Therefore, giving up idempotency in the CLI, in order to better accommodate
user expectation might be the correct thing to do. It will, perhaps, open
up new problems for users who script the CLI, however, so we should be
aware of that possibility.

Retaining idempotency in the D-Bus API is still a desirable goal.
If we issue a sequence of commands on the D-Bus and the D-Bus server
suffers a failure, then, if those commands are idempotent, we are
free to issue them again. This would be a good thing.

My concrete suggestion is the following:

1. Fix the bug in the "filesystem create" command.
If there is already a filesystem there, have it return
success on the D-Bus, but an empty list of filesystems created.

NOTE: StratPool::create_filesystems() should be defined so that it
creates any filesystem that does not have a conflicting name. It
is implemented to create multiple file systems. It is only called
w/ one filesystem spec, though, so always, under this situation
if there is a name conflict it should return Ok(vec![]). 

2. For each CLI call where the idempotency behavior conflicts
with user expectation, fix it up in the CLI. These are:
1. rename for pool and filesystem
2. destroy for pool and filesystem
3. create for filesystem
(Not for pool, because create for pool seemed too hard to make idempotent.
 Note that, according to my textbook, just because you can't meet the goal
 of idempotency in every case, that doesn't make useless the cases where you
 do. Even in NFS, mkdir is not idempotent. We should make sure one last
 time that create pool is too hard to make idempotent, though, before we
 leave it for good.)

How to fix it up in the CLI?

Take the function create_volumes, assuming that the D-Bus return
has been corrected in the manner I describe above as an example.

    @staticmethod
    def create_volumes(namespace):
        """
        Create volumes in a pool.

        :raises StratisCliEngineError:
        """
        proxy = get_object(TOP_OBJECT)
        managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {})
        (pool_object_path, _) = next(
            pools(props={
                'Name': namespace.pool_name
            }).require_unique_match(True).search(managed_objects))

        (_, rc, message) = Pool.Methods.CreateFilesystems(
            get_object(pool_object_path), {'specs': namespace.fs_name})

        if rc != StratisdErrors.OK:
            raise StratisCliEngineError(rc, message)


Get the list of object path/name pairs returned by the CreateFilesystems
call. If the names returned does not match the requested names,
raise an appropriate exception. I expect that a new type of exception
would have to be created for this purpose. Field the exception appropriately
in the run method and report the error.

This way, we retain idempotency as much as possible in the D-Bus API but
meet user expectations in the CLI.

- mulhern

----- Original Message -----
> From: "Jakub Krysl" <jkrysl(a)redhat.com&gt;
> To: stratis-devel(a)lists.fedorahosted.org
> Sent: Tuesday, March 19, 2019 11:08:41 AM
> Subject: [stratis-devel] idempotency versus/and user comfort
> 
> Hello all,
> 
> Recently I've raised an issue about user comfort when destroying stratis fs
> [1] which got rejected with idempotency as reason. I do not agree with this,
> let me explain why.
> 
> First let's start with the user comfort itself. It seems to be good practice
> to fail on item removal if the item user is trying to remove is not present.
> This can have many reasons varying from "item is already removed" to
"user
> made typo in item name". Here are some examples:
> device mapper:
> # dmsetup table
> test: 0 102400 linear 7:0 102400
> # dmsetup remove test
> # echo $?
> 0
> # dmsetup remove test
> device-mapper: remove ioctl on test failed: No such device or address
> Command failed.
> # echo $?
> 1
> LVM:
> # pvremove /dev/loop0
> Labels on physical volume "/dev/loop0" successfully wiped.
> # echo $?
> 0
> # pvremove /dev/loop0
> No PV found on device /dev/loop0.
> # echo $?
> 5
> 
> On the other hand Stratis CLI does not work this way:
> # stratis pool create test /dev/loop0
> # stratis fs create test test_fs
> # echo $?
> 0
> # stratis fs create test test_fs
> Execution failure caused by:
> ALREADY EXISTS: test_fs
> # echo $?
> 1
> # stratis fs destroy test test_fs
> # echo $?
> 0
> # stratis fs destroy test test_fs
> # echo $?
> 0
> 
> Now let's get to idempotency itself. Given the response in [1], command is
> idempotent only when the output is teh same. Based on the example above it
> seems that stratis create is actually not idempotent, as trying to create
> the same fs again fails. But I'd like to pinpoint the definition and
> application of idempotency first before settling this. I like the
> idempotency definition from RFC 7231 [2] (HTTP/1.1), mainly the end of the
> paragraph: "repeating the request will have the same intended effect, even
> if the original request succeeded, though the response might differ."
> Based on this it is actually idempotent to give user different response as
> long as the effect is the same, eg. item is present at the end of create
> command (unless something crashed). So even if I run the command again and
> the item got already created by previous command, the item still present.
> The user gets different response, but the state of the server is the same,
> hence idempotency stands. Same applies to any other command including
> 'stratis fs destroy' - idempotency is not broken as long as the fs is not
> present at the end of the command. Response to user does not matter to
> idempotency.
> 
> So I think based on examples on what others do (dm, LVM...), 'stratis fs
> remove' should actually warn user and fail if he tries to remove something
> that is not there. It does not break idempotency and makes the CLI much
> nicer to user.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1688262
> [2] https://tools.ietf.org/html/rfc7231#section-4.2.2
> 
> 
> _______________________________________________
> stratis-devel mailing list -- stratis-devel(a)lists.fedorahosted.org
> To unsubscribe send an email to stratis-devel-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...
>

[Tony Asleson]

On 3/19/19 10:08 AM, Jakub Krysl wrote:
>  So I think based on examples on what others do (dm, LVM...),
> 'stratis fs
>  remove' should actually warn user and fail if he tries to remove something
>  that is not there. It does not break idempotency and makes the CLI much
>  nicer to user. 
The Stratis CLI and corresponding API are a mishmash of non-idempotent
and idempotent behavior.  IMHO we should make them all non-idempotent
and make the CLI and API consistent.

Regards,
Tony