)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"01cc160d7b346902567efdec6fea350b7b166174","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"228b1d5b_056f4390","updated":"2022-05-13 16:02:41.000000000","message":"Hi Stephen,\n\nThere were a bunch of concerns with this and the major one being the direction and work not being discussed prior with the cinder team hence the following concerns:\n\n1) There is no core in openstackclient that has the authority to monitor/manage/merge code in that repository so removing our client (where we have full authority) in support of openstackclient (where we have no authority) seems not reasonable\nThe current proposal is to have at least one core from cinder team in openstackclient core team\n\n2) We are still not clear if microversions work as we expect them to be\nWe don\u0027t know if microversion features work and there are some of the microversion feature missing support, which i think is being worked on but not done yet so doesn\u0027t make much sense to deprecate cinderclient until we have all command/functionality support in OSC\nThere was also a concern regarding 3.latest not being supported\n\n3) The naming of commands does not match well with the actual functionality they serve\nEg:\na) volume create requires name as a positional parameter and size as an optional parameter whereas it should be the opposite i.e. size as positional and name as optional (as we have in cinderclient)\nb) There are a bunch of cinder operations clubbed inside the ``openstack volume set ...`` command which have unrelated functionalities\nc) ``cinder unmanage`` or ``volume delete –remote`` is the opposite of what it is doing, it is removing DB records of a volume that still exists in the storage array\n\nI recommend OSC team to attend the midcycle[1] (June 1st) and discuss properly the idea behind the efforts and also answer all the concerns that the cinder team has.\n\n[1] https://etherpad.opendev.org/p/cinder-zed-midcycles","commit_id":"59f4fdc3ec7f240c5ae86bd868b60a901da10809"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"455efe324c87a81f7abdd99e08d5bfb16b4cc0b2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4c4c8ecb_506bd351","in_reply_to":"228b1d5b_056f4390","updated":"2022-05-16 11:12:24.000000000","message":"\u003e Hi Stephen,\n\nI will preface all of the below with the note that we\u0027re not deleting anything here nor do I recommend doing so for some time. I am however suggesting that all new development should occur in OSC. Continuing to invest in cinderclient is wasted effort that pushes out OSC adoption even further.\n\n\u003e There were a bunch of concerns with this and the major one being the direction and work not being discussed prior with the cinder team hence the following concerns:\n\u003e \n\u003e 1) There is no core in openstackclient that has the authority to monitor/manage/merge code in that repository so removing our client (where we have full authority) in support of openstackclient (where we have no authority) seems not reasonable\n\u003e The current proposal is to have at least one core from cinder team in openstackclient core team\n\nWe\u0027d love to have more cores in OSC and SDK. Please feel free to suggest someone that you think would make sense and we\u0027ll happily add them. All we ask is that you run wholly new commands by one of the other cores (e.g. gtema or I) until you get a feel for how we expect commands to be structured. It would also be great if this person/people would invest some effort in closing gaps, of course 😊 (it\u0027s mostly been me and some interns so far).\n\n\u003e 2) We are still not clear if microversions work as we expect them to be\n\u003e We don\u0027t know if microversion features work and there are some of the microversion feature missing support, which i think is being worked on but not done yet so doesn\u0027t make much sense to deprecate cinderclient until we have all command/functionality support in OSC\n\nMicroversions have worked for at least two or three cycles now. You can validate this locally of course - just look for commands with \"needs --os-volume-api-version 3.N\" in their help text and try use those). For example:\n\n  $ openstack volume attachment create test-volume test.server --mode ro\n  --os-volume-api-version 3.27 or greater is required to support the \u0027volume attachment create\u0027 command\n\n  $ openstack --os-volume-api-version 3.27 volume attachment create test-volume test.server --mode ro                                                                                                                \n--os-volume-api-version 3.54 or greater is required to support the \u0027--mode\u0027 option\n\n  $ openstack --os-volume-api-version 3.54 volume attachment create test-volume test.server --mode ro\n  +-------------+-------+\n  | Field       | Value |\n  +-------------+-------+\n  | ID          |       |\n  | Volume ID   |       |\n  | Instance ID |       |\n  | Status      |       |\n  | Attach Mode |       |\n  | Attached At |       |\n  | Detached At |       |\n  | Properties  |       |\n  +-------------+-------+\n\n(the output of that is clearly wrong - looking at why now)\n\nThere are definitely gaps here but these are easily closed (we got most if not all of the gaps in nova, which has a much larger API, in one cycle).\n\n\u003e There was also a concern regarding 3.latest not being supported\n\nCorrect, this doesn\u0027t work currently. However, it does work for novaclient so it\u0027s just a case of copy some code between openstack.compute and openstack.volume. I haven\u0027t prioritised this though because we hope to move everything to sdk which does auto-negotiation of versions, making this unnecessary.\n\n\u003e 3) The naming of commands does not match well with the actual functionality they serve\n\nThis is a more fundamental thing. OSC\u0027s job is to unify the various OpenStack APIs and present them in a unified manner. This means hiding the idiosyncrasies of each individual API where possible. This means OSC\u0027s command won\u0027t always align with what the underlying API is called but that\u0027s perfectly okay: the cinder/nova/neutron developers might think their particular approach/naming scheme is the best one but having 10 different names for similar things is a bad idea for users and we\u0027re providing a user-facing tool here. A sprinkling of syntactic sugar is a good thing.\n\n\u003e Eg:\n\u003e a) volume create requires name as a positional parameter and size as an optional parameter whereas it should be the opposite i.e. size as positional and name as optional (as we have in cinderclient)\n\n\u0027--size\u0027 is only required if you\u0027re creating a blank volume or a volume based on an image. If you\u0027re creating a volume based on an existing volume or snapshot, it obviously isn\u0027t required. Nor will it be required when we add support for creating a new cinder volume from an existing volume on the storage backend (i.e. what cinder calls \"managing\"). You\u0027re right that \u0027name\u0027 could be an optional posarg though. Patches welcome!\n\n\u003e b) There are a bunch of cinder operations clubbed inside the ``openstack volume set ...`` command which have unrelated functionalities\n\n\u0027$resource set\u0027 is common OSC lingo. Almost anything that modifies an existing resource should be grouped in the \u0027set\u0027 and \u0027unset\u0027 commands. The main exceptions are things that affect multiple resources (e.g. add a volume to server). You\u0027ll notice that many nova things are designed the same way (e.g. \u0027server set\u0027). Things are working as expected here IMO.\n\n\u003e c) ``cinder unmanage`` or ``volume delete –remote`` is the opposite of what it is doing, it is removing DB records of a volume that still exists in the storage array\n\nWhat command are you referring to here? I don\u0027t see a \u0027--remote\u0027 argument for the \u0027volume delete\u0027 command.\n\n\u003e I recommend OSC team to attend the midcycle[1] (June 1st) and discuss properly the idea behind the efforts and also answer all the concerns that the cinder team has.\n\nSure, I can do that. We\u0027re also at the Forum if anyone is attending Summit.\n\n\u003e [1] https://etherpad.opendev.org/p/cinder-zed-midcycles","commit_id":"59f4fdc3ec7f240c5ae86bd868b60a901da10809"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"a94485f9e358b4639479a9bc51f33741169bc0df","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0e21a4fd_3a16a8cf","in_reply_to":"4c4c8ecb_506bd351","updated":"2022-05-17 19:42:08.000000000","message":"\u003e We\u0027d love to have more cores in OSC and SDK. Please feel free to suggest someone that you think would make sense and we\u0027ll happily add them. All we ask is that you run wholly new commands by one of the other cores (e.g. gtema or I) until you get a feel for how we expect commands to be structured. It would also be great if this person/people would invest some effort in closing gaps, of course 😊 (it\u0027s mostly been me and some interns so far).\n\nAck, good to know. We can ask if any core would like to volunteer when we discuss this with the team.\n\n\u003e Microversions have worked for at least two or three cycles now.\n\nThis has been one of the major concerns to avoid shifting to OSC but good to know that it is resolved now.\n\n\u003e This is a more fundamental thing. OSC\u0027s job is to unify the various OpenStack APIs and present them in a unified manner. This means hiding the idiosyncrasies of each individual API where possible. This means OSC\u0027s command won\u0027t always align with what the underlying API is called but that\u0027s perfectly okay: the cinder/nova/neutron developers might think their particular approach/naming scheme is the best one but having 10 different names for similar things is a bad idea for users and we\u0027re providing a user-facing tool here. A sprinkling of syntactic sugar is a good thing.\n\nI\u0027m not referring to naming schemes suggested by teams but more of my concern lies is if the command name is able to convey what the operation does. Some of the commands doesn\u0027t give it away and are confusing and some are totally opposite of the operation we are performing (as mentioned in my example later in this comment).\n\n\u003e \u0027--size\u0027 is only required if you\u0027re creating a blank volume or a volume based on an image. If you\u0027re creating a volume based on an existing volume or snapshot, it obviously isn\u0027t required. Nor will it be required when we add support for creating a new cinder volume from an existing volume on the storage backend (i.e. what cinder calls \"managing\"). You\u0027re right that \u0027name\u0027 could be an optional posarg though. Patches welcome!\n\nThat is partially True. In some cases, users don\u0027t want to create a same size copy of a snapshot/volume/backup which they created in the past. Providing the size in these cases results in a volume being created of the given size and not as the same size as the source resource which is a real use case.\nEg:\ncinder create 1 # creates 1 GB volume\ncinder create --source-volid \u003csource-volid\u003e 5 # creates a 5 GB volume which is also a clone of the original 1GB volume\n\nSo in conclusion, in these cases as well ``size`` is a valid argument. I agree that currently cinderclient doesn\u0027t mandate the size field for creating a volume from other source resources and I would be very much happy to see the same behavior in openstackclient rather than the name field as a positional argument.\n\n\u003e \u0027$resource set\u0027 is common OSC lingo. Almost anything that modifies an existing resource should be grouped in the \u0027set\u0027 and \u0027unset\u0027 commands. The main exceptions are things that affect multiple resources (e.g. add a volume to server). You\u0027ll notice that many nova things are designed the same way (e.g. \u0027server set\u0027). Things are working as expected here IMO.\n\nThis we can discuss more but there is a difference between setting a volume description (non-admin action) and resetting a volume state in DB (admin only action) which both lie under the same ``set`` subcommand.\n\n\u003e What command are you referring to here? I don\u0027t see a \u0027--remote\u0027 argument for the \u0027volume delete\u0027 command.\n\nIf you see the volume unmanage command in the doc here[1], the ``volume delete --remote`` looks like we are removing a volume at a remote location in the storage array whereas what we are actually doing is removing the DB entry of the volume keeping the actual volume in the storage array.\n\ncinderclient: unmanage\n\nOSC: volume delete –remote\n\nThis is an example of what i was referring above regarding naming of the commands which convey a wrong meaning.\n\n[1] https://docs.openstack.org/python-openstackclient/latest/cli/decoder.html","commit_id":"59f4fdc3ec7f240c5ae86bd868b60a901da10809"}]}