Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On 03/07/2012 10:04 AM, Tomas Lestach wrote: I will find time for that in the next days and I can then come up with a new patch for that file after I verified the behavior. Great! Thank you Johannes, So, AFAICS the rhnServerNeededCache table is there for caching which errata need to be applied to a system together with the respective package ids. So I don't really see how it makes sense to insert rows in there with errata id == null. I compared the resulting cache entries with what we get when running the stored procedure 'rhn_server.update_needed_cache' (with respective parameters of course). The output was equal, but missing the 'null' rows. Even if i think these 'null' entries don't hurt anybody I would propose to apply the attached patch to PublishErrataAction.java (also the code was quite a mess). Please tell me if this breaks anything and throw away my patch in this case ;-) Thanks, Johannes P.S.: Further, if this is all true, I would actually question the whole method 'insertCacheForChannelPackagesAsync()' in ErrataCacheManager.java, since it does exactly this: ... uece.setErrataId(null); ... But it's called in 3-4 other places as well, so you (or somebody else who knows) should better have a look and verify this first, see the second patch. -- SUSE LINUX Products GmbH, HRB 16746 (AG Nürnberg) GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer From 22cab15a789c1d8cc50f2efb8244fbec1c9a2236 Mon Sep 17 00:00:00 2001 From: Johannes Renner jren...@suse.de Date: Thu, 8 Mar 2012 15:26:33 +0100 Subject: [PATCH] Remove obsolete cache insertion --- .../action/channel/manage/PublishErrataAction.java | 17 ++--- 1 files changed, 2 insertions(+), 15 deletions(-) diff --git a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java index 7a24554..8d98e3d 100644 --- a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java +++ b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java @@ -22,7 +22,6 @@ import com.redhat.rhn.frontend.struts.RequestContext; import com.redhat.rhn.frontend.struts.RhnListAction; import com.redhat.rhn.manager.channel.ChannelManager; import com.redhat.rhn.manager.errata.ErrataManager; -import com.redhat.rhn.manager.errata.cache.ErrataCacheManager; import com.redhat.rhn.manager.rhnset.RhnSetDecl; import org.apache.log4j.Logger; @@ -79,9 +78,7 @@ public class PublishErrataAction extends RhnListAction { ErrataManager.publishErrataToChannelAsync(currentChan, errataIds, user); -//ErrataManager.publishErrataToChannel(currentChan, errataIds, user); - - +// Add missing packages to the channel ListLong pidList = new ArrayListLong(); pidList.addAll(packageIds); @@ -93,28 +90,18 @@ public class PublishErrataAction extends RhnListAction { } } - -//update the errata info -List chanList = new ArrayList(); -chanList.add(currentChan.getId()); -ErrataCacheManager.insertCacheForChannelPackagesAsync(chanList, pidList); ChannelManager.refreshWithNewestPackages(currentChan, web.errata_push); request.setAttribute(cid, cid); +// Add success message ActionMessages msg = new ActionMessages(); String[] params = {errataIds.size() + , packageIds.size() + , currentChan.getName()}; msg.add(ActionMessages.GLOBAL_MESSAGE, new ActionMessage(frontend.actions.channels.manager.add.success, params)); - getStrutsDelegate().saveMessages(requestContext.getRequest(), msg); return mapping.findForward(default); } - - - - - } -- 1.7.7 From dba13014a02468031faeab8700d0959f006946c3 Mon Sep 17 00:00:00 2001 From: Johannes Renner jren...@suse.de Date: Thu, 8 Mar 2012 15:41:16 +0100 Subject: [PATCH] Remove obsolete cache insertion completely --- .../manage/ChannelPackagesAddConfirmAction.java|1 - .../action/errata/RemovePackagesAction.java|7 -- .../channel/software/ChannelSoftwareHandler.java |8 --- .../manager/errata/cache/ErrataCacheManager.java | 21 4 files changed, 0 insertions(+), 37 deletions(-) diff --git a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/ChannelPackagesAddConfirmAction.java b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/ChannelPackagesAddConfirmAction.java index f98206a..77f69e7 100644 --- a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/ChannelPackagesAddConfirmAction.java +++ b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/ChannelPackagesAddConfirmAction.java @@ -125,7 +125,6 @@ public class ChannelPackagesAddConfirmAction extends RhnAction { ListLong packList = new ArrayListLong(); chanList.add(chan.getId());
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On Thursday 08 of March 2012 15:46:46 Johannes Renner wrote: On 03/07/2012 10:04 AM, Tomas Lestach wrote: I will find time for that in the next days and I can then come up with a new patch for that file after I verified the behavior. Great! Thank you Johannes, So, AFAICS the rhnServerNeededCache table is there for caching which errata need to be applied to a system together with the respective package ids. So I don't really see how it makes sense to insert rows in there with errata id == null. The reason is that importing errata packages into the Spacewalk server isn't the only option, how to populate you channels. You may push packages into your Spacewalk server using rhnpush or you may get a package into your channel from another channel. In these cases those packages may be valid updates for your servers, however they're not associated with any errata. For these cases errata_id is null. Regards, Tomas -- Tomas Lestach RHN Satellite Engineering, Red Hat I compared the resulting cache entries with what we get when running the stored procedure 'rhn_server.update_needed_cache' (with respective parameters of course). The output was equal, but missing the 'null' rows. Even if i think these 'null' entries don't hurt anybody I would propose to apply the attached patch to PublishErrataAction.java (also the code was quite a mess). Please tell me if this breaks anything and throw away my patch in this case ;-) Thanks, Johannes P.S.: Further, if this is all true, I would actually question the whole method 'insertCacheForChannelPackagesAsync()' in ErrataCacheManager.java, since it does exactly this: ... uece.setErrataId(null); ... But it's called in 3-4 other places as well, so you (or somebody else who knows) should better have a look and verify this first, see the second patch. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
I will find time for that in the next days and I can then come up with a new patch for that file after I verified the behavior. Great! Thank you Johannes, -- Tomas Lestach RHN Satellite Engineering, Red Hat Regards, Johannes ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On Friday 02 of March 2012 18:11:41 Johannes Renner wrote: On 02/29/2012 05:38 PM, Cliff Perry wrote: On 02/29/2012 11:05 AM, Johannes Renner wrote: Hello, I am investigating into a bug about cloned channels and errata. This is how to reproduce it on the UI: 1. Clone a channel that contains errata, but select clone without errata 2. Go to Channels - Manage Software Channels - Errata - Add - Add Custom Errata (or Add RedHat Errata) - Unmark the checkbox Package Assoc. if necessary 3. Choose an Erratum, clone it and confirm Result: The cloned erratum can be found in the cloned channel as CLxxx 5. Now subscribe a system to the newly cloned channel only 6. Go to this System - Software - Errata Result: There is two errata listed, xxx and CLxxx, but only CLxxx should be. Also the table 'RhnServerNeededCache' shows even more entries where errata_id == null? I found a lot of open bugs about this topic, but not exactly this one. If one of you can reproduce it I could create a bug report for spacewalk, or are you aware of such misbehavior already? To me it looks like the used statement ('insert_new_cache_entries_by_packages' in ErrataCache_queries.xml) is not doing the right thing. There even seems to be an easy fix that I attached as a patch for master, which just calls a stored procedure ('update_needed_cache_for_channel', ErrataCache_queries.xml) instead. Maybe someone of you who knows about this code could have a look at the issue, I might be missing something .. Replying to add more background. I was not aware of this specific bug myself either. I'd be interested to see if we can reproduce on Sat 5.4 code as well as current Spacewalk code. This area of the code seems to get updates and re-factor once every couple of years, due to subtle bugs in logic, along with scalability/performance feedback. The particular line you attached to propose changing was last changed in Feb 2009 as part of a larger re-factor: http://git.fedorahosted.org/git/?p=spacewalk.git;a=commitdiff;h=3707af41 49d84a1ddb1ce693c78f7b791a044cd1 If memory serves me correct, we were in the middle of a Satellite release cycle, so changing the stored procedure was not an ideal solution for Satellite. We fixed Java code - which performed well. I'm not sure if I'd want the fix, as proposed, in 1.7, without us spending time to confirm it is the best solution for logic and performance. I don't think we will have time to dedicate to assist until post 1.7. If one the guys finds we have time to dig in before 1.7 goes out in the next week (hopefully) - we will. Cliff Sorry, I now found out I was experiencing this bug and was missing the patch: https://bugzilla.redhat.com/show_bug.cgi?id=707658 So the cloned patches now show up correctly on the UI, but anyways I think there is some code in here that does strange things. E.g. when I analyze the file PublishErrataAction.java: The newly cloned patch is published by calling in line 80: ErrataManager.publishErrataToChannelAsync(...) This also triggers insertion into the cache table (rhnServerNeededCache). So why do we need to call later on (and without the actual errata ids): ErrataCacheManager.insertCacheForChannelPackagesAsync(chanList, pidList); (line 100) This seems to insert another row into rhnServerNeededCache where the eid is null! If this is wanted behaviour, then what is it for? Right Johannes, good catch. this definitelly isn't wanted behavior. If the added packages to a channals are associated with any erratum (what is our case), we definitelly want to consider also the erratum, when regenerating errata cache. Will you find some time to verify the behavior, when removing the errata cache update on line 100? If not, please, file a BZ for the issue. Thanks, Johannes Thank you, -- Tomas Lestach RHN Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On 03/05/2012 03:40 PM, Tomas Lestach wrote: On Friday 02 of March 2012 18:11:41 Johannes Renner wrote: On 02/29/2012 05:38 PM, Cliff Perry wrote: On 02/29/2012 11:05 AM, Johannes Renner wrote: Hello, I am investigating into a bug about cloned channels and errata. This is how to reproduce it on the UI: 1. Clone a channel that contains errata, but select clone without errata 2. Go to Channels - Manage Software Channels - Errata - Add - Add Custom Errata (or Add RedHat Errata) - Unmark the checkbox Package Assoc. if necessary 3. Choose an Erratum, clone it and confirm Result: The cloned erratum can be found in the cloned channel as CLxxx 5. Now subscribe a system to the newly cloned channel only 6. Go to this System - Software - Errata Result: There is two errata listed, xxx and CLxxx, but only CLxxx should be. Also the table 'RhnServerNeededCache' shows even more entries where errata_id == null? I found a lot of open bugs about this topic, but not exactly this one. If one of you can reproduce it I could create a bug report for spacewalk, or are you aware of such misbehavior already? To me it looks like the used statement ('insert_new_cache_entries_by_packages' in ErrataCache_queries.xml) is not doing the right thing. There even seems to be an easy fix that I attached as a patch for master, which just calls a stored procedure ('update_needed_cache_for_channel', ErrataCache_queries.xml) instead. Maybe someone of you who knows about this code could have a look at the issue, I might be missing something .. Replying to add more background. I was not aware of this specific bug myself either. I'd be interested to see if we can reproduce on Sat 5.4 code as well as current Spacewalk code. This area of the code seems to get updates and re-factor once every couple of years, due to subtle bugs in logic, along with scalability/performance feedback. The particular line you attached to propose changing was last changed in Feb 2009 as part of a larger re-factor: http://git.fedorahosted.org/git/?p=spacewalk.git;a=commitdiff;h=3707af41 49d84a1ddb1ce693c78f7b791a044cd1 If memory serves me correct, we were in the middle of a Satellite release cycle, so changing the stored procedure was not an ideal solution for Satellite. We fixed Java code - which performed well. I'm not sure if I'd want the fix, as proposed, in 1.7, without us spending time to confirm it is the best solution for logic and performance. I don't think we will have time to dedicate to assist until post 1.7. If one the guys finds we have time to dig in before 1.7 goes out in the next week (hopefully) - we will. Cliff Sorry, I now found out I was experiencing this bug and was missing the patch: https://bugzilla.redhat.com/show_bug.cgi?id=707658 So the cloned patches now show up correctly on the UI, but anyways I think there is some code in here that does strange things. E.g. when I analyze the file PublishErrataAction.java: The newly cloned patch is published by calling in line 80: ErrataManager.publishErrataToChannelAsync(...) This also triggers insertion into the cache table (rhnServerNeededCache). So why do we need to call later on (and without the actual errata ids): ErrataCacheManager.insertCacheForChannelPackagesAsync(chanList, pidList); (line 100) This seems to insert another row into rhnServerNeededCache where the eid is null! If this is wanted behaviour, then what is it for? Right Johannes, good catch. this definitelly isn't wanted behavior. If the added packages to a channals are associated with any erratum (what is our case), we definitelly want to consider also the erratum, when regenerating errata cache. Will you find some time to verify the behavior, when removing the errata cache update on line 100? If not, please, file a BZ for the issue. I will find time for that in the next days and I can then come up with a new patch for that file after I verified the behavior. Regards, Johannes -- SUSE LINUX Products GmbH, HRB 16746 (AG Nürnberg) GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
[Spacewalk-devel] [PATCH] Bug about cloned channels and errata
Hello, I am investigating into a bug about cloned channels and errata. This is how to reproduce it on the UI: 1. Clone a channel that contains errata, but select clone without errata 2. Go to Channels - Manage Software Channels - Errata - Add - Add Custom Errata (or Add RedHat Errata) - Unmark the checkbox Package Assoc. if necessary 3. Choose an Erratum, clone it and confirm Result: The cloned erratum can be found in the cloned channel as CLxxx 5. Now subscribe a system to the newly cloned channel only 6. Go to this System - Software - Errata Result: There is two errata listed, xxx and CLxxx, but only CLxxx should be. Also the table 'RhnServerNeededCache' shows even more entries where errata_id == null? I found a lot of open bugs about this topic, but not exactly this one. If one of you can reproduce it I could create a bug report for spacewalk, or are you aware of such misbehavior already? To me it looks like the used statement ('insert_new_cache_entries_by_packages' in ErrataCache_queries.xml) is not doing the right thing. There even seems to be an easy fix that I attached as a patch for master, which just calls a stored procedure ('update_needed_cache_for_channel', ErrataCache_queries.xml) instead. Maybe someone of you who knows about this code could have a look at the issue, I might be missing something .. Thank you, Johannes -- SUSE LINUX Products GmbH, HRB 16746 (AG Nürnberg) GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer From 25cac35e2f20c249dd18ba522fe112ab058acd08 Mon Sep 17 00:00:00 2001 From: Johannes Renner jren...@suse.de Date: Wed, 29 Feb 2012 16:52:43 +0100 Subject: [PATCH] Fix bug about cloned channels and errata --- .../action/channel/manage/PublishErrataAction.java |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java index 7a24554..226bb8a 100644 --- a/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java +++ b/java/code/src/com/redhat/rhn/frontend/action/channel/manage/PublishErrataAction.java @@ -97,7 +97,7 @@ public class PublishErrataAction extends RhnListAction { //update the errata info List chanList = new ArrayList(); chanList.add(currentChan.getId()); -ErrataCacheManager.insertCacheForChannelPackagesAsync(chanList, pidList); +ErrataCacheManager.updateCacheForChannelsAsync(chanList); ChannelManager.refreshWithNewestPackages(currentChan, web.errata_push); request.setAttribute(cid, cid); -- 1.7.7 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On 02/29/2012 11:05 AM, Johannes Renner wrote: Hello, I am investigating into a bug about cloned channels and errata. This is how to reproduce it on the UI: 1. Clone a channel that contains errata, but select clone without errata 2. Go to Channels - Manage Software Channels - Errata - Add - Add Custom Errata (or Add RedHat Errata) - Unmark the checkbox Package Assoc. if necessary 3. Choose an Erratum, clone it and confirm Result: The cloned erratum can be found in the cloned channel as CLxxx 5. Now subscribe a system to the newly cloned channel only 6. Go to this System - Software - Errata Result: There is two errata listed, xxx and CLxxx, but only CLxxx should be. Also the table 'RhnServerNeededCache' shows even more entries where errata_id == null? I found a lot of open bugs about this topic, but not exactly this one. If one of you can reproduce it I could create a bug report for spacewalk, or are you aware of such misbehavior already? To me it looks like the used statement ('insert_new_cache_entries_by_packages' in ErrataCache_queries.xml) is not doing the right thing. There even seems to be an easy fix that I attached as a patch for master, which just calls a stored procedure ('update_needed_cache_for_channel', ErrataCache_queries.xml) instead. Maybe someone of you who knows about this code could have a look at the issue, I might be missing something .. Replying to add more background. I was not aware of this specific bug myself either. I'd be interested to see if we can reproduce on Sat 5.4 code as well as current Spacewalk code. This area of the code seems to get updates and re-factor once every couple of years, due to subtle bugs in logic, along with scalability/performance feedback. The particular line you attached to propose changing was last changed in Feb 2009 as part of a larger re-factor: http://git.fedorahosted.org/git/?p=spacewalk.git;a=commitdiff;h=3707af4149d84a1ddb1ce693c78f7b791a044cd1 If memory serves me correct, we were in the middle of a Satellite release cycle, so changing the stored procedure was not an ideal solution for Satellite. We fixed Java code - which performed well. I'm not sure if I'd want the fix, as proposed, in 1.7, without us spending time to confirm it is the best solution for logic and performance. I don't think we will have time to dedicate to assist until post 1.7. If one the guys finds we have time to dig in before 1.7 goes out in the next week (hopefully) - we will. Cliff Thank you, Johannes ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] [PATCH] Bug about cloned channels and errata
On 02/29/2012 05:38 PM, Cliff Perry wrote: On 02/29/2012 11:05 AM, Johannes Renner wrote: Hello, I am investigating into a bug about cloned channels and errata. This is how to reproduce it on the UI: 1. Clone a channel that contains errata, but select clone without errata 2. Go to Channels - Manage Software Channels - Errata - Add - Add Custom Errata (or Add RedHat Errata) - Unmark the checkbox Package Assoc. if necessary 3. Choose an Erratum, clone it and confirm Result: The cloned erratum can be found in the cloned channel as CLxxx 5. Now subscribe a system to the newly cloned channel only 6. Go to this System - Software - Errata Result: There is two errata listed, xxx and CLxxx, but only CLxxx should be. Also the table 'RhnServerNeededCache' shows even more entries where errata_id == null? I found a lot of open bugs about this topic, but not exactly this one. If one of you can reproduce it I could create a bug report for spacewalk, or are you aware of such misbehavior already? To me it looks like the used statement ('insert_new_cache_entries_by_packages' in ErrataCache_queries.xml) is not doing the right thing. There even seems to be an easy fix that I attached as a patch for master, which just calls a stored procedure ('update_needed_cache_for_channel', ErrataCache_queries.xml) instead. Maybe someone of you who knows about this code could have a look at the issue, I might be missing something .. Replying to add more background. I was not aware of this specific bug myself either. I'd be interested to see if we can reproduce on Sat 5.4 code as well as current Spacewalk code. This area of the code seems to get updates and re-factor once every couple of years, due to subtle bugs in logic, along with scalability/performance feedback. The particular line you attached to propose changing was last changed in Feb 2009 as part of a larger re-factor: http://git.fedorahosted.org/git/?p=spacewalk.git;a=commitdiff;h=3707af4149d84a1ddb1ce693c78f7b791a044cd1 If memory serves me correct, we were in the middle of a Satellite release cycle, so changing the stored procedure was not an ideal solution for Satellite. We fixed Java code - which performed well. I'm not sure if I'd want the fix, as proposed, in 1.7, without us spending time to confirm it is the best solution for logic and performance. I don't think we will have time to dedicate to assist until post 1.7. If one the guys finds we have time to dig in before 1.7 goes out in the next week (hopefully) - we will. Cliff Thanks for your reply. The patch was not intended to go into 1.7 directly and it is probably not even complete as it is. It rather reflects the smallest possible change that seems to fix the bug. More code in the same class will be obsolete when we call the stored procedure, e.g. we do not need the list of all packages of the current channel anymore. If one of you could have a look as soon as there is time, it would be great. After 1.7 is fine of course. Thanks, Johannes -- SUSE LINUX Products GmbH, HRB 16746 (AG Nürnberg) GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel