Community
Participate
Working Groups
3.5 M6 Martin had a case where a bundle from dropins remained installed into the profile even after being deleted from disk. This resulted in a broken installation. I think the case is: 1) Install bundle a version 1.0 via dropins 2) Install bundle b requiring 'a' [1.0,2.0) via normal install 3) Shutdown, replace a 1.0 in dropins with a 2.0 (actually changed value of .link file to point to newer version) 4) Restart. 'a' 1.0 is still in the profile (presumably because it's required by bundle 'b', however it no longer exists on disk)
Sounds similar to bug 269297.
Actually this sounds like another bug that Pascal and I were talking about a while back. (sorry can't find the link) Pascal, do you remember? It was the one where we were taking the IUs already installed in the profile into consideration and that was causing us problems.
Don't remember if we have a bug for this precise issue, but DJ's description is right on.
It's completely natural behaviour for the planner to decide to keep the bundle around because it is needed by another installed IU. It seems like we need some way to force the IU to be removed. Maybe it is still available in a repository at that point, but shouldn't be?
Created attachment 134799 [details] repo Here is a repository which contains bundles which exhibit the problem. - put "a 1.0" in your dropins - startup - install "b 1.0" via the UI - shutdown - delete "a 1.0" from dropins - add "a 2.0" to the dropins - startup John and I talked about this a bit and it seems like we need some sort of "force uninstall" mode. In this case it would uninstall the bundle which no longer exists as well as anything else which depends on it. Something to consider post-3.5.
Interesting. To clarify a bit (maybe just for me) this is a force uninstall at the planner level.
Yes, essentially we invoke the planner with "please uninstall X", but the planner comes back with an empty plan because someone else needs X. We need a planner request more like "thou shalt uninstall X".
See also: bug 306424 comment 6
With the fix for bug 308934, the behaviour is now as follows where A depends on B: - put B in dropins and start Eclipse - install A via UI - exit Eclipse - delete B from dropins - start Eclipse - get the following in the log: !ENTRY org.eclipse.equinox.p2.director 4 0 2010-04-14 10:11:23.396 !MESSAGE Problems resolving provisioning plan. !SUBENTRY 1 org.eclipse.equinox.p2.director 4 0 2010-04-14 10:11:23.396 !MESSAGE No solution found because the problem is unsatisfiable. Also A is now installed but not resolved. (according to the OSGi console)
Yuck. I guess the minimum I'd expect to be logged is information about WHAT specific IU's requirements can not be resolved (A in this case). In the ideal case it would also say that B is missing.
The behaviour exposed in comment #9 is not correct. This will result in completely breaking the installation and possibly causing it to not be fixable. We need to discuss this but I don't think we can ship with this. IMO the consequences of this new problem are far worse than the problem being fixed.
To summarize: In the old case if you manually deleted something from the dropins and you have something installed (via the UI) which depended on it then you would get no notification that what you have installed is broken and its UI pieces would still appear, etc. Then when you tried to click on an icon or something which had behaviour depending on a piece which is now missing, then it wouldn't work. In the new case if you manually delete something from the dropins and you have something installed via the UI which depends on it then we unresolve the pieces whose dependencies are now broken. Agreed that the message should be made clearer to help the user understand what exactly went wrong. Pascal, please feel free to elaborate as to what you feel the solution should be. The only thing that I can think of to make the situation better is that when you install something from the UI and it depends on something which is in the dropins, then we move (copy?) the bundles from the dropins to the plugins so we now manage them and then aren't optional anymore. Although I doubt that users would want us to move their bundles around like that.
> In the new case if you manually delete something from the dropins and you have > something installed via the UI which depends on it then we unresolve the pieces > whose dependencies are now broken. Agreed that the message should be made > clearer to help the user understand what exactly went wrong. In the example of A req B with B is in the dropin. Does this mean that the root IU A will be uninstalled as well? - If A is uninstalled then I'm cool. - If A is not uninstalled then other problems can ensue. The biggest would be when the user goes and try to install something else in the UI (unrelated to A and B), no solution gets found because no repository containing B can be found.
(In reply to comment #13) > In the example of A req B with B is in the dropin. Does this mean that the > root IU A will be uninstalled as well? > - If A is uninstalled then I'm cool. > - If A is not uninstalled then other problems can ensue. The biggest would > be when the user goes and try to install something else in the UI (unrelated to > A and B), no solution gets found because no repository containing B can be > found. Just to make sure I understand you correctly. This essentially means nothing can ever be provisioned because we will always have a broken resolution state according to p2. This is rather drastic.
(In reply to comment #13) > In the example of A req B with B is in the dropin. Does this mean that the > root IU A will be uninstalled as well? > - If A is uninstalled then I'm cool. > - If A is not uninstalled then other problems can ensue. The biggest would > be when the user goes and try to install something else in the UI (unrelated to > A and B), no solution gets found because no repository containing B can be > found. You're cool ;)
>You're cool ;) Unfortunately I'm not. I've tested this on I20100414-1200 with some plug-ins (will attach later) and the behaviour I have observed is rather surprising :) Steps followed: - put the ToDropin in the dropins folder, start and notice the presence of the plugin - go to the UI and install the TestMixedInstall IU, notice that the plugin called ToInstallIn is there and notice the presence of the root. - delete from the dropins the ToDropin plugin restart Some of the surprising things: - go to the installed software page and notice that root IU is still visible. This is misleading since in reality all the plug-ins are gone. - the install is not broken, this is somewhat a good thing because the user can continue installing things - the install is not broken, this is somewhat a bad thing because the user can install something that depends on "TestMixedInstall" IU, it will look like if it has worked and it will not have worked. - the IU for ToDropin is *still* in the profile. This is really not expected So in short, I think the new behaviour is no better than the previous one, if not worse. As for how to improve the situation. The uninstallation of something that is depended on from a root should cause the root IU to be uninstalled. I have not looked at the code but it feels like the removal is bypassing the planner which is the only way you can end up with such a situation. So an approach would: - Notice that a file has been deleted from disk - Emit a request causing the removal of the IU (PCR#removeIU) but also forces it absence in the final result (PCR#addExtraRequirement(Negation of the IU being removed)). - If the resolution fails, it is because the item being removed is being depended on from a root. - Then find the root (this should be available in the planner result), emit another request that is the same one from the original plus requesting the forced removal of the root. - Repeat until the plan comes back without error. This approach has the advantage that it leaves the installation completely consistent from a dependency standpoint but also from a runtime standpoint. The only problem now is how to let the user know that some things have been uninstalled because of the removal of things in the dropins. Ideally we could introduce a concept of "root error" but this may be a bit too much for this. In any event since it has been decided to invest in this, I think it needs to be fully fixed or leave the old behavior. I don't see the point in introducing in introducing an alternatively bogus behavior :)
Pascal and I talked about this today and have come up with a plan. 1). For the "move" case we will not go through the planner. We will recognize that some IUs need to be moved and then manually create the plan to remove/add them and call the engine directly. 2). To handle the case where we have strict IUs which depend on things we want to remove, we will: - create a plan with the removed IUs - include all root IUs as optional - get the plan back - for each root, does the plan now remove it? - if so then add a negation to remove it We need to watch out for the case where the user has messed things up and all the roots want to be removed. We can't do this. In the end we probably want 2 calls to the engine (one for move and one for the second part) but we might be able to get away with one.
Created attachment 166962 [details] partial patch Pascal, here is a work-in-progress patch. The code which I need a hand with is in #createAddRemovePlan. I don't have the logic quite right when changing some of the roots from strict to optional, etc.
Created attachment 167844 [details] New patch Here is a new patch. I've only focused on the addition/removal case. It is now able to reset the inclusion rules, and warn the user when a strict root is being removed as a result of the reconciliation. One thing I wonder is if instead of resetting the inclusion rules manually like this new patch does, we would not be better off calling the planner a second time with the set of strict roots that are not modified (for example if we detect that GMF needs to be removed, then explicitly remove it and leave the SDK as strict). I have also released two new test cases ProfileSynchronizerTest and ProfileSynchronizerTest2 that are not enabled.
I would really like to see this go in RC1 otherwise it will be in 3.6.1.
Sorry Pascal, I didn't get to review this today. Since the last RC1 build is tomorrow morning it looks like it won't make it. If you don't feel comfortable releasing this into RC2 then we should review the current behaviour in HEAD and ensure that is what we want for 3.6. We may need to back out of other changes as I know you didn't like some of the changes to get the move case detected.
I think we are very close to have something good here. At this point I would rather polish this bug rather than diving in the other things that I did not like. Let's try to get to this tomorrow and release it in RC2 (or a respin of RC1 if we can manage this).
Based on the change, I would vote for RC2 over RC1 so we can get more testing on it rather than releasing it on the last day of the RC in the final build.
Created attachment 168455 [details] patch Updated patch. I made a few modifications to get the move case working again, tried to get rid of some of the TODOs, etc. Still testing so not ready for release.
Created attachment 168488 [details] patch Here is an updated patch and it is starting to feel good. I've separated the move and add/remove actions into separate "perform" methods to better report to the user when we have a problem creating a plan. I've done some manual testing but would like more automated tests for this. I've also verified that the scenario in comment #5 now works.
Created attachment 168852 [details] Another version of the patch Here is yet another copy of the patch. I was not a big fan of bashing the IU properties to strict on the initial plan, and also not planning with the roots with their real inclusion status. Therefore this change actually look at the result from the first plan (where everything is optional), create a new request and replan with what has been learnt from the first plan (i.e. explicitly removing the strict roots that will be dropped). The patch includes a change in the profile change request to fix the behaviour clone (one field was not copied). Now I really like the patch and I think we are good to release this.
One final observation the following if-block should never ever occur. If it does it means that we have a bug in our code which is why I'm throwing the illegal state exception and writing in sysout. if (!plan.getStatus().isOK()) { System.out.println("original request"); //$NON-NLS-1$ System.out.println(request); System.out.println("final request"); //$NON-NLS-1$ System.out.println(finalRequest); throw new IllegalStateException("The second plan is not resolvable."); //$NON-NLS-1$ }
Pascal does the latest patch leave the root IUs in a state where they are all still marked as optional? I think we are missing code that sets the inclusion rule back to strict for roots which aren't removed by the plan. This might be easier done by cloning early and then adding the negation and property changes later after we get the initial plan results.
Created attachment 168993 [details] patch Updated version of the patch implementing changes described in the previous comment.
For part 1) in comment #17 I like this fix better than what we have in HEAD today. I would agree to doing that part of the fix. However I'm worried about part 2), in particular marking strict roots as optional. I see that this will handle the case where the strict root requires something that has been deleted, but I'm worried it will cause strict roots to be uninstalled in other scenarios. From my point of view, uninstalling a strict root should *never* happen unless it is completely impossible for it to remain installed (like its dependencies have been deleted. For example imagine a case like this: A is installed strict, depends on C [1.0,1.1) B is installed optional, depends on C [2.0,3.0) B, c 1.0 and c 2.0 are in dropins. Normally, the planner will pick (A, C 1.0). However if A is marked optional, it might instead pick (B, C 2.0) since that solution is equally valid. Now we have uninstalled a strict root in order to satisfy an optional root. This is one example, but I'm worried there might be other side-effects like this from marking strict roots as optional here.
My current thinking is that we should either do nothing for 3.6.0, or do only part 1) and defer the rest. From DJ's testing it looks like we will be no worse than the 3.5 behavior if we defer the fix for 2). We would then have more time in 3.7 and possibly 3.6.1. to make a safe fix for case 2 (uninstalling strict roots whose dependencies have been deleted).
(In reply to comment #30) > For part 1) in comment #17 I like this fix better than what we have in HEAD > today. I would agree to doing that part of the fix. +1 > However I'm worried about part 2), in particular marking strict roots as > optional. I see that this will handle the case where the strict root requires > something that has been deleted, but I'm worried it will cause strict roots to > be uninstalled in other scenarios. From my point of view, uninstalling a strict > root should *never* happen unless it is completely impossible for it to remain > installed (like its dependencies have been deleted. >[...] This is a good point. Initially we wanted to look into the results of the planner (in case of error) and this is likely the solution to adopt going forward.
Accidentally moved to 3.6.1. Moving back to rc2 for the move part.
I suggest that we also release the clone() method in ProfileChangeRequest
Created attachment 169212 [details] patch New patch. Same as last one but just disables the "remove strict roots" code. (I left it in the class and disabled it so we wouldn't lose it or worry about having to merge changed code and stale patches later)
Some comments from code review with DJ: - Modify debug(request) to also print move info - Change in synchronizeCacheExtensions not needed - Changed method signature in createProvisioningPlan not needed - Make sure debug(request,plan) output still makes sense
Created attachment 169234 [details] patch Updated patch including changes from John's feedback. We still need to test more but I am posting this for others to see. Note to reviewers: if you delete the #performStrictRootRemoval method (which is disabled) then it makes the patch much simpler to review.
The only comment would be to rename the method performMove to performRemoveForMovedIUs since it only take care of the removal part.
I agree with renaming the method as Pascal suggests. One hopefully final comment, in the synchronize method we have: IStatus moveResult = performMove(request, context, monitor); if (!moveResult.isOK()) return moveResult; Usually with a plan result we do something like if (status.matches(IStatus.ERROR | IStatus.CANCEL)) rather than status.isOk(). I don't know if we have cases of the engine returning INFO or WARNING, but we should probably be consistent here and let the reconcile continue on to the next step unless we get ERROR or CANCEL.
Created attachment 169348 [details] patch Updated patch based on recent comments.
Opened bug 313795 to track the original issue and we'll use this bug to track the #move issue in part one of comment #17.
I think DJ wants to do some more testing, but +1 for RC3.
Let me state here my original use case (which might be a helpful test case for this once it's done): 1.) Download eclipse-SDK-3.6Mx.zip 2.) Add *.link file pointing to emf-2.5 extension location 3.) In the UI, install something that depends on EMF, e.g. GMF or TM-discovery 4.) Replace the *.link file by a new one pointing to emf-2.6 ext.loc. Since I expect the EMF "update" to be fully compatible, I expect the whole environment to come up properly again. I'd never expect anything that I installed from the UI to be removed. In the worst case (that I really broke some dependency, which I personally am pretty sure I never did since the "update" should be compatible) I would expect my UI-installed extension to be flagged with an error marker, such that I can "repair" my install by reverting to emf-2.5. I think my original use case as per comment 0 was not using EMF but the CDT, and a DSDP-TM addon that depends on the CDT. We have since migrated that addon into the CDT, so the original case isn't easily reproducable any more. But I am very sure that I didn't break any version range (as John indicates in comment 0 points 2) and 3) with the [1.0,2.0) version range). Hope that helps!
+1 for doing the "move" part for 3.6 RC3. It looks to me like our algorithm for part 2 was very close and we might consider doing something in 3.6.1.
Released to HEAD. Using bug 313795 to track remaining issues for 3.6.1/3.7 target. Closing.
I created a patch with just the desired changed (as opposed to the current patch with the force remove operations if'd out). I did this to get a better sense of what this patch covers. I'm happy with either version, as it doesn't seem dangerous to release a bunch of 'dead code', but again, those are always famous last words :-). The only comment I had was we do a short-circuit: if (additions.isEmpty() && removals.isEmpty()) return Status.OK_STATUS; If we (for 3.6.1) flip the bit on the force remove, this short-circuit might not be a good idea (I think we compute the additions / removals for the force removal after this check).
