The dhcp template generation is broken when there is no dns-name for interfaces set. The attached patch fixes this problem and does some cleanup to the code. Also the entries are now named with the system record name and interface instead of generic<idx>. This makes debugging the template easier.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
Vreman, Peter - Acision wrote:
The dhcp template generation is broken when there is no dns-name for interfaces set. The attached patch fixes this problem and does some cleanup to the code. Also the entries are now named with the system record name and interface instead of generic<idx>. This makes debugging the template easier.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler-devel
Not applying... I'll explain.
I am generally not interested in "refactoring" in general -- the practice of shuffling code around has a high likelihood of introducing more bugs, and we do not generally have a large amount of folks that help with pre-release testing. Also, such patches are notoriously hard to review. We prefer that bugfixes are more surgical. So, if you're fixing a bug always strive to fix it in the least amount of lines possible. Be a ninja.
Cleanup -- where we decide it's needed -- needs to go on devel, while fixes need to go to master & devel.
Further, the generic naming only applied when the hostnames were not set. I believe it used the names when they were there ... so, was that wrong? When you say something "is broken", it's best to explain why it's broken. It might be very well horribly broken, but I'd need to know ... broken how? How do I show what is broken? What happens to the file, etc? Broken can mean lots of things ... tracebacks, unwanted behavior, error messages, etc. Being specific about what is broken makes it easier to understand what the actual problem is. I can't intuit that from the patch seeing it's mixed in with the code reorganization.
Sorry for longwinded comments, though I can't apply this to 1.6.4 because it is a bit intrusive for a release coming out in a few days; so I really need to know what the root problem is instead.
--Michael
-----Original Message----- From: cobbler-devel-bounces@lists.fedorahosted.org [mailto:cobbler-devel- bounces@lists.fedorahosted.org] On Behalf Of Michael DeHaan Sent: woensdag 6 mei 2009 16:53 To: cobbler development list Subject: Re: [PATCH] fix dhcp template generation
Vreman, Peter - Acision wrote:
The dhcp template generation is broken when there is no dns-name for interfaces set. The attached patch fixes this problem and does some cleanup to the code. Also the entries are now named with the system record name and interface instead of generic<idx>. This makes debugging the template easier.
Not applying... I'll explain.
I am generally not interested in "refactoring" in general -- the practice of shuffling code around has a high likelihood of introducing more bugs, and we do not generally have a large amount of folks that help with pre-release testing. Also, such patches are notoriously hard to review. We prefer that bugfixes are more surgical. So, if you're fixing a bug always strive to fix it in the least amount of lines possible. Be a ninja.
Cleanup -- where we decide it's needed -- needs to go on devel, while fixes need to go to master & devel.
Further, the generic naming only applied when the hostnames were not set. I believe it used the names when they were there ... so, was that wrong? When you say something "is broken", it's best to explain why it's broken. It might be very well horribly broken, but I'd need to know ... broken how? How do I show what is broken? What happens to the file, etc? Broken can mean lots of things ... tracebacks, unwanted behavior, error messages, etc. Being specific about what is broken makes it easier to understand what the actual problem is. I can't intuit that from the patch seeing it's mixed in with the code reorganization.
Sorry for longwinded comments, though I can't apply this to 1.6.4 because it is a bit intrusive for a release coming out in a few days; so I really need to know what the root problem is instead.
For master the most simple fix is to remove the blender_cache code and keep only the blended_system=utils.blender line. But this might have impact on the performance because the blending will then be done for every interface.
My patch removes the blender_cache and moves the blended_system generation to the systems-loop and uses the blended_system dictionary in the interface loop instead of retrieving a value from the distro.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
Vreman, Peter - Acision wrote:
-----Original Message----- From: cobbler-devel-bounces@lists.fedorahosted.org [mailto:cobbler-devel- bounces@lists.fedorahosted.org] On Behalf Of Michael DeHaan Sent: woensdag 6 mei 2009 16:53 To: cobbler development list Subject: Re: [PATCH] fix dhcp template generation
Vreman, Peter - Acision wrote:
The dhcp template generation is broken when there is no dns-name for interfaces set. The attached patch fixes this problem and does some cleanup to the code. Also the entries are now named with the system record name and interface instead of generic<idx>. This makes debugging the template easier.
Not applying... I'll explain.
I am generally not interested in "refactoring" in general -- the practice of shuffling code around has a high likelihood of introducing more bugs, and we do not generally have a large amount of folks that help with pre-release testing. Also, such patches are notoriously hard to review. We prefer that bugfixes are more surgical. So, if you're fixing a bug always strive to fix it in the least amount of lines possible. Be a ninja.
Cleanup -- where we decide it's needed -- needs to go on devel, while fixes need to go to master & devel.
Further, the generic naming only applied when the hostnames were not set. I believe it used the names when they were there ... so, was that wrong? When you say something "is broken", it's best to explain why it's broken. It might be very well horribly broken, but I'd need to know ... broken how? How do I show what is broken? What happens to the file, etc? Broken can mean lots of things ... tracebacks, unwanted behavior, error messages, etc. Being specific about what is broken makes it easier to understand what the actual problem is. I can't intuit that from the patch seeing it's mixed in with the code reorganization.
Sorry for longwinded comments, though I can't apply this to 1.6.4 because it is a bit intrusive for a release coming out in a few days; so I really need to know what the root problem is instead.
For master the most simple fix is to remove the blender_cache code and keep only the blended_system=utils.blender line. But this might have impact on the performance because the blending will then be done for every interface.
Blender_cache needs to stay.
Rather than tell me what the fix is, how about explaining what the problem is first? What "is broken" ?
What are the things you are trying to do, and what behavior are you seeing that is unexpected?
My patch removes the blender_cache and moves the blended_system generation to the systems-loop and uses the blended_system dictionary in the interface loop instead of retrieving a value from the distro.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler-devel
For master the most simple fix is to remove the blender_cache code and
keep only the blended_system=utils.blender line. But this might have impact on the performance because the blending will then be done for every interface.
Blender_cache needs to stay.
Rather than tell me what the fix is, how about explaining what the problem is first? What "is broken" ?
What are the things you are trying to do, and what behavior are you seeing that is unexpected?
The variable host is filled with the dns_name. Later this host variable is used as key in the blended_cache. That means that for all systems with an equal dns_name the same cache entry is used. This includes all systems with an empty dns_name that use a single cache entry. This cache entry is then the blended system record of the first system with an empty dns_name.
To make this behaviour visible you can print the blended_system["name"] on the screen and you will see that it lists always the same system name for all interfaces that have an empty dns_name field
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
Blender_cache needs to stay.
Rather than tell me what the fix is, how about explaining what the problem is first? What "is broken" ?
What are the things you are trying to do, and what behavior are you seeing that is unexpected?
The variable host is filled with the dns_name. Later this host variable is used as key in the blended_cache. That means that for all systems with an equal dns_name the same cache entry is used. This includes all systems with an empty dns_name that use a single cache entry. This cache entry is then the blended system record of the first system with an empty dns_name.
To make this behaviour visible you can print the blended_system["name"] on the screen and you will see that it lists always the same system name for all interfaces that have an empty dns_name field
Please find attached a new patch that only fixes the caching bug. The caching is now based on the system.name instead of the interface.dns_name field.
This patch can also be used on master. The same is for the host-name patch.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
Vreman, Peter - Acision wrote:
Blender_cache needs to stay.
Rather than tell me what the fix is, how about explaining what the problem is first? What "is broken" ?
What are the things you are trying to do, and what behavior are you seeing that is unexpected?
The variable host is filled with the dns_name. Later this host variable is used as key in the blended_cache. That means that for all systems with an equal dns_name the same cache entry is used. This includes all systems with an empty dns_name that use a single cache entry. This cache entry is then the blended system record of the first system with an empty dns_name.
To make this behaviour visible you can print the blended_system["name"] on the screen and you will see that it lists always the same system name for all interfaces that have an empty dns_name field
Please find attached a new patch that only fixes the caching bug. The caching is now based on the system.name instead of the interface.dns_name field.
This patch can also be used on master. The same is for the host-name patch.
Peter
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler-devel
Applied, thanks! (And thanks for the extended explanation too!)
--Michael
cobbler-devel@lists.fedorahosted.org
-
Michael DeHaan -
Vreman, Peter