Merge lp:~justin-fathomdb/nova/raw-disk-images into lp:~hudson-openstack/nova/trunk
- raw-disk-images
- Merge into trunk
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~justin-fathomdb/nova/raw-disk-images | ||||
Merge into: | lp:~hudson-openstack/nova/trunk | ||||
Prerequisite: | lp:~justin-fathomdb/nova/check-subprocess-exit-code | ||||
Diff against target: |
344 lines (+109/-49) 8 files modified
nova/api/ec2/cloud.py (+10/-4) nova/compute/disk.py (+13/-5) nova/flags.py (+3/-0) nova/virt/libvirt.qemu.xml.template (+14/-10) nova/virt/libvirt.uml.xml.template (+11/-11) nova/virt/libvirt.xen.xml.template (+15/-10) nova/virt/libvirt_conn.py (+42/-9) tools/pip-requires (+1/-0) |
||||
To merge this branch: | bzr merge lp:~justin-fathomdb/nova/raw-disk-images | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
termie (community) | Needs Fixing | ||
Soren Hansen (community) | Needs Fixing | ||
Jay Pipes (community) | Needs Fixing | ||
Review via email:
|
This proposal has been superseded by a proposal from 2010-10-14.
Commit message
Description of the change
Add support for disk images that are 'raw', i.e. don't need a kernel or ramdisk. This makes it simpler to have Windows VMs, or to use VirtualBox as the host. To remain EC2 API compatible, if you specify the kernel as aki-00000000, this triggers the 'raw disk image' behaviour.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
justinsb (justin-fathomdb) wrote : | # |
Thanks for the review Jay.
The 'mapped device not found' exception is actually tricky; it is expected if we can't read the partitions after loopback, which happens if we have a non-raw-disk-image (i.e. a 'sparse' format). The test is to try to make that case obvious. I'm not sure what we should do here, but currently the caller (at least for virtualbox) ignores errors when trying to inject the machine image. I've therefore left it as a raw exception.Error, but clarified the message to make the intent clearer.
I've added the TODO. Again, there's a big picture question of what we should be injecting and how we should be injecting it; hopefully this will become clearer as we start launching more OSes and the guest agent shapes up (which is required for Windows, I believe)
As for Cheetah, I have no particular reason to choose Cheetah vs any other templating system - it looked like decent syntax, the project seemed not-dead, and there's an Ubuntu package. I would suggest that we do need _a_ templating system, for complex templating or optional inclusions (here I use it so that the kernel and ramdisk XML elements are only output if they are needed). What that templating system is, I don't really care. The one datapoint I have is that Cheetah "works for me", but if anyone else has more experience...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jay Pipes (jaypipes) wrote : | # |
> The 'mapped device not found' exception is actually tricky; it is expected if
> we can't read the partitions after loopback, which happens if we have a non-
> raw-disk-image (i.e. a 'sparse' format). The test is to try to make that case
> obvious. I'm not sure what we should do here, but currently the caller (at
> least for virtualbox) ignores errors when trying to inject the machine image.
> I've therefore left it as a raw exception.Error, but clarified the message to
> make the intent clearer.
OK, understood. I would still recommend making a new exception class in /nova/exception.py (MappedDeviceNo
> I've added the TODO. Again, there's a big picture question of what we should
> be injecting and how we should be injecting it; hopefully this will become
> clearer as we start launching more OSes and the guest agent shapes up (which
> is required for Windows, I believe)
Cool :)
> As for Cheetah, I have no particular reason to choose Cheetah vs any other
> templating system - it looked like decent syntax, the project seemed not-dead,
> and there's an Ubuntu package. I would suggest that we do need _a_ templating
> system, for complex templating or optional inclusions (here I use it so that
> the kernel and ramdisk XML elements are only output if they are needed). What
> that templating system is, I don't really care. The one datapoint I have is
> that Cheetah "works for me", but if anyone else has more experience...
Yeah, I'm not sure, which is why I asked others for opinions. I'm not familiar with Python's templating frameworks/
-jay
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Soren Hansen (soren) wrote : | # |
I'm cool with Cheetah. I've used it before and I don't know of another templating system that's supposed to be the canonical one.
As for the udev network thing: Ubuntu will refrain from storing the MAC->interface-name mapping if the MAC address is "locally administered". nova.utils.
> @defer.
> -def inject_data(image, key=None, net=None, partition=None, execute=None):
> +def inject_data( image, key=None, net=None, dns=None,
> + remove_
> + partition=None, execute=None):
PEP-8 says not to put whitespace immediately inside parentheses.
=== modified file 'nova/virt/
--- nova/virt/
+++ nova/virt/
@@ -260,13 +275,28 @@
def toXml(self, instance):
# TODO(termie): cache?
+ template_contents = open(FLAGS.
xml_info = instance.
# TODO(joshua): Make this xml express the attached disks as well
Since I added UML capabilities to Nova, the template to use is already
put into self.libvirt_xml in __init__. Please use that instead, as
it's based on a different template if libvirt_type is "uml". Also, the
changes you made to libvirt.
for libvirt.
Other than that, it looks good, and I'm really looking forward to seeing
this land! :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Vish Ishaya (vishvananda) wrote : | # |
Justin, did we lose you? I'd like to get this merged, but it looks like there are a few fixes requested.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
termie (termie) wrote : | # |
I'll just weigh in here on my thoughts about Cheetah, mainly that I think it is bad to allow much logic in the template code. Agreed that we all need conditionals, but seeing 'getVar' in there really throws some flags for me.
A much much smaller but powerful and declarative-style templating engine that I've grown to prefer is called jsontemplate, http://
Our usage is pretty minimal here so the syntax isn't shockingly different but in Cheetah
#if $getVar('kernel', None)
#if $getVar('ramdisk', None)
#end if
#end if
and
template_contents = open(FLAGS.
str(Template(
becomes
{.section kernel}
{.section ramdisk}
{.end}
{.end}
and
jsontemplate.
The other benefits are that the engine is a single file and easily extensible with formatters
The "json" part of it comes from the fact that it was designed and implemented in python and javascript at the same time.
There are a variety of style nits (extra spaces before or around things, importing objects instead of modules, using tabs instead of spaces in the xml), so please take another scan over the code to resolve those :)
Other than those discussion points looks like a great addition to our core functionality :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Rick Clark (dendrobates) wrote : | # |
If Justin is willing to fix this up. I am fine with moving forward with cheetah. We can have an in depth discussions about templating engines at the summit if we want.
Today is the last day to get any preexisting feature merge requests merged.
The code base has changed quite a bit since this was proposed. It may be a difficult task to get it done today, but if Justin is will to do the work today, we will merge it.
- 163. By Justin Santa Barbara <justinsb@justinsb-desktop>
-
Merged with trunk, fixed broken stuff
- 164. By Justin Santa Barbara <justinsb@justinsb-desktop>
-
Minimized diff, fixed formatting
- 165. By Justin Santa Barbara <justinsb@justinsb-desktop>
-
Removed stray spaces that were causing an unnecessary diff line
- 166. By Justin Santa Barbara <justinsb@justinsb-desktop>
-
Removed 'and True' oddity
Unmerged revisions
Preview Diff
1 | === modified file 'nova/api/ec2/cloud.py' |
2 | --- nova/api/ec2/cloud.py 2010-10-14 06:17:40 +0000 |
3 | +++ nova/api/ec2/cloud.py 2010-10-14 20:50:58 +0000 |
4 | @@ -794,9 +794,15 @@ |
5 | kernel_id = kwargs.get('kernel_id', kernel_id) |
6 | ramdisk_id = kwargs.get('ramdisk_id', ramdisk_id) |
7 | |
8 | + if kernel_id == str(FLAGS.null_kernel): |
9 | + kernel_id = None |
10 | + ramdisk_id = None |
11 | + |
12 | # make sure we have access to kernel and ramdisk |
13 | - images.get(context, kernel_id) |
14 | - images.get(context, ramdisk_id) |
15 | + if kernel_id: |
16 | + images.get(context, kernel_id) |
17 | + if ramdisk_id: |
18 | + images.get(context, ramdisk_id) |
19 | |
20 | logging.debug("Going to run %s instances...", num_instances) |
21 | launch_time = time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime()) |
22 | @@ -823,8 +829,8 @@ |
23 | base_options = {} |
24 | base_options['state_description'] = 'scheduling' |
25 | base_options['image_id'] = image_id |
26 | - base_options['kernel_id'] = kernel_id |
27 | - base_options['ramdisk_id'] = ramdisk_id |
28 | + base_options['kernel_id'] = kernel_id or '' |
29 | + base_options['ramdisk_id'] = ramdisk_id or '' |
30 | base_options['reservation_id'] = reservation_id |
31 | base_options['key_data'] = key_data |
32 | base_options['key_name'] = kwargs.get('key_name', None) |
33 | |
34 | === modified file 'nova/compute/disk.py' |
35 | --- nova/compute/disk.py 2010-08-16 12:16:21 +0000 |
36 | +++ nova/compute/disk.py 2010-10-14 20:50:58 +0000 |
37 | @@ -96,7 +96,7 @@ |
38 | If partition is not specified it mounts the image as a single partition. |
39 | |
40 | """ |
41 | - out, err = yield execute('sudo losetup -f --show %s' % image) |
42 | + out, err = yield execute('sudo losetup --find --show %s' % image) |
43 | if err: |
44 | raise exception.Error('Could not attach image to loopback: %s' % err) |
45 | device = out.strip() |
46 | @@ -110,6 +110,15 @@ |
47 | partition) |
48 | else: |
49 | mapped_device = device |
50 | + |
51 | + # We can only loopback mount raw images. If the device isn't there, |
52 | + # it's normally because it's a .vmdk or a .vdi etc |
53 | + if not os.path.exists(mapped_device): |
54 | + raise exception.Error( |
55 | + 'Mapped device was not found (we can only inject raw disk images): %s' |
56 | + % mapped_device) |
57 | + |
58 | + # Configure ext2fs so that it doesn't auto-check every N boots |
59 | out, err = yield execute('sudo tune2fs -c 0 -i 0 %s' % mapped_device) |
60 | |
61 | tmpdir = tempfile.mkdtemp() |
62 | @@ -137,12 +146,12 @@ |
63 | yield execute('sudo kpartx -d %s' % device) |
64 | finally: |
65 | # remove loopback |
66 | - yield execute('sudo losetup -d %s' % device) |
67 | + yield execute('sudo losetup --detach %s' % device) |
68 | |
69 | |
70 | @defer.inlineCallbacks |
71 | def _inject_key_into_fs(key, fs, execute=None): |
72 | - sshdir = os.path.join(os.path.join(fs, 'root'), '.ssh') |
73 | + sshdir = os.path.join(fs, 'root', '.ssh') |
74 | yield execute('sudo mkdir -p %s' % sshdir) # existing dir doesn't matter |
75 | yield execute('sudo chown root %s' % sshdir) |
76 | yield execute('sudo chmod 700 %s' % sshdir) |
77 | @@ -152,7 +161,6 @@ |
78 | |
79 | @defer.inlineCallbacks |
80 | def _inject_net_into_fs(net, fs, execute=None): |
81 | - netfile = os.path.join(os.path.join(os.path.join( |
82 | - fs, 'etc'), 'network'), 'interfaces') |
83 | + netfile = os.path.join(fs, 'etc', 'network', 'interfaces') |
84 | yield execute('sudo tee %s' % netfile, net) |
85 | |
86 | |
87 | === modified file 'nova/flags.py' |
88 | --- nova/flags.py 2010-10-01 20:06:14 +0000 |
89 | +++ nova/flags.py 2010-10-14 20:50:58 +0000 |
90 | @@ -201,6 +201,9 @@ |
91 | 'default ramdisk to use, testing only') |
92 | DEFINE_string('default_instance_type', 'm1.small', |
93 | 'default instance type to use, testing only') |
94 | +DEFINE_string('null_kernel', 'aki-00000000', |
95 | + 'kernel image that indicates not to use a kernel, ' |
96 | + ' but to use a raw disk image instead') |
97 | |
98 | DEFINE_string('vpn_image_id', 'ami-CLOUDPIPE', 'AMI for cloudpipe vpn server') |
99 | DEFINE_string('vpn_key_suffix', |
100 | |
101 | === modified file 'nova/virt/libvirt.qemu.xml.template' |
102 | --- nova/virt/libvirt.qemu.xml.template 2010-09-27 11:13:29 +0000 |
103 | +++ nova/virt/libvirt.qemu.xml.template 2010-10-14 20:50:58 +0000 |
104 | @@ -1,24 +1,28 @@ |
105 | -<domain type='%(type)s'> |
106 | - <name>%(name)s</name> |
107 | +<domain type='${type}'> |
108 | + <name>${name}</name> |
109 | <os> |
110 | <type>hvm</type> |
111 | - <kernel>%(basepath)s/kernel</kernel> |
112 | - <initrd>%(basepath)s/ramdisk</initrd> |
113 | +#if $getVar('kernel', None) |
114 | + <kernel>${kernel}</kernel> |
115 | + #if $getVar('ramdisk', None) |
116 | + <initrd>${ramdisk}</initrd> |
117 | + #end if |
118 | <cmdline>root=/dev/vda1 console=ttyS0</cmdline> |
119 | +#end if |
120 | </os> |
121 | <features> |
122 | <acpi/> |
123 | </features> |
124 | - <memory>%(memory_kb)s</memory> |
125 | - <vcpu>%(vcpus)s</vcpu> |
126 | + <memory>${memory_kb}</memory> |
127 | + <vcpu>${vcpus}</vcpu> |
128 | <devices> |
129 | <disk type='file'> |
130 | - <source file='%(basepath)s/disk'/> |
131 | + <source file='${disk}'/> |
132 | <target dev='vda' bus='virtio'/> |
133 | </disk> |
134 | <interface type='bridge'> |
135 | - <source bridge='%(bridge_name)s'/> |
136 | - <mac address='%(mac_address)s'/> |
137 | + <source bridge='${bridge_name}'/> |
138 | + <mac address='${mac_address}'/> |
139 | <!-- <model type='virtio'/> CANT RUN virtio network right now --> |
140 | <filterref filter="nova-instance-%(name)s"> |
141 | <parameter name="IP" value="%(ip_address)s" /> |
142 | @@ -26,7 +30,7 @@ |
143 | </filterref> |
144 | </interface> |
145 | <serial type="file"> |
146 | - <source path='%(basepath)s/console.log'/> |
147 | + <source path='${basepath}/console.log'/> |
148 | <target port='1'/> |
149 | </serial> |
150 | </devices> |
151 | |
152 | === modified file 'nova/virt/libvirt.uml.xml.template' |
153 | --- nova/virt/libvirt.uml.xml.template 2010-09-27 11:13:29 +0000 |
154 | +++ nova/virt/libvirt.uml.xml.template 2010-10-14 20:50:58 +0000 |
155 | @@ -1,26 +1,26 @@ |
156 | -<domain type='%(type)s'> |
157 | - <name>%(name)s</name> |
158 | - <memory>%(memory_kb)s</memory> |
159 | +<domain type='${type}'> |
160 | + <name>${name}</name> |
161 | + <memory>${memory_kb}</memory> |
162 | <os> |
163 | - <type>%(type)s</type> |
164 | + <type>${type}</type> |
165 | <kernel>/usr/bin/linux</kernel> |
166 | <root>/dev/ubda1</root> |
167 | </os> |
168 | <devices> |
169 | <disk type='file'> |
170 | - <source file='%(basepath)s/disk'/> |
171 | + <source file='${disk}'/> |
172 | <target dev='ubd0' bus='uml'/> |
173 | </disk> |
174 | <interface type='bridge'> |
175 | - <source bridge='%(bridge_name)s'/> |
176 | - <mac address='%(mac_address)s'/> |
177 | - <filterref filter="nova-instance-%(name)s"> |
178 | - <parameter name="IP" value="%(ip_address)s" /> |
179 | - <parameter name="DHCPSERVER" value="%(dhcp_server)s" /> |
180 | + <source bridge='${bridge_name}'/> |
181 | + <mac address='${mac_address}'/> |
182 | + <filterref filter="nova-instance-${name}"> |
183 | + <parameter name="IP" value="${ip_address}" /> |
184 | + <parameter name="DHCPSERVER" value="${dhcp_server}" /> |
185 | </filterref> |
186 | </interface> |
187 | <console type="file"> |
188 | - <source path='%(basepath)s/console.log'/> |
189 | + <source path='${basepath}/console.log'/> |
190 | </console> |
191 | </devices> |
192 | </domain> |
193 | |
194 | === modified file 'nova/virt/libvirt.xen.xml.template' |
195 | --- nova/virt/libvirt.xen.xml.template 2010-09-20 09:33:35 +0000 |
196 | +++ nova/virt/libvirt.xen.xml.template 2010-10-14 20:50:58 +0000 |
197 | @@ -1,28 +1,33 @@ |
198 | -<domain type='%(type)s'> |
199 | - <name>%(name)s</name> |
200 | +<domain type='${type}'> |
201 | + <name>${name}</name> |
202 | <os> |
203 | <type>linux</type> |
204 | - <kernel>%(basepath)s/kernel</kernel> |
205 | - <initrd>%(basepath)s/ramdisk</initrd> |
206 | +#if $getVar('kernel', None) |
207 | + <kernel>${kernel}</kernel> |
208 | + #if $getVar('ramdisk', None) |
209 | + <initrd>${ramdisk}</initrd> |
210 | + #end if |
211 | + <cmdline>root=/dev/vda1 console=ttyS0</cmdline> |
212 | +#end if |
213 | <root>/dev/xvda1</root> |
214 | <cmdline>ro</cmdline> |
215 | </os> |
216 | <features> |
217 | <acpi/> |
218 | </features> |
219 | - <memory>%(memory_kb)s</memory> |
220 | - <vcpu>%(vcpus)s</vcpu> |
221 | + <memory>${memory_kb}</memory> |
222 | + <vcpu>${vcpus}</vcpu> |
223 | <devices> |
224 | <disk type='file'> |
225 | - <source file='%(basepath)s/disk'/> |
226 | + <source file='${disk}'/> |
227 | <target dev='sda' /> |
228 | </disk> |
229 | <interface type='bridge'> |
230 | - <source bridge='%(bridge_name)s'/> |
231 | - <mac address='%(mac_address)s'/> |
232 | + <source bridge='${bridge_name}'/> |
233 | + <mac address='${mac_address}'/> |
234 | </interface> |
235 | <console type="file"> |
236 | - <source path='%(basepath)s/console.log'/> |
237 | + <source path='${basepath}/console.log'/> |
238 | <target port='1'/> |
239 | </console> |
240 | </devices> |
241 | |
242 | === modified file 'nova/virt/libvirt_conn.py' |
243 | --- nova/virt/libvirt_conn.py 2010-10-13 17:13:35 +0000 |
244 | +++ nova/virt/libvirt_conn.py 2010-10-14 20:50:58 +0000 |
245 | @@ -42,6 +42,8 @@ |
246 | from nova.compute import power_state |
247 | from nova.virt import images |
248 | |
249 | +from Cheetah.Template import Template |
250 | + |
251 | libvirt = None |
252 | libxml2 = None |
253 | |
254 | @@ -321,16 +323,26 @@ |
255 | |
256 | if not os.path.exists(basepath('disk')): |
257 | yield images.fetch(inst.image_id, basepath('disk-raw'), user, project) |
258 | - if not os.path.exists(basepath('kernel')): |
259 | - yield images.fetch(inst.kernel_id, basepath('kernel'), user, project) |
260 | - if not os.path.exists(basepath('ramdisk')): |
261 | - yield images.fetch(inst.ramdisk_id, basepath('ramdisk'), user, project) |
262 | + |
263 | + using_kernel = inst.kernel_id and True |
264 | + if using_kernel: |
265 | + if not os.path.exists(basepath('kernel')): |
266 | + yield images.fetch(inst.kernel_id, basepath('kernel'), user, project) |
267 | + if not os.path.exists(basepath('ramdisk')): |
268 | + yield images.fetch(inst.ramdisk_id, basepath('ramdisk'), user, project) |
269 | |
270 | execute = lambda cmd, process_input=None: \ |
271 | process.simple_execute(cmd=cmd, |
272 | process_input=process_input, |
273 | check_exit_code=True) |
274 | |
275 | + # For now, we assume that if we're not using a kernel, we're using a |
276 | + # partitioned disk image where the target partition is the first |
277 | + # partition |
278 | + target_partition = None |
279 | + if not using_kernel: |
280 | + target_partition = "1" |
281 | + |
282 | key = str(inst['key_data']) |
283 | net = None |
284 | network_ref = db.network_get_by_instance(None, inst['id']) |
285 | @@ -349,10 +361,19 @@ |
286 | if net: |
287 | logging.info('instance %s: injecting net into image %s', |
288 | inst['name'], inst.image_id) |
289 | - yield disk.inject_data(basepath('disk-raw'), key, net, execute=execute) |
290 | + try: |
291 | + yield disk.inject_data(basepath('disk-raw'), key, net, |
292 | + partition=target_partition, |
293 | + execute=execute) |
294 | + except Exception as e: |
295 | + # This could be a windows image, or a vmdk format disk |
296 | + logging.warn('instance %s: ignoring error injecting data' |
297 | + ' into image %s (%s)', |
298 | + inst['name'], inst.image_id, e) |
299 | |
300 | - if os.path.exists(basepath('disk')): |
301 | - yield process.simple_execute('rm -f %s' % basepath('disk')) |
302 | + if using_kernel: |
303 | + if os.path.exists(basepath('disk')): |
304 | + yield process.simple_execute('rm -f %s' % basepath('disk')) |
305 | |
306 | bytes = (instance_types.INSTANCE_TYPES[inst.instance_type]['local_gb'] |
307 | * 1024 * 1024 * 1024) |
308 | @@ -383,10 +404,22 @@ |
309 | 'mac_address': instance['mac_address'], |
310 | 'ip_address': ip_address, |
311 | 'dhcp_server': dhcp_server } |
312 | - libvirt_xml = self.libvirt_xml % xml_info |
313 | + |
314 | + if xml_info['kernel_id']: |
315 | + xml_info['kernel'] = xml_info['basepath'] + "/kernel" |
316 | + |
317 | + if xml_info['ramdisk_id']: |
318 | + xml_info['ramdisk'] = xml_info['basepath'] + "/ramdisk" |
319 | + |
320 | + if xml_info['ramdisk_id'] or xml_info['kernel_id']: |
321 | + xml_info['disk'] = xml_info['basepath'] + "/disk" |
322 | + else: |
323 | + xml_info['disk'] = xml_info['basepath'] + "/disk-raw" |
324 | + |
325 | + xml = str(Template(self.libvirt_xml, searchList=[ xml_info ] )) |
326 | logging.debug('instance %s: finished toXML method', instance['name']) |
327 | |
328 | - return libvirt_xml |
329 | + return xml |
330 | |
331 | def get_info(self, instance_name): |
332 | virt_dom = self._conn.lookupByName(instance_name) |
333 | |
334 | === modified file 'tools/pip-requires' |
335 | --- tools/pip-requires 2010-10-01 18:02:51 +0000 |
336 | +++ tools/pip-requires 2010-10-14 20:50:58 +0000 |
337 | @@ -2,6 +2,7 @@ |
338 | pep8==0.5.0 |
339 | pylint==0.19 |
340 | IPy==0.70 |
341 | +Cheetah==2.4.2.1 |
342 | M2Crypto==0.20.2 |
343 | amqplib==0.6.1 |
344 | anyjson==0.2.4 |
Hi!
Good stuff, Justin! ++ for long option names :)
A couple tiny nits, though :)
27 + if not os.path. exists( mapped_ device) : Error(' Mapped device was not found: %s' % mapped_device)
28 + raise exception.
There is a NotFound exception class in /nova/exception.py that may be better that raising the generic error here. Or, alternately, you could raise IOError, as that is what is raised by open() when the file does not exist?
77 + # This is correct for Ubuntu, but might not be right for other distros net.rules' )
78 + rulesfile = os.path.join(fs, 'etc', 'udev', 'rules.d', '70-persistent-
79 + yield execute('rm -f %s' % rulesfile)
Best to add a TODO(justinsb) there so it's noted for future improvements :)
Finally, not sure about making Cheetah a dependency here...what do other think?
Cheers!
jay