New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spec.running in body is required when create VM from template #945

Open
gouyang opened this Issue Apr 24, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@gouyang
Contributor

gouyang commented Apr 24, 2018

# oc process -f vm-template-fedora.yaml -p NAME=Fedora27 | oc create -f -
The OfflineVirtualMachine "Fedora27" is invalid: []: Invalid value: map[string]interface {}{"apiVersion":"kubevirt.io/v1alpha1", "kind":"OfflineVirtualMachine", "metadata":map[string]interface {}{"creationTimestamp":"2018-04-24T06:51:24Z", "uid":"e74fe886-478b-11e8-bff4-44a842130b3e", "selfLink":"", "clusterName":"", "labels":map[string]interface {}{"kubevirt-ovm":"ovm-Fedora27"}, "name":"Fedora27", "namespace":"golden-images"}, "spec":map[string]interface {}{"template":map[string]interface {}{"spec":map[string]interface {}{"domain":map[string]interface {}{"resources":map[string]interface {}{"requests":map[string]interface {}{"memory":"4096Mi"}}, "cpu":map[string]interface {}{"cores":4}, "devices":map[string]interface {}{"disks":[]interface {}{map[string]interface {}{"volumeName":"registryvolume", "disk":map[string]interface {}{"bus":"virtio"}, "name":"disk0"}, map[string]interface {}{"disk":map[string]interface {}{"bus":"virtio"}, "name":"disk1", "volumeName":"cloudinitvolume"}}}}, "volumes":[]interface {}{map[string]interface {}{"name":"registryvolume", "registryDisk":map[string]interface {}{"image":"kubevirt/fedora-cloud-registry-disk-demo:devel"}}, map[string]interface {}{"name":"cloudinitvolume", "cloudInitNoCloud":map[string]interface {}{"userDataBase64":"I2Nsb3VkLWNvbmZpZwpwYXNzd29yZDogYXRvbWljCnNzaF9wd2F1dGg6IFRydWUKY2hwYXNzd2Q6IHsgZXhwaXJlOiBGYWxzZSB9Cg=="}}}}, "metadata":map[string]interface {}{"labels":map[string]interface {}{"kubevirt-ovm":"ovm-Fedora27"}}}}}: validation failure list:
spec.running in body is required

If I add to running: false to the spec section, it works then.

# git diff
diff --git a/cluster/vm-template-fedora.yaml b/cluster/vm-template-fedora.yaml
index 617089c..e53dd1f 100644
--- a/cluster/vm-template-fedora.yaml
+++ b/cluster/vm-template-fedora.yaml
@@ -17,6 +17,7 @@ objects:
     labels:
       kubevirt-ovm: ovm-${NAME}
   spec:
+    running: false
     template:
       metadata:
         labels:
@davidvossel

This comment has been minimized.

Show comment
Hide comment
@davidvossel

davidvossel May 7, 2018

Member

Running has always been marked as a required field in our API. The only difference now is required fields are being enforced at creation time.

All our example ovms reflect this requirement.

Member

davidvossel commented May 7, 2018

Running has always been marked as a required field in our API. The only difference now is required fields are being enforced at creation time.

All our example ovms reflect this requirement.

@petrkotas

This comment has been minimized.

Show comment
Hide comment
@petrkotas

petrkotas May 11, 2018

Member

The running is required field and have to be provided. If you do not add running:true|false than the ovm does not have any information how to proceed. We can discuss whether the ovm should do some defaulting to set default value to be running: false. @rmohr @fabiand what do you think?

Member

petrkotas commented May 11, 2018

The running is required field and have to be provided. If you do not add running:true|false than the ovm does not have any information how to proceed. We can discuss whether the ovm should do some defaulting to set default value to be running: false. @rmohr @fabiand what do you think?

@fabiand

This comment has been minimized.

Show comment
Hide comment
@fabiand

fabiand May 15, 2018

Member

I'd say we can sanely default to running: false - it would just be a default like with any other field.

Member

fabiand commented May 15, 2018

I'd say we can sanely default to running: false - it would just be a default like with any other field.

@davidvossel

This comment has been minimized.

Show comment
Hide comment
@davidvossel

davidvossel May 15, 2018

Member

I'd say we can sanely default to running: false - it would just be a default like with any other field.

Defaulting running to "false" makes sense when the object is called a OfflineVirtualMachine. What about when we change the name to something like a StatefulVirtualMachine? I'd be confused if I posted a SVM and it didn't run.

The name OVM will change soon. I'd suggest leaving running as a required field until then. After we settle on a name, we can decide what the default should be for running.

Member

davidvossel commented May 15, 2018

I'd say we can sanely default to running: false - it would just be a default like with any other field.

Defaulting running to "false" makes sense when the object is called a OfflineVirtualMachine. What about when we change the name to something like a StatefulVirtualMachine? I'd be confused if I posted a SVM and it didn't run.

The name OVM will change soon. I'd suggest leaving running as a required field until then. After we settle on a name, we can decide what the default should be for running.

@petrkotas

This comment has been minimized.

Show comment
Hide comment
@petrkotas

petrkotas May 16, 2018

Member

I tend to agree with David. running: false for statefull VM is confusing. Having it required means, user have to decide what he wants to do with it.

Member

petrkotas commented May 16, 2018

I tend to agree with David. running: false for statefull VM is confusing. Having it required means, user have to decide what he wants to do with it.

@itamarh

This comment has been minimized.

Show comment
Hide comment
@itamarh

itamarh May 16, 2018

Member

I agree the different names may be confusing. I don't agree with putting a burden of the user to have to decide as a simple way out for us (as a general rule of thinking, sometimes we have to)

Member

itamarh commented May 16, 2018

I agree the different names may be confusing. I don't agree with putting a burden of the user to have to decide as a simple way out for us (as a general rule of thinking, sometimes we have to)

@davidvossel

This comment has been minimized.

Show comment
Hide comment
@davidvossel

davidvossel May 16, 2018

Member

I agree the different names may be confusing. I don't agree with putting a burden of the user to have to decide as a simple way out for us (as a general rule of thinking, sometimes we have to)

yep, I agree. We need a default. I suggest we hold off on making a decision about the default until after we are 100% committed to a name (ovm vs svm discussion) because the name impacts the default we use.

Member

davidvossel commented May 16, 2018

I agree the different names may be confusing. I don't agree with putting a burden of the user to have to decide as a simple way out for us (as a general rule of thinking, sometimes we have to)

yep, I agree. We need a default. I suggest we hold off on making a decision about the default until after we are 100% committed to a name (ovm vs svm discussion) because the name impacts the default we use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment