Move SPICE part from kubectl.sh to own spice.go package #201

bond95 · 2017-04-28T15:12:38Z

#181
@rmohr Please review

kubevirt-bot · 2017-04-28T15:12:40Z

Can one of the admins verify this patch?

rmohr

Thanks a lot for you contribution. 🍰

I added a few notes on how to better leverege the kubernetes SDK.

Also don't forget to call this code when cluster/kubectl.sh console is invoked.

rmohr · 2017-04-28T16:11:47Z

pkg/virtctl/spice/spice.go

+	if details {
+		fmt.Printf("%s", body)
+	} else {
+		f, err := os.Create(SPICE_FILENAME)


Maybe we can improve that a little bit and write it into a temp file which we delete after we are done:

file, err := ioutil.TempFile(os.TempDir(), "console.vv") defer os.Remove(file.Name())

rmohr · 2017-04-28T16:16:12Z

pkg/virtctl/spice/spice.go

+
+func DownloadSpice(vm, host string) (string, error) {
+	var path string = fmt.Sprintf(VM_SPICE_URL, host, vm)
+	req, err := http.NewRequest("GET", path, nil)


We can use the rest client from kubernetes here:

body, err := restClient.Get(). Resource("vms").SetHeader("Accept", rest.MIME_INI). SubResource("spice"). Namespace(kubev1.NamespaceDefault). Name(vm.GetObjectMeta().GetName()).Do().Raw()

You can get a rest client which understands the VM TPR like this:

restClient, err := kubecli.GetRESTClientFromFlags(master, kubeconfig)

This also allows us to reuse the authentication information from kubeconfig.

rmohr · 2017-04-28T16:17:31Z

pkg/virtctl/spice/spice.go

+}
+
+func (o *Spice) Usage() string {
+	usage := "The following options can be passed to any command:\n\n"


Here some usage examples like in virtctl/console/console.go would be nice here.

rmohr · 2017-04-28T16:18:21Z

pkg/virtctl/spice/spice.go

+	}
+	vm := flags.Arg(1)
+
+	config, err := clientcmd.BuildConfigFromFlags(server, kubeconfig)


Here you can dreate the rest client instead and directly use it.

bond95 · 2017-05-03T13:01:32Z

@rmohr fixed comments, please review.

rmohr

Almost there

rmohr · 2017-05-03T13:40:58Z

pkg/virtctl/spice/spice.go

+			log.Fatalf("Can't open file: %s", err.Error())
+			return 1
+		}
+		defer f.Close()


here we would need a defer os.Remove(f.Name()) too, otherwise the file stays if we return early

rmohr · 2017-05-03T13:44:46Z

pkg/virtctl/spice/spice.go

+		f.Sync()
+
+		cmnd := exec.Command("remote-viewer", f.Name())
+		cmnd.Run()


This line returns an error, we have to check for it, to return the correct exit code

rmohr · 2017-05-03T14:35:12Z

pkg/virtctl/spice/spice.go

+
+const FLAG = "spice"
+const SPICE_FILENAME = "console.vv"
+const VM_SPICE_URL = "http://%s/apis/kubevirt.io/v1alpha1/namespaces/default/vms/%s/spice"


That one can be removed

rmohr · 2017-05-03T14:46:42Z

pkg/virtctl/spice/spice.go

