search

open-iscsi / rtslib-fb

Python library for configuring the Linux kernel-based multiprotocol SCSI target (LIO)
Apache License 2.0
72 stars 90 forks source link

saveconfig: support saving configuration at block granularity #123

Closed pkalever closed 6 years ago

pkalever commented 6 years ago

Add an option to update configuration for single block at a time. As of today changes done for one single block needs the whole configuration update.

This patch introduces a new option 'block_name' to saveconfig command

$ targetcli help [...] AVAILABLE COMMANDS [...]

Batch mode:

$ targetcli / saveconfig block_name=block1

Interactive mode:

$ targetcli targetcli shell version 2.1.fb48 Copyright 2011-2013 by Datera, Inc and others. For help on commands, type 'help'.

/> / saveconfig block_name=block1 Last 26 configs saved in /etc/target/backup. Configuration saved to /etc/target/saveconfig.json

Signed-off-by: Prasanna Kumar Kalever prasanna.kalever@redhat.com

pkalever commented 6 years ago

This change needs https://github.com/open-iscsi/targetcli-fb/pull/104

pkalever commented 6 years ago

review-request: @agrover

agrover commented 6 years ago

In order to understand better so I can evaluate this PR, can you tell me a little bit more how the current code is causing you issues? This will also help me to know if other parts of the saved state might need similar capability.

pkalever commented 6 years ago

Sure, here is the link to bug [1] that we have seen.

Let me try to pick a different example and explain:

  1. Create 2 user:glfs storageObjects each on a different volume (say block1 on vol1 and block2 on vol2) and run targetcli / saveconfig
  2. Say we had to face a node reboot, followed by target restorconfig
  3. While the restoreconfig is issued say vol1 is down for all practical reasons
  4. On RHEL, the behavior is since the vol1 is down storageObject block1 will not be loaded as tcmu-runner-> handle_netlink -> add_device()->tcmu_glfs_open() failed. So here 'targetcli ls' list Storage Objects = 1 (only block2), and Targets=2.
  5. If some one issues a saveconfig soon or later after creating/modifying some more storage objects. This saveconfig will update the saveconfig.json file with removed block1 StorageObject in it.

Which will then result in loss of the Storage Object[s]. I really want the reader to think like what happens we hit this case while we scale at 100's of StorageObjects on a node having multiple volumes (few down).

If we have a granular saveconfig option at storageObject level we really don't have to worry about overriding of other storageObjects(if their underline storage is down) while we wish to save the current StorageObject set.

@agrover Hope that make-sense. Thanks!

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1524791

pkalever commented 6 years ago

I have updated this PR with the suggested changes. Thanks!

maurizio-lombardi commented 6 years ago

Hi @pkalever , I looked at the patch and I have some observations:

  1. If there is a saveconfig at the backstore level, IMO there should be a saveconfig at the target level too (this could be added later however)

  2. -    def ui_command_saveconfig(self, savefile=default_save_file):
+    def ui_command_saveconfig(self, savefile=default_save_file, block_name=None):

What doesn't totally convince me of this solution is that if you add partial saves from other spots in the tree you will have to add more optional parameters, maybe it tends to become a bit too messy and confused?

Also, IMO it should be called "storageobj_name", not "block_name" since it is generic to all kind of storage objects, no?

  1. How can you be sure you are saving the correct storage object by using only its name? Suppose you have something like this:

/> ls / o- / ......................................................................................................................... [...] o- backstores .............................................................................................................. [...] | o- fileio ................................................................................................... [1 Storage Object] | | o- disk ...................................................................................... [/home/disk (1.0M) deactivated] | o- iblock ................................................................................................... [1 Storage Object] | | o- disk ................................................................................................. [/dev/sda activated]



Won't it end up saving the fileio object instead of the iblock one?
pkalever commented 6 years ago

Hi @pkalever , I looked at the patch and I have some observations:

Hi @maurizio-lombardi Thanks for your thoughts about the PR's.

If there is a saveconfig at the backstore level, IMO there should be a saveconfig at the target level too (this could be added later however)

The code in the patch i.e. saveconfig at the Storage Object level (_get_saveconf) dive into the related target too and if it finds that there is a relevant target Object associated with the specified Storage Object, it saves info of the target Object too.

Well! that said, I have no objections in having the saveconfig at storage object and target object independently, but

Can there be a target Object without storage Object in an ideal case ?

- def ui_command_saveconfig(self, savefile=default_save_file): + def ui_command_saveconfig(self, savefile=default_save_file, block_name=None): What doesn't totally convince me of this solution is that if you add partial saves from other spots in the tree you will have to add more optional parameters, maybe it tends to become a bit too messy and confused?

Make-sense to me as well. I have removed this in the updated version.

Also, IMO it should be called "storageobj_name", not "block_name" since it is generic to all kind of storage objects, no?

I have changed the variable name to 'so_path' (internal to code) and removed 'block_name' from ui_command_saveconfig()

so with updated version of patches we don't support $ targetcli / saveconfig block_name=block1

We only support $ targetcli /backstores/user:glfs/block1 saveconfig

How can you be sure you are saving the correct storage object by using only its name? Suppose you have something like this:

/> ls / o- / ......................................................................................................................... [...] o- backstores .............................................................................................................. [...] | o- fileio ................................................................................................... [1 Storage Object] | | o- disk ...................................................................................... [/home/disk (1.0M) deactivated] | o- iblock ................................................................................................... [1 Storage Object] | | o- disk ................................................................................................. [/dev/sda activated]

Won't it end up saving the fileio object instead of the iblock one?

This is perfectly valid point and a good catch too. I have changed the code to check for '/backstores/so.plugin/so.name' instead of just so.name.

All are very good points :-)

please find the updated patches https://github.com/open-iscsi/rtslib-fb/pull/123/commits/418507651ee9c71718c1a435b81e16f01238e3a4 (rtslib) & https://github.com/open-iscsi/targetcli-fb/pull/104/commits/9dc85912f6ff2ba77fa8b136478ae3d2776c2b5e (targetcli) addressing all the review comments (except saveconfig at target Object level).

Thanks a lot.