FW: [PATCH] Logging and cli error patches for master

[Vreman, Peter]

I still have the patches below for cobbler. Can the patches be integrated upstream?

The Koan and CLI error handling patches are real issues that break scripts and KVM virsh console support.

Regards,
Peter

From: [e-mail] [mailto:[e-mail]] On Behalf Of Vreman, Peter
Sent: woensdag 21 oktober 2009 8:01
To: cobbler development list
Subject: [PATCH] Logging and cli error patches for master

I have the following list of patches for master available at http://github.com/pvreman/cobbler/commits/master/.

Koan:
+ afda8a88249ed3a124651a25fc52e9f78d1ab414 update string_to_hash and hash_to_string from cobblerd to support multiple kernel options with the same name

Logging improvements for background tasks:
+ 90c00b3abba0d15a2e6857469ea24fad00f95b0b make sync actions always verbose, before the copy actions were only logged without context
+ 1a09c11eb2c546d53c366b26ab4acce96d3bc314 logging for creation of dhcp and dns configurations
+ 9867963dc8eb11052ec3521c5baf18803fe826e9 log creation of pxe files
+ 9ee3887130fe2beb49d332a3420166398595242d log updates, add criteria to find_items and make more debug

CLI error handling:
+ e9d0cf90df35dd351c954c097a28ddf752edeb6b exit with error code 1 if command fails
+ 92ba9bb9c5594223083029cc8676b5c20d826c33 raise error in blended data if profile or system not found

WebUI redundant code:
+ efbc6519b34e9b9c95a9298d68fbf340b0a82c1d remove duplicate code

Regards,
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.

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.

[Scott Henson]

Excerpts from Vreman, Peter's message of Tue Jan 12 05:51:13 -0500 2010:
> I still have the patches below for cobbler. Can the patches be integrated
> upstream?

I'll have a look at these today.  I'll try to get them pushed to my
personal branch tonight and the fedorahosted in the next two days
(pending some smoke testing).

[Scott Henson]

Excerpts from Vreman, Peter's message of Tue Jan 12 05:51:13 -0500 2010:
> Koan:
> afda8a88249ed3a124651a25fc52e9f78d1ab414 update string_to_hash and
>
> hash_to_string from cobblerd to support multiple kernel options with the same
> name

Instead of type(hash) != dict, try isinstance(hash, dict), it should
catch subclasses of dict as well.  Otherwise this patch looks good.
> Logging improvements for background tasks:
> 90c00b3abba0d15a2e6857469ea24fad00f95b0b make sync actions always verbose,
>
> before the copy actions were only logged without context

Why?  The patch looks good, just curious why make sync actions always verbose?
> 1a09c11eb2c546d53c366b26ab4acce96d3bc314 logging for creation of dhcp and dns
>
> configurations
> 9867963dc8eb11052ec3521c5baf18803fe826e9 log creation of pxe files
> 9ee3887130fe2beb49d332a3420166398595242d log updates, add criteria to
>
> find_items and make more debug

Same question, otherwise looks good.  Making the items debug makes
sense though.
> CLI error handling:
> e9d0cf90df35dd351c954c097a28ddf752edeb6b exit with error code 1 if command
>
> fails
> 92ba9bb9c5594223083029cc8676b5c20d826c33 raise error in blended data if
>
> profile or system not found

Both look good.  I'm applying them now.
> WebUI redundant code:
> efbc6519b34e9b9c95a9298d68fbf340b0a82c1d remove duplicate code

I don't know much about the WUI, if someone else can verify that this
works I'll merge it.

I'll wait for a response on the balance of these before merging up.

[Vreman, Peter]

> -----Original Message-----
> From: [e-mail] [mailto:cobbler-
> [e-mail]] On Behalf Of Scott Henson
> Sent: woensdag 13 januari 2010 16:22
> To: cobbler-devel
> Subject: Re: FW: [PATCH] Logging and cli error patches for master
>
> Excerpts from Vreman, Peter's message of Tue Jan 12 05:51:13 -0500
> 2010:
> > Koan:
> > afda8a88249ed3a124651a25fc52e9f78d1ab414 update string_to_hash and
> >
> > hash_to_string from cobblerd to support multiple kernel options with
> > the same name
>
> Instead of type(hash) != dict, try isinstance(hash, dict), it should
> catch subclasses of dict as well.  Otherwise this patch looks good.

The change to isinstance() shall also be applied in cobbler/utils.py:hash_to_string()
> > Logging improvements for background tasks:
> > 90c00b3abba0d15a2e6857469ea24fad00f95b0b make sync actions always
> > verbose,
> >
> > before the copy actions were only logged without context
>
> Why?  The patch looks good, just curious why make sync actions always
> verbose?
> > 1a09c11eb2c546d53c366b26ab4acce96d3bc314 logging for creation of
> > dhcp and dns
> >
> > configurations
> > 9867963dc8eb11052ec3521c5baf18803fe826e9 log creation of pxe files
> > 9ee3887130fe2beb49d332a3420166398595242d log updates, add criteria
> > to
> >
> > find_items and make more debug
>
> Same question, otherwise looks good.  Making the items debug makes
> sense though.

I did this change to have a single configuration to change the verbosity of the logging. Now the logging depends on both the log level and the verbosity flag. When the verbose options is not set you get logging without contexts which action did make a call to a failed procedure.

For example the generation of a PXE file was not traced. When you change the netboot flag from CLI/WebUI it was never logged that a PXE file in tftpboot was created. And for sync the logging was inconsistent, the removal of the PXE files was logged, but not the creation.
> > CLI error handling:
> > e9d0cf90df35dd351c954c097a28ddf752edeb6b exit with error code 1 if
> > command
> >
> > fails
> > 92ba9bb9c5594223083029cc8676b5c20d826c33 raise error in blended
>
> data
> > if
> >
> > profile or system not found
>
> Both look good.  I'm applying them now.
> > WebUI redundant code:
> > efbc6519b34e9b9c95a9298d68fbf340b0a82c1d remove duplicate code
>
> I don't know much about the WUI, if someone else can verify that this
> works I'll merge it.
>
> I'll wait for a response on the balance of these before merging up.

There was one additional commit I missed in the mail:

remove none from interface dict 	99ffc8ebd59f6209828385707f11ae1202dacc34

This fixes a single issue of RHEL4 XMLRPC not supporting None values. But it looks like it is a general problem.
For the responses from the cobblerd the None values are translated to ~. But for the POST requests they are not.
The commit fixes one situation, but i have already seen another place. But no time yet to patch that also.

Thanks,
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.

[Scott Henson]

Excerpts from Vreman, Peter's message of Wed Jan 13 11:33:30 -0500 2010:
> The change to isinstance() shall also be applied in
> cobbler/utils.py:hash_to_string()

I've gone ahead and checked this closer.  It seems that isinstance
doesn't work on el2.1AS, which means that we can't use it in koan
unless we want to drop support for that.

As far as cobbler goes, I'd love to use it, as it should give a more
accurate response.  It should work on everything down to el3.  I've
gone ahead and merged these changes because there are plenty of
examples of using that idiom all over cobbler.  I'll probably look
into redoing this sometime soonish.
> I did this change to have a single configuration to change the verbosity of the
> logging. Now the logging depends on both the log level and the verbosity flag.
> When the verbose options is not set you get logging without contexts which
> action did make a call to a failed procedure.
>
> For example the generation of a PXE file was not traced. When you change the
> netboot flag from CLI/WebUI it was never logged that a PXE file in tftpboot was
> created. And for sync the logging was inconsistent, the removal of the PXE
> files was logged, but not the creation.

Sounds good.  Merging to my github.
> There was one additional commit I missed in the mail:
>
> remove none from interface dict     99ffc8ebd59f6209828385707f11ae1202dacc34
>
> This fixes a single issue of RHEL4 XMLRPC not supporting None values. But it
> looks like it is a general problem.
> For the responses from the cobblerd the None values are translated to ~. But
> for the POST requests they are not.
> The commit fixes one situation, but i have already seen another place. But no
> time yet to patch that also.

Sounds good.  Merged everything.  I'll try to test in the next couple
of days and merge it up to fh.

[Vreman, Peter]

> > I did this change to have a single configuration to change the
> > verbosity of the logging. Now the logging depends on both the log
>
> level and the verbosity flag.
> > When the verbose options is not set you get logging without contexts
> > which action did make a call to a failed procedure.
> >
> > For example the generation of a PXE file was not traced. When you
> > change the netboot flag from CLI/WebUI it was never logged that a PXE
> > file in tftpboot was created. And for sync the logging was
> > inconsistent, the removal of the PXE files was logged, but not the
>
> creation.
>
> Sounds good.  Merging to my github.
> > There was one additional commit I missed in the mail:
> >
> > remove none from interface dict
>
> 99ffc8ebd59f6209828385707f11ae1202dacc34
> > This fixes a single issue of RHEL4 XMLRPC not supporting None values.
> > But it looks like it is a general problem.
> > For the responses from the cobblerd the None values are translated to
> > ~. But for the POST requests they are not.
> > The commit fixes one situation, but i have already seen another
>
> place.
> > But no time yet to patch that also.
>
> Sounds good.  Merged everything.  I'll try to test in the next couple
> of days and merge it up to fh.

I don't see the patches yet in the fedorahosted tree. When is it expected that these patches are pushed to the fedorahosted tree?

Thanks,
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.

[Cristian Ciupitu]

> From: Scott Henson 
> To: cobbler-devel 
> Sent: Wed, January 13, 2010 5:21:50 PM
> Subject: Re: FW: [PATCH] Logging and cli error patches for master
>
> Excerpts from Vreman, Peter's message of Tue Jan 12 05:51:13 -0500 2010:
> > Koan:
> > afda8a88249ed3a124651a25fc52e9f78d1ab414 update string_to_hash and
> >
> > hash_to_string from cobblerd to support multiple kernel options with the same
> > name
>
> Instead of type(hash) != dict, try isinstance(hash, dict), it should
> catch subclasses of dict as well.  Otherwise this patch looks good.

(quoting from an old email of mine)
If string values are legacy why not test against (base)str, so that we
could use dictionary like objects in future?

Also, you could make buffer a list of strings ("key=val") and then join
them by doing " ".join(buffer).

Cheers,
Cristian