+}
+
+func (o *Spice) Usage() string {
+	usage := "Usage: ./virtctl spice vm_name [--details] [common_options]\n\n"


Could you give real examples of the usage here? For instance kubectl exec --help gives:

Execute a command in a container. Examples: # Get output from running 'date' from pod 123456-7890, using the first container by default kubectl exec 123456-7890 date # Get output from running 'date' in ruby-container from pod 123456-7890 kubectl exec 123456-7890 -c ruby-container date # Switch to raw terminal mode, sends stdin to 'bash' in ruby-container from pod 123456-7890 # and sends stdout/stderr from 'bash' back to the client kubectl exec 123456-7890 -c ruby-container -i -t -- bash -il Options: -c, --container='': Container name. If omitted, the first container in the pod will be chosen -p, --pod='': Pod name -i, --stdin=false: Pass stdin to the container -t, --tty=false: Stdin is a TTY Usage: kubectl exec POD [-c CONTAINER] -- COMMAND [args...] [options]

Maybe

Fetch SPICE connections details of a VM and connect via remote-viewer Examples: #Show connection details of the VM myvm virtctl spice myvm --details # Open a SPICE connection to the VM myvm via remote-viewer virtctl spice myvm Options:

The rest will be added by o.FlagSet.FlagUsages()

rmohr · 2017-05-03T14:47:19Z

pkg/virtctl/spice/spice.go

+}
+
+func (o *Spice) Usage() string {
+	usage := "Usage: ./virtctl spice vm_name [--details] [common_options]\n\n"


o.FlagSet.FlagUsages() adds options and the usage for you

bond95 · 2017-05-10T14:32:01Z

Fixed @rmohr please review

rmohr · 2017-05-10T15:23:49Z

pkg/virtctl/spice/spice.go

+
+		cmnd := exec.Command("remote-viewer", f.Name())
+		err = cmnd.Run()
+		defer os.Remove(f.Name())


This defer is too late, it needs to be directly after defer f.Close(), the rest looks good to me :)

bond95 · 2017-05-11T13:39:09Z

@rmohr fixed =)

rmohr · 2017-05-11T13:47:41Z

Ups, I suggested the wrong defer order. It is a stack, so we need

defer os.Remove(f.Name())
defer f.Close()

Also see https://tour.golang.org/flowcontrol/13 for a nice example.

bond95 · 2017-05-11T13:57:57Z

Fixed @rmohr

rmohr · 2017-05-12T07:21:41Z

Thx! 👍

exit 1 on user abort

MAINTAINERS: add Tom Denham and Gabe Rosenhouse

rmohr requested changes Apr 28, 2017

View reviewed changes

bond95 force-pushed the move-spice-go branch 2 times, most recently from 5a76ec9 to 2b7b08d Compare May 3, 2017 12:59

rmohr requested changes May 3, 2017

View reviewed changes

bond95 force-pushed the move-spice-go branch 2 times, most recently from 4e8c43c to 0c01246 Compare May 10, 2017 14:31

rmohr reviewed May 10, 2017

View reviewed changes

bond95 force-pushed the move-spice-go branch from 0c01246 to c281a80 Compare May 11, 2017 13:38

Move SPICE part from kubectl.sh to own spice.go package

50a44d5

bond95 force-pushed the move-spice-go branch from c281a80 to 50a44d5 Compare May 11, 2017 13:56

rmohr merged commit 9e27045 into kubevirt:master May 12, 2017

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

Merge pull request kubevirt#201 from copejon/abort-versioning

570d834

exit 1 on user abort

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

Merge pull request kubevirt#201 from jonboulle/master

e1907bd

MAINTAINERS: add Tom Denham and Gabe Rosenhouse

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move SPICE part from kubectl.sh to own spice.go package #201

Move SPICE part from kubectl.sh to own spice.go package #201

bond95 commented Apr 28, 2017

kubevirt-bot commented Apr 28, 2017

rmohr left a comment

rmohr Apr 28, 2017

rmohr Apr 28, 2017

rmohr Apr 28, 2017

rmohr Apr 28, 2017

bond95 commented May 3, 2017

rmohr left a comment

rmohr May 3, 2017

rmohr May 3, 2017

rmohr May 3, 2017

rmohr May 3, 2017

rmohr May 3, 2017

bond95 commented May 10, 2017

rmohr May 10, 2017

bond95 commented May 11, 2017

rmohr commented May 11, 2017

bond95 commented May 11, 2017

rmohr commented May 12, 2017

Move SPICE part from kubectl.sh to own spice.go package #201

Move SPICE part from kubectl.sh to own spice.go package #201

Conversation

bond95 commented Apr 28, 2017

kubevirt-bot commented Apr 28, 2017

rmohr 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

bond95 commented May 3, 2017

rmohr 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

bond95 commented May 10, 2017

Choose a reason for hiding this comment

bond95 commented May 11, 2017

rmohr commented May 11, 2017

bond95 commented May 11, 2017

rmohr commented May 12, 2017