This is another instance where the CLI is idempotent, but that doesn't seem user-friendly. We should make a general non-idempotency issue, and fold all similar cases into it.

Upstream issue: https://github.com/stratis-storage/project/issues/51.

Upstream PRs:
https://github.com/stratis-storage/stratisd/pull/1597
https://github.com/stratis-storage/stratis-cli/pull/297

Hi Jakub, I have been working on this issue and want to verify that the behavior I'm implementing meets expectations for the CLI. Currently, I have no output and a 0 exit code when the action is completed successfully. In the error case, I have the following output and a 1 exit code:

stratis-cli issued a command that had no effect: The command 'AddDataDevs' has already been completed for resource or resources '['/dev/sda']'

Let me know if something else would be clearer or generally more intuitive before we merge my changes.

(In reply to John Baublitz from comment #4)
> Hi Jakub, I have been working on this issue and want to verify that the
> behavior I'm implementing meets expectations for the CLI. Currently, I have
> no output and a 0 exit code when the action is completed successfully. In
> the error case, I have the following output and a 1 exit code:
> 
> stratis-cli issued a command that had no effect: The command 'AddDataDevs'
> has already been completed for resource or resources '['/dev/sda']'
> 
> Let me know if something else would be clearer or generally more intuitive
> before we merge my changes.

Hi John,

Thanks for reaching out. I like how verbose it is and that it covers the possibility of adding more at once. I have a question regarding that: What happens if I try to add multiple devices where some of them are already added? Does it try to 'AddDataDevs' for each of them and list only those failed ones or does it return at the first fail and list them all (which is wrong) or just the single one (at which case it should not state 'resources')?

And a note regarding the command 'AddDataDevs': The user hits this error only by running stratis-cli command "stratis pool add-data test_pool /dev/sda", is that correct? In that case I think it should link to the command and not internal stratis-cli function 'AddDataDev' which is hidden to user. Returning something known to user instead of something unknown seems like better practise to me, prevents confusion ("What is 'AddDataDev', how did I call it?"). So my suggestion would be to either somehow reword it to link it to 'add-data' called by user OR just state something similar to:
"stratis-cli issued a command that had no effect: Resource or resources '['/dev/sda']' is already part of stratis pool X"
/
"stratis-cli issued a command that had no effect: Stratis pool X already contains resource or resources '['/dev/sda']'".

(In reply to Jakub Krysl from comment #5) 
> Hi John,
> 
> Thanks for reaching out. I like how verbose it is and that it covers the
> possibility of adding more at once. I have a question regarding that: What
> happens if I try to add multiple devices where some of them are already
> added? Does it try to 'AddDataDevs' for each of them and list only those
> failed ones or does it return at the first fail and list them all (which is
> wrong) or just the single one (at which case it should not state
> 'resources')?

Hi Jakub, given that we currently have enough information to provide warning messages about some devices already being added, I'm happy to put that together. As it stands at the moment, any command that adds any new data/cache devices will simply succeed with no output even if some devices have already been added. I can see how that may be confusing so I'm happy to discuss with the team about how we want to approach warning messages for the devices that have already been added if that sounds better. 

> 
> And a note regarding the command 'AddDataDevs': The user hits this error
> only by running stratis-cli command "stratis pool add-data test_pool
> /dev/sda", is that correct? In that case I think it should link to the
> command and not internal stratis-cli function 'AddDataDev' which is hidden
> to user. Returning something known to user instead of something unknown
> seems like better practise to me, prevents confusion ("What is 'AddDataDev',
> how did I call it?"). So my suggestion would be to either somehow reword it
> to link it to 'add-data' called by user OR just state something similar to:
> "stratis-cli issued a command that had no effect: Resource or resources
> '['/dev/sda']' is already part of stratis pool X"
> /
> "stratis-cli issued a command that had no effect: Stratis pool X already
> contains resource or resources '['/dev/sda']'".

I can most definitely change the command text as that's a fairly simple change to make. I'll use the CLI command as opposed to the DBus command as that does seem to be more intuitive.

Thank you for your feedback, and let me know if you have any other concerns.

Hi Jakub, I've settled on two separate types of behavior, one for single resource operations and one for multiple resource operations.

For commands that operate on a single resource like pool create, I emit the following error message if no changes are applied:

$ stratis pool create test_pool /dev/sda
Execution failed:
stratis-cli issued a command that had no effect: The command 'create' has already been completed for resource 'test_pool'

For commands that operate on multiple resources like pool add-cache, I emit the following error message if *any* of the provided blockdevs already exist in the pool:

$ stratis pool add-data test_pool /dev/sda /dev/sdb
Execution failed:
stratis-cli issued a command that could not be completely applied: The command 'add-data' has already been completed for resource or resources ['/dev/sda'] but not yet for resource or resources ['/dev/sdb']

This is different behavior from what was discussed before mainly because we decided this should return an error on the CLI side, but there should be no partial application of resources that would could be added if there were also some that could not be added. It seems like it would be very confusing to have some actions performed successfully while simultaneously returning an error code.

Does the outlined behavior sound like it would be intuitive to you when using the CLI?

(In reply to John Baublitz from comment #7)
> Hi Jakub, I've settled on two separate types of behavior, one for single
> resource operations and one for multiple resource operations.
> 
> For commands that operate on a single resource like pool create, I emit the
> following error message if no changes are applied:
> 
> $ stratis pool create test_pool /dev/sda
> Execution failed:
> stratis-cli issued a command that had no effect: The command 'create' has
> already been completed for resource 'test_pool'
> 
> For commands that operate on multiple resources like pool add-cache, I emit
> the following error message if *any* of the provided blockdevs already exist
> in the pool:
> 
> $ stratis pool add-data test_pool /dev/sda /dev/sdb
> Execution failed:
> stratis-cli issued a command that could not be completely applied: The
> command 'add-data' has already been completed for resource or resources
> ['/dev/sda'] but not yet for resource or resources ['/dev/sdb']
> 
> This is different behavior from what was discussed before mainly because we
> decided this should return an error on the CLI side, but there should be no
> partial application of resources that would could be added if there were
> also some that could not be added. It seems like it would be very confusing
> to have some actions performed successfully while simultaneously returning
> an error code.
> 
> Does the outlined behavior sound like it would be intuitive to you when
> using the CLI?

Hi John,

I like the proposed messages and I agree with you that it would be confusing to perform any action when the command fails.

Mass migration to Guangwu.

Hello


test pass with stratis-cli-2.0.0-1.el8 so move to verified 

[root@storageqe-24 ~]# systemctl restart stratisd
[root@storageqe-24 ~]# stratis pool create test_pool /dev/sdb
[root@storageqe-24 ~]# stratis pool add-data test_pool /dev/sdb
Execution failed:
You issued a command that would have a partial or no effect: The 'add to data' action has no effect for resource /dev/sdb
[root@storageqe-24 ~]# stratis pool add-data test_pool /dev/sdc
Execution failed:
stratisd failed to perform the operation that you requested. It returned the following information via the D-Bus: ERROR: Device /dev/sdc has an existing signature ID_FS_UUID=b876566e-e111-4092-b365-802feac557ed ID_FS_UUID_ENC=b876566e-e111-4092-b365-802feac557ed ID_FS_TYPE=xfs ID_FS_USAGE=filesystem.
[root@storageqe-24 ~]# 
[root@storageqe-24 ~]# 
[root@storageqe-24 ~]# stratis pool add-data test_pool /dev/sdc
Execution failed:
stratisd failed to perform the operation that you requested. It returned the following information via the D-Bus: ERROR: Device /dev/sdc has an existing signature ID_FS_UUID=b876566e-e111-4092-b365-802feac557ed ID_FS_USAGE=filesystem ID_FS_TYPE=xfs ID_FS_UUID_ENC=b876566e-e111-4092-b365-802feac557ed.
[root@storageqe-24 ~]# lsblk
NAME                                                                    MAJ:MIN RM  SIZE RO TYPE    MOUNTPOINT
sda                                                                       8:0    0  5.5T  0 disk    
├─sda1                                                                    8:1    0    1M  0 part    
├─sda2                                                                    8:2    0    1G  0 part    /boot
└─sda3                                                                    8:3    0  5.5T  0 part    
  ├─rhel_storageqe--24-root                                             253:0    0   50G  0 lvm     /
  ├─rhel_storageqe--24-swap                                             253:1    0 31.5G  0 lvm     [SWAP]
  └─rhel_storageqe--24-home                                             253:2    0  5.4T  0 lvm     /home
sdb                                                                       8:16   0  5.5T  0 disk    
└─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-physical-originsub 253:3    0  5.5T  0 stratis 
  ├─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-flex-thinmeta    253:4    0  5.6G  0 stratis 
  │ └─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-thinpool-pool  253:7    0  5.5T  0 stratis 
  ├─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-flex-thindata    253:5    0  5.5T  0 stratis 
  │ └─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-thinpool-pool  253:7    0  5.5T  0 stratis 
  └─stratis-1-private-a5d7c6ee26f242bcb3a5fbc8fa66eb7f-flex-mdv         253:6    0   16M  0 stratis 
sdc                                                                       8:32   0  5.5T  0 disk    
sdd                                                                       8:48   0  5.5T  0 disk    
sde                                                                       8:64   0  5.5T  0 disk    
sdf                                                                       8:80   0  5.5T  0 disk    
sdg                                                                       8:96   0  5.5T  0 disk    
sdh                                                                       8:112  0  5.5T  0 disk    
sdi                                                                       8:128  0  5.5T  0 disk    
sdj                                                                       8:144  0  5.5T  0 disk    
sdk                                                                       8:160  0  5.5T  0 disk    
└─sdk1                                                                    8:161  0   10G  0 part    
sdl                                                                       8:176  0  9.8G  0 disk 

[root@storageqe-24 ~]# dd if=/dev/zero of=/dev/sdc bs=1M count=10
10+0 records in
10+0 records out
10485760 bytes (10 MB, 10 MiB) copied, 0.0579613 s, 181 MB/s
[root@storageqe-24 ~]# stratis pool add-data test_pool /dev/sdc
[root@storageqe-24 ~]# dd if=/dev/zero of=/dev/sdd bs=1M count=10
10+0 records in
10+0 records out
10485760 bytes (10 MB, 10 MiB) copied, 0.25584 s, 41.0 MB/s
[root@storageqe-24 ~]# 
[root@storageqe-24 ~]# stratis pool add-data test_pool /dev/sdc
Execution failed:
You issued a command that would have a partial or no effect: The 'add to data' action has no effect for resource /dev/sdc
[root@storageqe-24 ~]# stratis pool add-cache  test_pool /dev/sdd
[root@storageqe-24 ~]# stratis pool add-cache  test_pool /dev/sdd
Execution failed:
You issued a command that would have a partial or no effect: The 'add to cache' action has no effect for resource /dev/sdd
[root@storageqe-24 ~]# 

[root@storageqe-24 ~]# stratis pool add-cache  test_pool /dev/sdd /dev/sde
Execution failed:
You issued a command that would have a partial or no effect: The 'add to cache' action has no effect for resource /dev/sdd but does for resource /dev/sde
[root@storageqe-24 ~]# stratis pool add-data  test_pool /dev/sdd /dev/sde
Execution failed:
You issued a command that would have resulted in including a block device in both cache and data tiers: The block device /dev/sdd would be added to the Data tier but is already in use in the Cache tier
[root@storageqe-24 ~]#

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:1634