Merge lp:~rconradharris/glance/lp723453 into lp:~glance-coresec/glance/cactus-trunk

[Rick Harris]

Makes --kernel and --ramdisk required arguments for glance-upload since Nova currently requires them.

The `null_kernel` concept in Nova is slated to be removed, so this is really just a stop gap measure.

[Jay Pipes (jaypipes)]

lgtm. please check out my api-image-format merge prop to see where I'm proposing things start moving...

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Vish Ishaya (vishvananda)]

Maybe I'm missing something but why are kernel and ramdisk required? Admittedly I"m not using glance, but If i create an image without a kernel and ramdisk in the objectstore then launch it, tt launches it as a whole disk. There is no need for me to specify the "null_kernel" to get it to boot properly.

review: Needs Information

[Rick Harris (rconradharris)]

Vish:

I noticed this code in Nova's compute/api.py

        if kernel_id == str(FLAGS.null_kernel):
            kernel_id = None
            ramdisk_id = None
            LOG.debug(_("Creating a raw instance"))

I read this as Nova having a requirement that 'nokernel' be specified when an image is raw (or in my case, VHD).

It sounds like some code paths (maybe all) don't in-practice require this, but for my own sanity, I wanted the theoretical requirement to be mirrored in Glance until it is (hopefully) removed from Nova entirely.

Basically, make the wonkiness consistent, then fix everywhere.

(I wanted to punt on fixing the `null_kernel` issue since it's going to require quite a bit of testing to make sure we don't break any of the multitudinous image-lookup-fetch-store code-paths)

[Vish Ishaya (vishvananda)]

Yes you can pass in null kernel to force it to boot raw, but I think this is really unnecessary if the image is created properly. It is possible that euca-upload-bundle doesn't allow you to pass an empty kernel and ramdisk, but I think you can.

Vish

On Feb 24, 2011, at 12:14 PM, Rick Harris wrote:

> Vish:
>
> I noticed this code in Nova's compute/api.py
>
> if kernel_id == str(FLAGS.null_kernel):
> kernel_id = None
> ramdisk_id = None
> LOG.debug(_("Creating a raw instance"))
>
> I read this as Nova having a requirement that 'nokernel' be specified when an image is raw (or in my case, VHD).
>
> It sounds like some code paths (maybe all) don't in-practice require this, but for my own sanity, I wanted the theoretical requirement to be mirrored in Glance until it is (hopefully) removed from Nova entirely.
>
> Basically, make the wonkiness consistent, then fix everywhere.
>
> (I wanted to punt on fixing the `null_kernel` issue since it's going to require quite a bit of testing to make sure we don't break any of the multitudinous image-lookup-fetch-store code-paths)
>
>
>
>
>
>
> --
> https://code.launchpad.net/~rconradharris/glance/lp723453/+merge/51181
> You are reviewing the proposed merge of lp:~rconradharris/glance/lp723453 into lp:glance.

[Rick Harris (rconradharris)]

> Yes you can pass in null kernel to force it to boot raw, but I think this is really unnecessary

Yup, that's why I think barring someone coming forward with a compelling reason, I think we should drop the `null_kernel` concept in Nova.

I'd really hate to see this patch cause *more* confusion, so, I'm going to put this patch on hold while I investigate some alternatives.

[Rick Harris (rconradharris)]

Per Vish's comments, I've removed the requirement for `kernel` and `ramdisk` in the glance-upload tool.

This patch just updates it to respect the values regardless of `image_type`.

[Vish Ishaya (vishvananda)]

cool. LGTM

review: Approve

=== modified file 'bin/glance-upload'
--- bin/glance-upload	2011-02-16 18:55:08 +0000
+++ bin/glance-upload	2011-02-24 23:09:15 +0000
@@ -19,19 +19,29 @@
 """
 Upload an image into Glance
 
-Usage:
-
-Raw:
-
-    glance-upload <filename> <name>
-
-Kernel-outside:
-
-    glance-upload --type=kernel <filename> <name>
-    glance-upload --type=ramdisk <filename> <name>
-    glance-upload --type=machine --kernel=KERNEL_ID --ramdisk=RAMDISK_ID \
-                     <filename> <name>
-
+Usage
+=====
+
+    Machine - Kernel outside of the image
+    -------------------------------------
+
+        glance-upload --type=kernel <filename> <name>
+        glance-upload --type=ramdisk <filename> <name>
+        glance-upload --type=machine --kernel=KERNEL_ID --ramdisk=RAMDISK_ID \
+                         <filename> <name>
+
+    Raw - Kernel inside, raw image data
+    -----------------------------------
+
+        glance-upload --type=raw --kernel=nokernel --ramdisk=noramdisk \
+                         <filename> <name>
+
+
+    VHD - Kernel inside, data encoded with VHD format
+    -------------------------------------------------
+
+        glance-upload --type=vhd --kernel=nokernel --ramdisk=noramdisk \
+                         <filename> <name>
 """
 
 # FIXME(sirp): This can be merged into glance-admin when that becomes
@@ -70,14 +80,14 @@
 
 def main():
     args = parse_args()
-    meta = {'name': args.name, 'type': args.type, 'is_public': True}
-
-    if args.type == 'machine':
-        if args.kernel and args.ramdisk:
-            meta['properties'] = {'kernel_id': args.kernel,
-                                  'ramdisk_id': args.ramdisk}
-        else:
-            die("kernel and ramdisk required for machine image")
+    meta = {'name': args.name, 'type': args.type, 'is_public': True,
+            'properties': {}}
+
+    if args.kernel:
+        meta['properties']['kernel_id'] = args.kernel
+
+    if args.ramdisk:
+        meta['properties']['ramdisk_id'] = args.ramdisk
 
     client = Client(args.host, args.port)
     with open(args.filename) as f:

=== modified file 'tests/unit/test_misc.py'
--- tests/unit/test_misc.py	2011-02-23 20:35:01 +0000
+++ tests/unit/test_misc.py	2011-02-24 23:09:15 +0000
@@ -154,7 +154,8 @@
                             "in output: %s" % out)
 
             cmd = "./bin/glance-upload --port=%(api_port)d "\
-                  "--type=invalid %(conf_file_name)s 'my image'" % locals()
+                  "--type=invalid %(conf_file_name)s "\
+                  "'my image'" % locals()
 
             # Normally, we would simply self.assertRaises() here, but
             # we also want to check that the Invalid image type is in