`fs rename` command for modifying template values #2743

joshmoore · 2014-07-03T11:32:32Z

When the FS template is changed (for example to make use of the new features in #2701), sysadmins may want to rearrange their old data to match the new template. The bin/omero fs rename Fileset:ID command now does that.

Testing as a regular user:

bin/omero import something
bin/omero fs sets # should list the latest fileset
bin/omero fs rename FILESET_ID # should rename it
you will be asked to move the files (e.g. as inplace_user) outside of OMERO.

Testing as admin:

as above, but the bin/omero fs rename step should use the user's name and not root's.
If --move is passed, then the files should be moved "server-side" with no user interaction.

In order for an admin user to be able to move files server-side, RawAccessRequest needed to add a `mv` operation. In addition, CheckedPath objects need to expose renameTo and moveToDir methods.

In order to produce a lambda which takes just an ID, a default can now be passed to proxy_to_instance which will then be used if the proxy string does not contain the "Class:" prefix as otherwise required.

Now `rename Fileset:ID` will rename the OriginalFile.path fields of a Fileset to match the current template. This is restricted to the owner of the Fileset for the moment.

* print out all paths to be moved * save fileset change in the same transaction

In order to write the integration tests, we need to have context objects passed everywhere. This will also be of use when an admin wants to rename a fileset for a user.

ximenesuk · 2014-07-09T14:28:54Z

A few comments from a brief run through:

Everything works as expected for both a user and an admin.
If I don't do the mv following the rename then bin/omero fs sets --check fails expectedly but isn't very useful.
I can rename the same id multiple times in a row without moving after each rename.
I noticed that the logfile stays put using the original name. I assume this is expected but it does break the expected naming scheme.

A couple of these are clearly idiot-cases but are perhaps easily possible if using shell history laxly. Is there any way to recover other than trawling through history to work out what happened?

ximenesuk · 2014-07-09T14:42:31Z

Incidentally, also tested with a change to config:

jrs-macbookpro-25031:ome cblackburn$ dist/bin/omero config set omero.fs.repo.path "%user%_%userId%//%year%-%month%-%day%/%time%"
...
jrs-macbookpro-25031:ome cblackburn$ dist/bin/omero fs rename 53 --move
...
Renaming Fileset:53 to user-0_2/2014-07-09/15-39-22.023/
Moving user-0_2/2014-07/09/15-37-09.186/Cats.png to user-0_2/2014-07-09/15-39-22.023/

where the dislocation of the logfile is more immediately apparent

joshmoore · 2014-07-09T15:25:35Z

NB: I'll add in a move of the log file as well, but I do start to fear what's going to happen when we allow adding random data into these directories!

joshmoore · 2014-07-09T15:39:44Z

If I don't do the mv following the rename then bin/omero fs sets --check fails expectedly but isn't very useful.

What would you expect to happen?

I can rename the same id multiple times in a row without moving after each rename.

Ditto.

ximenesuk · 2014-07-09T15:43:14Z

I think in the case of --check getting some results back. I.e. I get an exception rather than a table telling me I have a screwed up fileset.
I don't know. I just wondered if I could move what is effectively and empty fileset.

ximenesuk · 2014-07-09T16:17:45Z

What is the rationale for a non-admin user to use this command? What is the rationale for not enforcing the move by default?

joshmoore · 2014-07-09T16:29:14Z

What is the rationale for a non-admin user to use this command? What is the rationale for not enforcing the move by default?

For a user with in-place access, it could be useful; for anyone else, it's mostly dangerous. Having it admin-only would certainly be an option.

The reason for not doing the move by default was mostly not really trusting the code. Error conditions like "move fails in the middle" aren't handled (nor are they easily testable!), whereas a sysadmin would know how to handle move errors on the command-line. Happy to swap the defaults; just trying to be careful at the last minute. /cc @kennethgillen

`fs rename` followed by `fs sets --check` (without a move inbetween) was leading to a `CmdError` being printed before this change.

kennethgillen · 2014-07-09T16:50:53Z

👍 for @joshmoore's admin-only restriction and safer default.

joshmoore · 2014-07-09T17:01:30Z

@ximenesuk : urgh. Looking at the log files makes this all quite unseemly. Perhaps a chat tomorrow?

ximenesuk · 2014-07-09T17:41:20Z

@joshmoore Yes, for a chat.

In preparation for also specifying a tomove for the log file, we need to specify where the file should be moved *to*, since the log file is a directory up from the actual fileset.

joshmoore · 2014-07-10T07:32:59Z

Log file move pushed. Looking at one example:

.../ManagedRepository/tmpl_9734/2014-07/10$ find 09-26-58*
09-26-58.656
.../ManagedRepository/tmpl_9734/2014-07/10$ find 09-28-26.796*
09-28-26.796
09-28-26.796/mods.fake
09-28-26.796.log

ximenesuk · 2014-07-10T10:00:38Z

Looks good. Moves logfiles or asks for them to be moved. Fileset check works okay:

jrs-macbookpro-25031:ome cblackburn$ dist/bin/omero fs sets --check --offset 30 --limit 12
Using session 502dd3d0-c9b1-48b2-a3b4-54d1dd179597 (root@localhost:4064). Idle timeout: 10.0 min. Current group: system
 #  | Id  | Prefix                                                           | Images | Files | Transfer | Check  
----+-----+------------------------------------------------------------------+--------+-------+----------+--------
 0  | 116 | user-0_2/2014-07-09/16-09-24.615/                                | 1      | 3     |          | OK     
 1  | 115 | user-0_2/2014-07-09/16-09-22.739/                                | 1      | 4     |          | OK     
 2  | 114 | user-0_2/2014-07-09/16-07-41.539/                                | 4      | 203   |          | OK     
 3  | 113 | user-0_2/2014-07/10/10-51-47.014/                                | 73     | 175   |          | ERROR! 
 4  | 112 | user-0_2/2014-07/10/10-51-33.353/                                | 56     | 141   |          | OK     
 5  | 110 | root_0/2014-07-09/16-00-46.289/                                  | 1      | 2     |          | OK     
 6  | 108 | root_0/2014-07-09/16-00-34.585/                                  | 1      | 2     |          | OK     
 7  | 106 | bbe13664-d770-4007-8d58-30e045a745bf_60/2014-07-09/16-00-23.247/ | 1      | 2     |          | ERROR! 
 8  | 104 | 52b513d2-9f4b-47b2-b535-b66b13b495ee_56/2014-07-09/16-00-07.017/ | 1      | 2     |          | OK     
 9  | 102 | 727a8d68-f096-43d2-9a80-b9c8c3d7f730_53/2014-07-09/15-59-23.508/ | 1      | 2     |          | ERROR! 
 10 | 57  | user-0_2/2014-07/09/14-45-33.692/                                | 1      | 1     |          | OK     
 11 | 56  | user-0_2/2014-07/09/14-44-30.624/                                | 1      | 1     |          | OK     
(12 rows, starting at 30 of approx. 53)

The first error here was a lei fileset not moved when renamed. A later move-by-hand fixed that fine.

And non-admin users get a senible reply to admin options:

jrs-macbookpro-25031:ome cblackburn$ dist/bin/omero -C fs sets --check
Server: [localhost]
Username: [root]user-0
Password:
Created session e392af17-1d8b-4c8a-acdb-3b18ced914b9 (user-0@localhost:4064). Idle timeout: 10.0 min. Current group: pg-0
SecurityViolation: Admins only!

Any further commits to come or aspects that need reviewing?

joshmoore · 2014-07-10T10:55:47Z

Probably just a decision about who is permitted and what are the defaults a la #2743 (comment) Post-lunch chat?

joshmoore · 2014-07-10T17:22:28Z

Thoughts on talking to @ximenesuk:

attach a history annotation to each fileset that's moved
printing out 100s to 1000s lines of "move" statements in the console is bad both for collecting the move commands and for long-running server actions so, move the directory rather than individual files which leads to only 2 mv commands being printed
possibly print a warning that the move may not be atomic and remove the --move action.
probably a "bulk rename" will be requested early on (could conceivably be tested on octopus /cc @sbesson @pwalczysko) though perhaps anyone who wants something more sophisticated should use OMERO.scripts.

In order to move the top level directory, first it was necessary to check that the empty directory would be removed from disk.

There are a number of error conditions where neither the handle nor the callback object from client.submit() will be returned to the user. Since the exceptions do not include the objects and it was not a part of the "API" to require a `close()` we will make a best effort at cleaning up resources when an exception occurs.

Previous changes to waitOnCmd and submit in BaseClient broke backwards-compatibility since the handle which was returned could possibly be closed. Now, close is called on the handle instance by default only if the invoker is `client.submit()` since in that case, the consumer has never held a reference to the handle proxy.

The addition of a new annotation with a different namespace was causing filesets to no longer be shown by `bin/omero fs sets`

joshmoore · 2014-07-14T18:38:53Z

Ready for re-review. (Move is now by default)

sbesson · 2014-07-15T10:33:22Z

components/tools/OmeroPy/test/integration/test_repository.py

@@ -652,8 +652,8 @@ def testCmdDeleteCantDeleteDirectories(self):
        gateway = BlitzGateway(client_obj=self.client)
        handle = gateway.deleteObjects("/OriginalFile", [id])
        try:
-            pytest.raises(Exception,
-                    gateway._waitOnCmd, handle, failonerror=True)
+            with pytest.raises(CmdError):


Missing from omero import CmdError.
I will try to flake8 the remaining integration tests in between RCs.

ximenesuk · 2014-07-15T11:25:50Z

It all functions as expected with these revisions. With the final commit merged in all the modified and added tests pass.

joshmoore · 2014-07-15T12:05:50Z

Waiting on travis to be green and then will merge.

`fs rename` command for modifying template values

joshmoore · 2014-07-15T13:11:48Z

--rebased-to #2805

joshmoore added 10 commits July 3, 2014 13:05

Add 'mv' support to RawAccessRequest

0ad47a0

In order for an admin user to be able to move files server-side, RawAccessRequest needed to add a `mv` operation. In addition, CheckedPath objects need to expose renameTo and moveToDir methods.

First draft of renaming test

0d0dd26

Default command help to fun.__doc__

c5ce4be

Refactor prep_directory to fs.py

b5a4a79

Refactor rename_fileset to fs.py

ab90751

Add 'default' argument to proxy_to_instance

2362d68

In order to produce a lambda which takes just an ID, a default can now be passed to proxy_to_instance which will then be used if the proxy string does not contain the "Class:" prefix as otherwise required.

Add ProxyStringType for use with the CLI

b92ebec

First (non-admin) version of 'rename'

e0d3ed7

Now `rename Fileset:ID` will rename the OriginalFile.path fields of a Fileset to match the current template. This is restricted to the owner of the Fileset for the moment.

FS modification improvements

985c1c7

* print out all paths to be moved * save fileset change in the same transaction

Have 'rename' print out the mv command

eaa3c4a

joshmoore added the dev_5_0 label Jul 4, 2014

joshmoore added 4 commits July 8, 2014 15:01

test_rename: multi-user integration tests

641f0f8

In order to write the integration tests, we need to have context objects passed everywhere. This will also be of use when an admin wants to rename a fileset for a user.

rename-as functionality

3d4ad89

Use raw 'mv' as admin

7796158

Fix flake8 whitespace errors

ab50d4f

Handle errors on submit(raw)

b343acc

`fs rename` followed by `fs sets --check` (without a move inbetween) was leading to a `CmdError` being printed before this change.

joshmoore added 3 commits July 10, 2014 09:18

Refactor 'tomove' return to contain target

dfa1e46

In preparation for also specifying a tomove for the log file, we need to specify where the file should be moved *to*, since the log file is a directory up from the actual fileset.

Also print CmdError on --move

96d7ab9

Move log file as well

237a179

joshmoore added 2 commits July 10, 2014 10:28

Fix flake8 'OK import' err

52dc1d5

Remove 'import pdb'

bdb287c

joshmoore added 12 commits July 10, 2014 20:28

Add annotation on rename

d2e89e9

Disable --move and make rename admin only

61be315

Fix flake8 error

b77382d

Test directory delete

b1a5614

In order to move the top level directory, first it was necessary to check that the empty directory would be removed from disk.

Move dir deletion into prep_directory

9218276

Include only dir and log in tomove

703792c

Fix test_fs after changes to get_fileset

a53ca95

Fix flake8 err

de0507d

Fix "missing" filesets

43b8b3f

The addition of a new annotation with a different namespace was causing filesets to no longer be shown by `bin/omero fs sets`

Fix flake8 whitespace issues

55dbfa0

Move by default

2582cb7

sbesson reviewed Jul 15, 2014
View reviewed changes

Add CmdError import

bea62ab

joshmoore added a commit that referenced this pull request Jul 15, 2014

Merge pull request #2743 from joshmoore/template-mods

f1aea4c

`fs rename` command for modifying template values

joshmoore merged commit f1aea4c into ome:dev_5_0 Jul 15, 2014

joshmoore deleted the template-mods branch July 15, 2014 13:10

joshmoore mentioned this pull request Jul 15, 2014

fs rename command for modifying template values (rebased onto develop) #2805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fs rename` command for modifying template values #2743

`fs rename` command for modifying template values #2743

joshmoore commented Jul 3, 2014

ximenesuk commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 9, 2014

joshmoore commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 9, 2014

kennethgillen commented Jul 9, 2014

joshmoore commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 10, 2014

ximenesuk commented Jul 10, 2014

joshmoore commented Jul 10, 2014

joshmoore commented Jul 10, 2014

joshmoore commented Jul 14, 2014

sbesson Jul 15, 2014

joshmoore Jul 15, 2014

ximenesuk commented Jul 15, 2014

joshmoore commented Jul 15, 2014

joshmoore commented Jul 15, 2014

fs rename command for modifying template values #2743

fs rename command for modifying template values #2743

Conversation

joshmoore commented Jul 3, 2014

ximenesuk commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 9, 2014

joshmoore commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 9, 2014

kennethgillen commented Jul 9, 2014

joshmoore commented Jul 9, 2014

ximenesuk commented Jul 9, 2014

joshmoore commented Jul 10, 2014

ximenesuk commented Jul 10, 2014

joshmoore commented Jul 10, 2014

joshmoore commented Jul 10, 2014

joshmoore commented Jul 14, 2014

sbesson Jul 15, 2014

Choose a reason for hiding this comment

joshmoore Jul 15, 2014

Choose a reason for hiding this comment

ximenesuk commented Jul 15, 2014

joshmoore commented Jul 15, 2014

joshmoore commented Jul 15, 2014

`fs rename` command for modifying template values #2743

`fs rename` command for modifying template values #2743