Community
Participate
Working Groups
When running an installation the native actions performs unzip, cleanupzip and ln in a way that if the transaction rolls back, there is risk that old files are lost and a the new installation is trashed. To overcome this problem, there should be a mechanism to move files out of harms way and later restore them if needed. I have almost finished such an implementation and will submit it as a patch. The idea is that instead of simply deleting files the file is given to a BackupStore for safekeeping. When rolling back, actions should still undo their stuff, but can rely on BackupStore to restore all files and directories that were "deleted". If the operation succeeds, the BackupStore is asked to cleanup the backup copy. My first implementation will not support checkpointing - but that should be easy to add. It does however tries it best to create unique backup folders across the same VM, across multiple VMs on same box, and across boxes (since provisioning may take place concurrently in remote file systems). Most provisioning tasks are probably just touching files within one install location (paired with a locked profile) - but there are occasions where files outside of this directory needs to be backed up during provisioning - hence the more elaborate handling of uniqueness. Once finished, I will modify the CopyAction, and CleanucopyActions submitted as patches earlier to use the backup mechanism. Will supply JUnit test as well.
My idea is to make use of the touchpoint rollback() (to restore files), and commit() (to cleaout the backups).
Created attachment 124206 [details] Implementation of BackupStore - see Comment Patch contains: - Implementation of BackupStore (in o.e.e.p2.core.helpers - CopyAction, CleanupcopyAction - NativeTouchpoint use of BackupStore - FileUtils in o.e.e.p2.core.helpers use BackupStore when unzipping - Use of backup in Unzip, Cleanupzip, Rmdir, Mkdir, and Link actions
Created attachment 124216 [details] With RemoveAction and correct copyright text This replacement attachment Includes a RemoveAction as well (with JUnit test), and added missing copyright text on some classes.
Created attachment 124233 [details] Forgot to add RemoveAction extension point I was a bit too fast when adding the RemoveAction - forgot to also add an extension for it. Included in this patch.
In the last p2 call, someone said there were some questions about the unique id generation...
Created attachment 124694 [details] Failed tests on Windows I've taken a look at the code for BackupStore & ran the test cases on Windows, but had a few problems. A the moment 7 of the 18 tests fail, I believe indirectly caused by the location of the backup directory. The tests are creating directories in the root of C, but are not removing them (File.remove() isn't noticing that it isn't deleting any directory in the tree). Creating/deleting directories in the root would be problematic in Vista as users need Administrator access for those changes, so would fail, or present UAC pop-ups requesting permission. A similar issue might occur on a *nix system if the backup directory were to resolve to /. Simon recently added a File object with the key "profileDataDirectory" to the parameter Map, perhaps that could be used as the location for backups. I also wondered about backupCopy, to me logically it makes sense to leave the original file in place and create the backup, rather than move the original, and attempt to recreate it. If an IOException were to occur, the user would need to manually restore the original file. Perhaps FileUtils.copy() should used?
(In reply to comment #6) > Created an attachment (id=124694) [details] > Failed tests on Windows > > I've taken a look at the code for BackupStore & ran the test cases on Windows, > but had a few problems. > > A the moment 7 of the 18 tests fail, I believe indirectly caused by the > location of the backup directory. The tests are creating directories in the > root of C, but are not removing them (File.remove() isn't noticing that it > isn't deleting any directory in the tree). Creating/deleting directories in > the root would be problematic in Vista as users need Administrator access for > those changes, so would fail, or present UAC pop-ups requesting permission. A > similar issue might occur on a *nix system if the backup directory were to > resolve to /. Simon recently added a File object with the key > "profileDataDirectory" to the parameter Map, perhaps that could be used as the > location for backups. > The problem is that a useful root per filesystem is needed - there is no way to figure out what type of file system the backup file is stored on. The current implementation assumes that if you are allowed to create a file in a directory, you are also allowed to create (and subsequently delete) a directory there. Maybe the cure is to create and delete a directory instead in the probing logic. Do you think that technique would work on Windows? > I also wondered about backupCopy, to me logically it makes sense to leave the > original file in place and create the backup, rather than move the original, > and attempt to recreate it. If an IOException were to occur, the user would > need to manually restore the original file. Perhaps FileUtils.copy() should > used? The normal case is that the file is going to be deleted, so always copying wastes resources. (Although if a copy was made, the backup location could be anywhere on disk). By moving the file (instead of copying) the issue of permissions and special files is also worked around. Say that the original is a link, or something with special permissions. The user would expect the exact thing after a rollback. It would not be possible to recreate such special files from within java.
Created attachment 124710 [details] BackupStore.java Please try with the attached BackupStore.java - the probing now also checks if it is possible to create and delete a directory in the directory elected to be root. Hopefully this is enough to make it work on Windows and Vista.
Created attachment 124735 [details] Updated failed tests The new changes are causing failures in the BackupStore tests on Windows XP as well. It is also still creating directories in C:\
I brushed of the dust on my old XP box, and started debugging. I have a suspicion that it is the backupCopy that creates the problem. I think that somehow, windows thinks the files are "in use" and refuses to rename. The problem seems to be time sensitive - slowly stepping through the code made the code work. Have a strong suspicion this has to do with file handles not being released to windows... Anyone with similar experiences of rename?
Any chance a FileOutputStream, FileInputStream, JarFile etc. is left dangling somewhere and finally closed during GC?
I understand the rationale behind writing in a common parent folder, however somehow this is raising the red flag for me. Could not the backup store be rooted in the same folder where the writing? This would result in several stores but is this a real issue? > Maybe the cure is to create and delete a directory instead in the probing logic. This is what we are doing to properly determine in equinox or the launcher the write capability of a folder
(In reply to comment #12) > I understand the rationale behind writing in a common parent folder, however > somehow this is raising the red flag for me. Could not the backup store be > rooted in the same folder where the writing? This would result in several > stores but is this a real issue? > I started that way, but it gets quite complicated as the folder where the backup folder is placed could also be backed up. > > Maybe the cure is to create and delete a directory instead in the probing logic. > This is what we are doing to properly determine in equinox or the launcher > the write capability of a folder > Good to know. I am using this method now to detect directory writability. After long debugging session on a Windows XP, I now know that this has nothing to do with finding a writable directory. It is related to copying - I am writing a smaller test to provoke the problem.
I found the issue - it is an error in the test code where the file content assertion leaves the file open! New version of the test class will be uploaded shortly.
Created attachment 124825 [details] BackupStore.java Minor change in BackupStore.java - should not be required to make the tests run on Windows compared to previous attachment of this class.
Created attachment 124826 [details] BackupTest.java - fixed issue This BackupStore.java should pass the tests on Windows as well. The issue was that the assertion logic that checked the content of a file did not close the file after reading, And on Windows it is then impossible to move or delete the file.
Created attachment 124827 [details] patch of native touchpoint tests This patch fixes the same problem as in BackupTest - but for the native touchpoint action tests that makes use of Backup. (Same problem - the file content assertion left the file open). Would very much appreciate if someone could run the tests on Vista as well.
Reassigning to John for review and setting M6 as a goal
I have entered a CQ for this contribution: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=3105
One high level question I have is why is this IBackupStore in p2.core? It feels like utility classes for use only by touchpoints. Unless we have an immediate need for it elsewhere, org.eclipse.equinox.p2.engine feels like a better home for it.
(In reply to comment #20) > One high level question I have is why is this IBackupStore in p2.core? It feels > like utility classes for use only by touchpoints. Unless we have an immediate > need for it elsewhere, org.eclipse.equinox.p2.engine feels like a better home > for it. > I placed the backup store there because it was needed in one of the other helpers (in core) that performs unzipping. That was the only reason.
Created attachment 126320 [details] Updated patch with tests I was still having test failures on Windows due to a file being left open. I finally tracked this down to the assertFileContents methods in the tests that didn't close the reader. While tracking it down I found and fixed some other places where streams weren't being closed properly in finally blocks. This patch also moves the backup store out of p2.core and into p2.touchpoint.natives, which is the only user of it. We can consider moving it elsewhere later if needed, but this avoids adding the new code dependency where it isn't needed.
Some other outstanding issues: - File.renameTo is used. This method doesn't support renaming across volumes (as mentioned in the javadoc). We need to fall back to a copy+delete when renameTo fails. - File.listFiles can return null, but this isn't consistently checked (CleanupCopyAction, CopyAction, BackupStore#fullyDelete, BackupStore#restore).
(In reply to comment #23) > Some other outstanding issues: > > - File.renameTo is used. This method doesn't support renaming across volumes > (as mentioned in the javadoc). We need to fall back to a copy+delete when > renameTo fails. > - File.listFiles can return null, but this isn't consistently checked > (CleanupCopyAction, CopyAction, BackupStore#fullyDelete, BackupStore#restore). > The solution takes that into account. It never moves across volume boundaries. It starts by finding the volume boundaries. ok, the second issue should be handled.
(In reply to comment #22) > Created an attachment (id=126320) [details] > Updated patch with tests > > I was still having test failures on Windows due to a file being left open. I > finally tracked this down to the assertFileContents methods in the tests that > didn't close the reader. While tracking it down I found and fixed some other > places where streams weren't being closed properly in finally blocks. > ok, thanks, I thought I had all of those fixed in the additional attachments. > This patch also moves the backup store out of p2.core and into > p2.touchpoint.natives, which is the only user of it. We can consider moving it > elsewhere later if needed, but this avoids adding the new code dependency where > it isn't needed. > How did you deal with the fact that file utils that performs the unzip needs to support the backup store?
(In reply to comment #23) > Some other outstanding issues: > > - File.listFiles can return null, but this isn't consistently checked > (CleanupCopyAction, CopyAction, BackupStore#fullyDelete, BackupStore#restore). > So, should I patch your latest patch to fix these?
Yes, please start from my patch and update from there.
*** Bug 220647 has been marked as a duplicate of this bug. ***
I have fixed the handling of listFiles possibly returning null on IO Errors. New patch to follow. What I wonder about is the earlier suggestion to do a copy if the move fails. The move will not fail because of moving between volumes as the backup directory is always placed on the same volume as the file being backed up. So that leaves moves that are blocked by other reasons ("in use", "locking", "permissions" etc). I wonder if it is not better to fail immediately in these cases as it is unlikely that a delete of the files will work anyway, and later, a restore would most likely also fail. So - I suggest that the current behavior is kept (i.e. no copy if move fails). Under what circumstances do you think it is valuable to make a copy if move fails (other than volume boundaries - which is already handled)?
Created attachment 126371 [details] Updated patch - fixed cases where listFiles can return null Patch contains implementation and tests.
I think we need to support the case where the backup store isn't on the same volume as the files being backed up. We have code elsewhere in Eclipse for doing recursive move without renameTo, so there is no need for us to have this limitation (apart from performance). I don't know how this current code behaves on *nix, but on Windows it currently stores files at the root of the drive (C:\). This isn't a reasonable place for an application to be spreading its files. Windows has guidelines on where applications can write their date (under Documents and Settings, either the temp dir or user's home dir). I suggest we should keep BackupStore simple and require clients of BackupStore to provide the root directory for the backup store. Our usage in the engine and touchpoints can use the engine's profile data area (eclipse/p2/org.eclipse.p2.engine/profileRegistry/<profileId>/.data). We know the p2 location is writeable and under our control, and we have a story on Vista and elsewhere for ensuring this directory is writeable.
(In reply to comment #31) > I think we need to support the case where the backup store isn't on the same > volume as the files being backed up. We have code elsewhere in Eclipse for > doing recursive move without renameTo, so there is no need for us to have this > limitation (apart from performance). I don't know how this current code behaves > on *nix, but on Windows it currently stores files at the root of the drive > (C:\). This isn't a reasonable place for an application to be spreading its > files. Windows has guidelines on where applications can write their date (under > Documents and Settings, either the temp dir or user's home dir). > The current impl finds the highest writeable directory on a volume and places the backup directory there. Works on n*x and windows. > I suggest we should keep BackupStore simple and require clients of BackupStore > to provide the root directory for the backup store. Our usage in the engine and > touchpoints can use the engine's profile data area > (eclipse/p2/org.eclipse.p2.engine/profileRegistry/<profileId>/.data). We know > the p2 location is writeable and under our control, and we have a story on > Vista and elsewhere for ensuring this directory is writeable. > ok, I can change the impl so it moves files if it can, and otherwise makes a copy. The user can specify the directory where the backup directory should be stored. It gets a bit messier as the backup needs to understand things like Windows drive names and network mapped folders. But I found a simple solution. Working on the modified implementation now.
> The current impl finds the highest writeable directory on a volume and places > the backup directory there. Works on n*x and windows. I understand that it works at a technical level, but I think it doesn't match expected behaviour of applications on *nix or Windows. As a user if I found these extra files lying around at the root of my filesystem, I would be tempted to delete them. If I had multiple Eclipse-based applications installed on a volume, it would quickly create a lot of clutter. There are widely followed conventions on where application should store their state, such as: http://www.pathname.com/fhs/pub/fhs-2.3.html http://msdn.microsoft.com/en-us/library/bb530410.aspx#vistauac_topic6c We have often run into these problems in the past of people complaining Eclipse is storing state where it shouldn't, so we try be well-behaved in storing our state.
Created attachment 126505 [details] New impl (and modified tests) that does not use volume roots Here is a new implementation that does not try to find volume roots. Instead it will try a renameTo, and if that does not work, it will copy the file. It is now also possible to instantiate the BackupStore with a directory of choice. The default is to use user.home. Please test on Vista. A simple way to test that cross volume backup works is to modify the BackupStoreTest class in #setUp() and have it use a directory on a different drive for the files being backed up. It is difficult to write automated testcode for this as it is unknown what drives/volumes are available in the test environment.
Moving to early M7. We are focused this week on polishing up other work for M6, and I don't think we have time to shake out any possible bugs in this. There is no impact on API or visible end user function so it's an ideal item to move into M7. Note the CQ mentioned in comment #19 has been approved, so we should be able to get this in quickly after M6 is done.
Well, the patch contains the Copy action that we would like to start using. Other than that - understand there are other issues that needs to be ironed out - getting this into m6 would be appreciated. If issues are found - I have time to work on fixing them.
Can this get in now?
Any issues with getting this comitted?
Aiming to release this week. I did a quick check and there are two conflicts on the patch: the Util and CleanupzipAction in p2.touchpoint.natives. If you could provide an updated patch against HEAD it would speed up the process.
Ignore the previous comment - I think I've got it merged with latest from HEAD. Just running some tests.
Released to HEAD. The only significant change I made was to use java.io.tmpdir rather than user.home as the backup store location. This is the directory we use for all other temp files in p2 (signature files, temp folder for pack/unpack, etc). I also changed the backup folder prefix to be the profile ID, to help identify what backup folder is associated with what profile. I ran some further manual tests: - installing/uninstalling from legacy site (eclemma) - upgrading/reverting the Eclipse SDK (between I20090331 and I20090401)
Created attachment 130754 [details] Additional fix The tests were all passing, but after releaesing to HEAD they started failing. This is because the added "CVS" directories revealed a bug in the undo copy code. It was deleting directories from the top-down, which will fail when there are nested directories because you can't delete a directory with existing children. This patch flips the directory list around before deleting. The tests pass again with this fix. Henrik, can you think of any other places where we might have this problem?
I have released this additional fix to HEAD.
(In reply to comment #43) > I have released this additional fix to HEAD. > Thanks John, I will update the wiki page that describes touchpoint actions.
(In reply to comment #41) > Released to HEAD. The only significant change I made was to use java.io.tmpdir > rather than user.home as the backup store location. This is the directory we > use for all other temp files in p2 (signature files, temp folder for > pack/unpack, etc). I also changed the backup folder prefix to be the profile > ID, to help identify what backup folder is associated with what profile. The only possible downside is that if the machine crashes during install, the backup will be deleted if java.io.tmpdir points to a filesystem that is cleared on boot (or is in memory). That was the reason I used users home directory. The delete in wrong order was an oversight, and I will review if the same mistake was made elsewhere.
>The only possible downside is that if the machine crashes during install, the >backup will be deleted if java.io.tmpdir points to a filesystem that is cleared >on boot (or is in memory) Simon correct me if I'm wrong, but I don't think the engine currently supports rollback across machine restarts. I.e., if you crash in the middle of an install, I don't think the rollback will happen in the next session.
(In reply to comment #46) > >The only possible downside is that if the machine crashes during install, the > >backup will be deleted if java.io.tmpdir points to a filesystem that is cleared > >on boot (or is in memory) > > Simon correct me if I'm wrong, but I don't think the engine currently supports > rollback across machine restarts. I.e., if you crash in the middle of an > install, I don't think the rollback will happen in the next session. > Neither does the backup solution for native,,, but a manual restore is possible. And would make it possible to detect a previous crash and offer recover if the backup was kept. For now it is fine to keep the backup in temp, and if "recover on restart" is ever implemented, it would be very easy to move the backups to a place that survives a machine reboot.
(In reply to comment #46) > Simon correct me if I'm wrong, but I don't think the engine currently supports > rollback across machine restarts. I.e., if you crash in the middle of an > install, I don't think the rollback will happen in the next session. Someday... but not for this release.
(In reply to comment #48) > (In reply to comment #46) > > Simon correct me if I'm wrong, but I don't think the engine currently supports > > rollback across machine restarts. I.e., if you crash in the middle of an > > install, I don't think the rollback will happen in the next session. > > Someday... but not for this release. > Should we perhaps create an issue to keep track of this future enhancement?
*** Bug 262337 has been marked as a duplicate of this bug. ***
*** Bug 262336 has been marked as a duplicate of this bug. ***
