Migration controller #118

admiyo · 2017-03-01T11:20:31Z

No description provided.

rmohr

Very nice! 👍

rmohr · 2017-03-06T12:58:21Z

pkg/virt-controller/services/vm.go

 	}
-	if _, err := v.KubeCli.Core().Pods(v1.NamespaceDefault).Create(pod); err != nil {
+
+	err2 := UpdateMigration(migration)


Lower case, as it is just a helper and should not be exported

But its called from Watch, too.

Oh right ...

rmohr · 2017-03-06T13:01:17Z

tests/vm_migration_test.go

+		})
+
+		AfterEach(func() {
+			restClient.Delete().Resource("migrations").Namespace(api.NamespaceDefault).Name(migration.ObjectMeta.Name).Do()


Could you add the deletion of all migrations to tests.MustCleanup()?

rmohr · 2017-03-06T13:03:39Z

tests/vm_migration_test.go

+			Expect(err).To(BeNil())
+			var r runtime.Object = nil
+
+			time.Sleep(time.Second * 3)


Use Eventually()

rmohr · 2017-03-06T13:03:51Z

tests/vm_migration_test.go

+			Expect(vmR.ObjectMeta.Name).To(Equal(vm.ObjectMeta.Name))
+
+			//wait for the  controller to update status
+			time.Sleep(time.Second * 3)


Use Eventually()

rmohr · 2017-03-06T13:04:02Z

tests/vm_migration_test.go

+			err = restClient.Post().Resource("migrations").Namespace(api.NamespaceDefault).Body(migration).Do().Error()
+			Expect(err).To(BeNil())
+			//wait for the  controller to update status
+			time.Sleep(time.Second * 3)


Use Eventually()

rmohr · 2017-03-06T13:16:11Z

tests/vm_migration_test.go

+
+	Context("New Migration given", func() {
+
+		It("Should be No migrations", func() {


This test here seems to be pretty redundant ... BeforeEach and AfterEach should make sure that the system is clean ...

rmohr · 2017-03-06T13:18:34Z

pkg/virt-controller/services/vm.go

+	return err
+}
+
+func GetDefinedVMs(vmName string) (*corev1.VM, error) {


Looks like this can just return one VM.

Maybe FetchVM, would be ok too? The Fetch better indicates, that it is not just looked up in the cache ...

rmohr · 2017-03-06T13:19:22Z

pkg/virt-controller/services/vm.go

+
+func GetDefinedVMs(vmName string) (*corev1.VM, error) {
+	restClient, err := kubecli.GetRESTClient()
+	list, err := restClient.Get().Namespace(v1.NamespaceDefault).Resource("vms").Name(vmName).Do().Get()


list should be vm

rmohr · 2017-03-06T13:20:00Z

pkg/virt-controller/services/vm.go

+		return nil, err
+	}
+	vm := list.(*corev1.VM)
+	return vm, err


Mabye return vm.(*corev1.VM), nil instead?

rmohr · 2017-03-06T13:23:54Z

pkg/virt-controller/watch/migration.go

+
+		// Retrieve the Migration
+		if !exists {
+			cleanupOldMigration(key, queue, migrationService)


I would keep the creation of the old Migration object in the main loop and just do the cleanup in the function.

In general the same things apply like to the refactoring PR. The functions seem very polluted with infra parameters ...

stu-gott

I'd prefer if the changeset titled "Whitespace cleanup to fix formate check" were squashed

stu-gott · 2017-03-06T13:22:19Z

pkg/api/v1/types.go

@@ -341,6 +363,9 @@ type MigrationSpec struct {
 type MigrationPhase string

 const (
+	// Create Migration has been called but nothing has been done with it
+	MigrationUnknown MigrationPhase = ""


Is there a compelling reason to make this an empty string? It would be more consistent to make this:

MigrationUnknown MigrationPhase = "Unknown"

It means, that so far, no component touched it, if there is no state present ...

stu-gott · 2017-03-06T13:26:05Z

tests/vm_migration_test.go

+		It("Should be No migrations", func() {
+
+			b, err := restClient.Get().Resource("migrations").Namespace(api.NamespaceDefault).Name(migration.ObjectMeta.Name).DoRaw()
+			Expect(err).ToNot(BeNil())


Expect(err).ToNot(HaveOccurred())

This appears a lot in this file so I'll just reference this once.

rmohr · 2017-03-07T14:39:03Z

pkg/virt-api/rest/kubeproxy_test.go

@@ -25,6 +25,7 @@ import (
 	"kubevirt.io/kubevirt/pkg/logging"
 	rest2 "kubevirt.io/kubevirt/pkg/rest"
 	. "kubevirt.io/kubevirt/pkg/virt-api/rest"
+	matchers "kubevirt.io/kubevirt/test"


Could you do a "." import: . "kubevirt.io/kubevirt/test?

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

tests/vm_migration_test.go

@@ -93,30 +91,26 @@ var _ = Describe("VmMigration", func() {

 			time.Sleep(time.Second * 3)


There is a time.Sleep left

rmohr · 2017-03-07T14:43:39Z

tests/vm_migration_test.go

-			r, err = restClient.Get().Resource("vms").Namespace(api.NamespaceDefault).Name(vm.ObjectMeta.Name).Do().Get()
-			var vmR *v1.VM = r.(*v1.VM)
-			Expect(vmR.ObjectMeta.Name).To(Equal(vm.ObjectMeta.Name))
+			Eventually(func() string {


You probably have to give your Eventually calls an overall timeout and a polling intervall different to 1 second and 10 ms. Or did it just work for you?

admiyo · 2017-03-07T16:37:17Z

All comments from the last review were addressed. I had to rebase to push. I want to make one last push to try and get Unit test coverage before I squash the commits

rmohr · 2017-03-08T14:38:48Z

retest this please

Starts the migration process, through the launching of the target pod. Increased unit test coverage for both migration and VMs

rmohr · 2017-03-09T09:34:24Z

retest this please

rmohr · 2017-03-09T09:44:04Z

pkg/virt-controller/services/vm_test.go

+					ghttp.RespondWithJSONEncoded(http.StatusOK, migration),
+				),
+
+				/*ghttp.CombineHandlers(


That can be removed

Migration controller

As first step to implement node<->node communication using secondary nics we need to have unique mac address on them. Signed-off-by: Quique Llorente <ellorent@redhat.com>

admiyo force-pushed the migration-controller branch 4 times, most recently from 35fcdc4 to b2e42ed Compare March 6, 2017 12:02

admiyo changed the title ~~WIP Migration controller~~ Migration controller Mar 6, 2017

rmohr requested changes Mar 6, 2017

View reviewed changes

stu-gott requested changes Mar 6, 2017

View reviewed changes

rmohr requested changes Mar 7, 2017

View reviewed changes

admiyo force-pushed the migration-controller branch from 9267697 to 71451d3 Compare March 7, 2017 16:36

rmohr force-pushed the migration-controller branch from 86d0445 to 7f22181 Compare March 8, 2017 10:39

admiyo force-pushed the migration-controller branch 3 times, most recently from 1a6aa0b to 70621c1 Compare March 8, 2017 19:50

stu-gott approved these changes Mar 8, 2017

View reviewed changes

admiyo added 2 commits March 9, 2017 10:22

refactor: extract matchers for tests

6bdc25f

Migration controller pod launch

6216ff9

Starts the migration process, through the launching of the target pod. Increased unit test coverage for both migration and VMs

rmohr force-pushed the migration-controller branch from 70621c1 to 6216ff9 Compare March 9, 2017 09:23

rmohr approved these changes Mar 9, 2017

View reviewed changes

admiyo merged commit 5f8af89 into kubevirt:master Mar 9, 2017

rmohr pushed a commit that referenced this pull request Mar 13, 2017

Merge pull request #118 from admiyo/migration-controller

493b5bc

Migration controller

admiyo deleted the migration-controller branch April 7, 2017 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration controller #118

Migration controller #118

admiyo commented Mar 1, 2017

rmohr left a comment

rmohr Mar 6, 2017

admiyo Mar 7, 2017

rmohr Mar 7, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 6, 2017

stu-gott left a comment

stu-gott Mar 6, 2017

rmohr Mar 6, 2017

stu-gott Mar 6, 2017

rmohr Mar 6, 2017

rmohr Mar 7, 2017

rmohr Mar 7, 2017

rmohr Mar 7, 2017

admiyo commented Mar 7, 2017

rmohr commented Mar 8, 2017

rmohr commented Mar 9, 2017

rmohr Mar 9, 2017


		Context("New Migration given", func() {

		It("Should be No migrations", func() {

		@@ -93,30 +91,26 @@ var _ = Describe("VmMigration", func() {

		time.Sleep(time.Second * 3)

Migration controller #118

Migration controller #118

Conversation

admiyo commented Mar 1, 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

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 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

admiyo commented Mar 7, 2017

rmohr commented Mar 8, 2017

rmohr commented Mar 9, 2017

Choose a reason for hiding this comment