Convert spec command for virtctl #189

rmohr · 2017-04-07T14:29:54Z

~~Three things missing:~~

~~The libvirt struct still depends on libvirt-go stuff~~
~~Tests~~
~~Documentation~~

Example usage:

Convert between Kubevirt and Libvirt VM representations:

Examples:
# Convert Domain xml to yaml via stdin
virsh dumpxml testvm | virtctl convert-spec -f - -o yaml

# Convert yaml into json from a http resource
virtctl convert-spec -f http://127.0.0.1:4012/vm.yaml -o json

# Convert VM specification from a yaml file into a Libvirt Domain XML
virtctl convert-spec -f vm.yaml -o xml > dom.xml

Options:
  -f, --filename string         Filename, directory, or URL to files identifying the resource to get from a server.
  -o, --output string           Output format. One of: json|yaml|xml. (default "json")
      --output-version string   Output the formatted object with the given group version (default "kubevirt.io/v1alpha1")

The input format is automatically detected.

rmohr · 2017-04-07T14:34:10Z

/cc @fabiand @stu-gott @admiyo See above that it is not yet completely done, but the functionality is there and works.

Could you provide some feedback if the commandline arguments and the examples make sense in you eyes?

stu-gott · 2017-04-07T14:39:11Z

pkg/virtctl/convert/guess.go

+	return buffer, b, hasXMLPrefix(b)
+}
+
+var xmlPrefix = []byte("<")


Is this truly sufficient?

I think it is good enough. Everything more than that would involve parsing the whole xml (basically a validation then, which will be done by the decoder later on)

stu-gott · 2017-04-07T14:41:32Z

pkg/virtctl/convert/guess.go

