On 03/30/2012 04:46 PM, Johannes Renner wrote:
On 03/27/2012 02:53 PM, Miroslav Suchý wrote:
I'm looking forward to see your patches.
Ok, here they are. They apply to spacewalk master and I tried to not forget
anything.
The last patch is about fixing checkstyle issues only, while the other ones are
about
patching the respective parts of Spacewalk, not very intrusive though.
Oh yes, there is two new required jar files, while RPM packages + spec files
can be
found here:
-
https://build.opensuse.org/package/show?package=susestudio-java-client&project=Java%3Abase
- https://build.opensuse.org/package/show?package=simple-xml&project=Java%3Abase
The reason why susestudio-java-client currently requires simple-xml is that it
is
supposed to run on Android as well, where javax.xml.* classes are not available.
+CREATE TABLE suseCredentials
...
+ type VARCHAR2(32) NOT NULL,
+CREATE TABLE rhnActionImageDeploy
...
+ image_type VARCHAR2(32) NOT NULL,
Hmm, can we normalize those table. I suppose that type is something
enumarated. In such case I would put those values in separate table and
here put just reference.
+ bridge_device VARCHAR2(32) DEFAULT('br0') NOT NULL,
Really not null? I'm sure I had some virtual machines without network.
+ url = row['download_url']
+ proxy_server = row['proxy_server']
+ proxy_user = row['proxy_user']
+ proxy_pass = row['proxy_pass']
+ mem_kb = row['mem_kb']
+ vcpus = row['vcpus']
+ bridge_device = row['bridge_device']
+
+ if row['image_type'] == 'vmx':
+ image_type = 'vmdk'
+ elif row['image_type'] == 'xen':
+ image_type = 'xen'
+ else:
+ raise InvalidAction("image.deploy: invalid image_type")
+
+
+ return (url, proxy_server, proxy_user, proxy_pass, mem_kb, vcpus,
image_type, bridge_device)
That assigment is overkill. Can you just:
return (row['download_url'] ...)
you will save 6 lines. But this is really no blocker. :)
+++ b/backend/server/action_extra_data/image.py
...
+def deploy(server_id, action_id, data={}):
+ if not data:
+ return
+ log_error("action_error.image.deploy: Should do something "
+ "useful with this data", server_id, action_id, data)
log_error? really?
That would couse traceback IIRC. if you do not know what to do with
returned data just call pass or log_debug.
+++ b/client/tools/rhn-virtualization/actions/image.py
...
+sys.path.append("/usr/share/rhn/")
+import virtualization.support as virt_support
+from virtualization.util import hyphenize_uuid
+
+sys.path.append("/usr/share/rhn/")
I'm sure one append is enough.
+IMAGE_BASE_PATH = "/var/lib/libvirt/images/"
+STUDIO_KVM_TEMPLATE = "/etc/sysconfig/rhn/studio-kvm-template.xml"
+STUDIO_XEN_TEMPLATE = "/etc/sysconfig/rhn/studio-xen-template.xml"
Can this be defined in config file? That would be awesome.
+if os.path.isfile(STUDIO_KVM_TEMPLATE):
+ f = open(STUDIO_KVM_TEMPLATE, 'r')
+ KVM_CREATE_TEMPLATE = f.read()
+ f.close()
+
+if os.path.isfile(STUDIO_XEN_TEMPLATE):
+ f = open(STUDIO_XEN_TEMPLATE, 'r')
+ XEN_CREATE_TEMPLATE = f.read()
+ f.close()
Duplicate code. I would prefer to create function which read content of
file.
+def deploy(downloadURL, proxyURL="", proxyUser="", proxyPass="",
memKB=524288, vCPUs=1, imageType="vmdk", virtBridge="xenbr0",
extraParams="",cache_only=None):
I would try to persuade you to not pass every single attribute as
specific attribute of this function.
We had several problems with that in past.
The problem is that if you would pass another attribute (e.g storage
location) next year, you could not do that easily.
Because if you pass this action with additional storage_location
attribute (even if it will be set to "") to client which could not
handle it, than it will traceback. So you will have to use and check
capabilities (which is PITA).
If you pass these values as dictionary, you will save yourself lots of
problems in future.
Additionally you do not use attribute cache_only at all. That is bad.
If cache_only is true you should not execute this action and you just
prepare this action. I.e. action packages.install will download packages
to /var/cache/yum so when the action is really executed, then those rpm
are already downloaded and the action is executed faster.
If you do not want to implement or it does not make sense for you, then
just put at the top of this function:
if cache_only:
return (0, "no-ops for caching", {})
Although as I can see, in your case you can take advance from this
framework and put this if before:
+ # image exists in /var/lib/libvirt/images/image-name now
which means after:
_getImage(fileName,downloadURL,proxySettings)
if it needs to be called.
+def _md5(path):
Could not you flip to sha1 or sha256?:
https://www.google.cz/search?q=md5%20not%20safe&hl=cs&lr=
I know we use md5 on several places, but new things should not start
with md5. But if this can not be done easily (e.g. due Suse Studio) I
can live with that.
+import hashlib
You should put in Requires python-hashlib then.
+c = pycurl.Curl()
Why not use directly
from urlgrabber.grabber import URLGrabber
which will shield you from internals?
+# this is not nice but tarfile.py does not support
+# sparse file writing :(
I did not test it but:
http://docs.python.org/library/tarfile.html
say:
The GNU tar format (GNU_FORMAT). It supports long filenames and
linknames, files bigger than 8 gigabytes and sparse files. It is the de
facto standard on GNU/Linux systems. tarfile fully supports the GNU tar
extensions for long names, sparse file support is read-only.
+def _generate_uuid():
Can we this move to virtualization.util?
+%dir %{rhn_conf_dir}
This is not needed. This directory is already owned by rhn-client-tools,
which are required by rhn-virtualization-common
+ private static final long serialVersionUID = 1438261396065921002L;
I do not see it used anywhere in code. Does it have some sense?
Can you squash patches 0004 and 0005 together before commiting/submiting?
--
Miroslav Suchy
Red Hat Satellite Engineering
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