+	} else if r, _, isJSON := yaml.GuessJSONStream(r, size); isJSON {
+		return r, JSON
+	} else {
+		return r, YAML


Is it possible to explicitly test for yaml rather than assuming at this point?

Not really. For JSON, YAML and XML, we can just guess in every case. For JSON "{" (definitely not xml or yaml", for XML "<" (definitely not yaml or json).

If both don't match, I take the only possible left solution which will then later on fail when it is parsed.

stu-gott · 2017-04-07T14:42:03Z

pkg/virtctl/convert/guess.go

+	"unicode"
+)
+
+func GuessStreamType(source io.Reader, size int) (io.Reader, Type) {


This function signature should probably also include an error code. For example there might be parse errors, or the stream doesn't match anything we know.

My assumption is, that the conversion later on will fail, if we have an unsupported format. I will think about it a little bit more.

stu-gott · 2017-04-07T14:48:33Z

pkg/virtctl/convert/convert.go

+		return resp.Body, nil
+
+	} else {
+		return os.Open(sourceName)


Open returns (*File, error), is *File also a io.ReadCloser?

fabiand · 2017-04-07T15:01:07Z

/cc @michalskrivanek @mpolednik

To make sure, we don't depend on anything from libvirt-go we have to move the xml struct into a separate package. If we depend on libvirt-go, virtctl would required libvirt-client to be installed.

michalskrivanek · 2017-04-10T08:44:38Z

@rmohr why don't you want to depend on libvirt-go?

rmohr · 2017-04-10T09:21:52Z

@rmohr why don't you want to depend on libvirt-go?

@michalskrivanek In the backend we do. virtctl is just a frontend commandline tool. If we would depend anywhere there directly on libvirt-go code, golang would link against the libvirt C library. We don't need anything from libvirt-go in this frontend.

michalskrivanek · 2017-04-10T09:36:43Z

ah, sure, thanks for clarification

rmohr · 2017-04-10T12:21:55Z

Ready for review

stu-gott

Minor nits only. Looks good!

stu-gott · 2017-04-10T13:57:50Z

.travis.yml

-  - 1.6.3
-  - 1.7.x
+  - 1.7.5
+  - 1.8.x  


Trailing whitespace

stu-gott · 2017-04-10T14:00:16Z

cmd/virtctl/virtctl.go


 Basic Commands:
  console        Connect to a serial console on a VM
+  convert-spec   Convert between Libvirt and Kubevirt specifications


KubeVirt, right?

stu-gott · 2017-04-10T14:03:27Z

pkg/virtctl/convert/convert.go

+}
+
+func (c *Convert) Usage() string {
+	usage := "Convert between Kubevirt and Libvirt VM representations:\n\n"


stu-gott · 2017-04-10T14:04:57Z

pkg/virtctl/convert/convert.go

+
+	if sourceName == "" {
+		log.Println("No source specified")
+		return 1


It might be more clear if we create enums at the top of the module along the lines of "RuntimeSuccess" and "RuntimeError"

Let's return an error or nil instead, how about that?

I would do that in a separate PR, since it is an interface change for all virtcl commands ...

Anyway, your suggestion is good. Done.

stu-gott · 2017-04-10T14:07:03Z

pkg/virtctl/convert/guess_test.go

+)
+
+var _ = Describe("Guess", func() {
+	table.DescribeTable("Should detect stram type", func(data string, detected Type) {


"stream type"

stu-gott · 2017-04-21T13:13:44Z

retest this please

DataVolume CRD

Remove reference to policy on accepting PRs

We need special treatment to make OKD clusters working with the `pack8s` helper. Differently from docker, we can't just call podman, because we can be running with an user != root. In that case, the copy step will fail. Hence, we use a special new command in pack8s to do the copy on our behalf. Signed-off-by: Karel Šimon <ksimon@redhat.com>

stu-gott requested changes Apr 7, 2017

View reviewed changes

rmohr mentioned this pull request Apr 10, 2017

Windows 10 does not run out-of-the-box #170

Closed

6 tasks

Move Libvirt XML struct in separate subfolder

734c37a

To make sure, we don't depend on anything from libvirt-go we have to move the xml struct into a separate package. If we depend on libvirt-go, virtctl would required libvirt-client to be installed.

rmohr force-pushed the converter branch from afbcebd to 02ed980 Compare April 10, 2017 12:13

rmohr requested review from admiyo, stu-gott and fabiand April 10, 2017 12:16

rmohr changed the title ~~[WIP] Converter [skip ci]~~ Convert spec command for virtctl Apr 10, 2017

rmohr force-pushed the converter branch from a7b8b59 to 4270f42 Compare April 10, 2017 13:05

stu-gott reviewed Apr 10, 2017

View reviewed changes

rmohr force-pushed the converter branch from 4270f42 to 70cbbd4 Compare April 21, 2017 11:10

rmohr added 2 commits April 21, 2017 13:17

Add converter between xml, json and yaml to virtctl

ad82d73

Drop GO 1.6 support

7112e0b

rmohr force-pushed the converter branch from 70cbbd4 to 7112e0b Compare April 21, 2017 11:17

stu-gott approved these changes Apr 21, 2017

View reviewed changes

rmohr merged commit 13a495b into kubevirt:master Apr 21, 2017

kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Nov 6, 2020

Merge pull request kubevirt#189 from davidvossel/cdi-crds-v1

202e966

DataVolume CRD

mzzgaopeng pushed a commit to mzzgaopeng/kubevirt that referenced this pull request Mar 8, 2021

Merge pull request kubevirt#189 from tomdee/patch-2

cae6604

Remove reference to policy on accepting PRs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert spec command for virtctl #189

Convert spec command for virtctl #189

rmohr commented Apr 7, 2017 •

edited

rmohr commented Apr 7, 2017

stu-gott Apr 7, 2017

rmohr Apr 7, 2017

stu-gott Apr 7, 2017

rmohr Apr 7, 2017

stu-gott Apr 7, 2017

rmohr Apr 7, 2017 •

edited

stu-gott Apr 7, 2017

rmohr Apr 7, 2017

fabiand commented Apr 7, 2017 •

edited

michalskrivanek commented Apr 10, 2017

rmohr commented Apr 10, 2017 •

edited

michalskrivanek commented Apr 10, 2017

rmohr commented Apr 10, 2017

stu-gott left a comment

stu-gott Apr 10, 2017

stu-gott Apr 10, 2017

stu-gott Apr 10, 2017

stu-gott Apr 10, 2017

rmohr Apr 21, 2017

rmohr Apr 21, 2017

rmohr Apr 21, 2017

stu-gott Apr 10, 2017

stu-gott commented Apr 21, 2017

Convert spec command for virtctl #189

Convert spec command for virtctl #189

Conversation

rmohr commented Apr 7, 2017 • edited

rmohr commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr Apr 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiand commented Apr 7, 2017 • edited

michalskrivanek commented Apr 10, 2017

rmohr commented Apr 10, 2017 • edited

michalskrivanek commented Apr 10, 2017

rmohr commented Apr 10, 2017

stu-gott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-gott commented Apr 21, 2017

rmohr commented Apr 7, 2017 •

edited

rmohr Apr 7, 2017 •

edited

fabiand commented Apr 7, 2017 •

edited

rmohr commented Apr 10, 2017 •

edited