Non-default namespaces #249

vladikr · 2017-06-12T15:46:46Z

Fixes #239

kubevirt-bot · 2017-06-12T15:46:48Z

Can one of the admins verify this patch?

davidvossel · 2017-06-12T16:38:05Z

pkg/virt-controller/services/vm.go

 	return err
 }

 func (v *vmService) GetMigrationJob(migration *corev1.Migration) (*v1.Pod, bool, error) {
 	selector := migrationJobSelector(migration)
-	podList, err := v.KubeCli.CoreV1().Pods(v1.NamespaceDefault).List(selector)
+	podList, err := v.KubeCli.CoreV1().Pods(migration.ObjectMeta.Namespace).List(selector)


I'm curious about the mixed usage of the GetObjectMeta() accessor function and using the vm.ObjectMeta directly.

Is there a difference between migration.ObjectMeta.Namespace and migration.GetObjectMeta().getNamespace()?

Yes. I'm not sure about that either. In this first patch, I didn't see the use of GetObjectMeta(), so I just used ObjectMeta. Later, I used GetObjectMeta() if there was no previous use of ObjectMeta.. but honestly I'm not sure. If it does the same thing, I'll just change it to either one of these.

obj.ObjectMeta accesses the metadata through the public field, whereas obj.GetObjectMeta() accesses it through a methode defined in the runtime.Object interface. We were not really strict on how we want to access it, but I think that accessing it through the method, which is defined to implement the interface is nicer and I normally try to do it that way. On the other hand, accessing it through the public field is easier to read ...

davidvossel · 2017-06-12T16:39:43Z

pkg/virt-controller/services/vm.go

@@ -93,14 +93,14 @@ func (v *vmService) DeleteVMPod(vm *corev1.VM) error {
 	precond.MustNotBeNil(vm)
 	precond.MustNotBeEmpty(vm.GetObjectMeta().GetName())


may add a precondition for namespace here now that it is a required argument. However, I can't think of a scenario where name would exist but namespace would not.

ok, cool, I can add that.

We were not really strict with the procond thing. It is meant to be a helper, to detect contract mismatches which should not be possible to happen, and to help you when writing tests, so that you have to pass in more realistic data ...

Would be nice if we would continue using it, or use it even more. We can discuss if we want to keep it.

davidvossel · 2017-06-12T16:44:13Z

I believe the regression tests are failing because "pkg/virt-controller/watch/vm.go" was modified without including the resulting 'go generate' output.

vladikr · 2017-06-12T17:13:01Z

I'll go look what 'go generate' means... :)

rmohr

The direction looks good.

In the virt-handler code it gets a little bit more interesting. At the moment, we don't store on the domain, to which namespace the VM belongs. If we don't do that, it would only be allowed to have one VM with a specific name in the whole cluster, instead of allowing a VM name per namespace.

The easiest option would obviously be to name the VMs like this: namespace/name, but / is not allowed in domain names. Since libvirt allows us to use '>< but kubernetes does not, I suggest that we pick one of these, to encode the namespace into the VM name. E.g. default>testvm.

rmohr · 2017-06-13T08:16:06Z

pkg/virt-controller/services/vm.go

@@ -48,8 +48,8 @@ type VMService interface {
 	GetRunningMigrationPods(*corev1.Migration) (*v1.PodList, error)
 	CreateMigrationTargetPod(migration *corev1.Migration, vm *corev1.VM) error
 	UpdateMigration(migration *corev1.Migration) error
-	FetchVM(vmName string) (*corev1.VM, bool, error)
-	FetchMigration(migrationName string) (*corev1.Migration, bool, error)
+	FetchVM(vmName string, namespace string) (*corev1.VM, bool, error)


Instead of using FetchVM, you can start using the KubevirtClient from kubecli.go. It is modelled according to the kubernetes core client API.

Going to recommend that we hold off on that until a follow on patch. I'd rather see just the straight conversion from default namespace, with the rest of the code the same land first.

fine for me

rmohr · 2017-06-13T08:16:56Z

pkg/virt-controller/services/vm.go

-	FetchVM(vmName string) (*corev1.VM, bool, error)
-	FetchMigration(migrationName string) (*corev1.Migration, bool, error)
+	FetchVM(vmName string, namespace string) (*corev1.VM, bool, error)
+	FetchMigration(migrationName string, namespace string) (*corev1.Migration, bool, error)


Here we don't have a KubevirtClient for yet, so fine for me if you keep it for now. WIll provide a PR which will give us such a client for Migrations too.

rmohr · 2017-06-13T08:19:32Z

pkg/virt-controller/services/vm.go

@@ -93,14 +93,14 @@ func (v *vmService) DeleteVMPod(vm *corev1.VM) error {
 	precond.MustNotBeNil(vm)
 	precond.MustNotBeEmpty(vm.GetObjectMeta().GetName())


We were not really strict with the procond thing. It is meant to be a helper, to detect contract mismatches which should not be possible to happen, and to help you when writing tests, so that you have to pass in more realistic data ...

Would be nice if we would continue using it, or use it even more. We can discuss if we want to keep it.

rmohr · 2017-06-13T08:21:37Z

pkg/virt-controller/services/vm.go

 	return err
 }

 func (v *vmService) GetMigrationJob(migration *corev1.Migration) (*v1.Pod, bool, error) {
 	selector := migrationJobSelector(migration)
-	podList, err := v.KubeCli.CoreV1().Pods(v1.NamespaceDefault).List(selector)
+	podList, err := v.KubeCli.CoreV1().Pods(migration.ObjectMeta.Namespace).List(selector)


obj.ObjectMeta accesses the metadata through the public field, whereas obj.GetObjectMeta() accesses it through a methode defined in the runtime.Object interface. We were not really strict on how we want to access it, but I think that accessing it through the method, which is defined to implement the interface is nicer and I normally try to do it that way. On the other hand, accessing it through the public field is easier to read ...

admiyo · 2017-06-13T01:34:28Z

pkg/virt-controller/services/generated_mock_vm.go

@@ -103,28 +103,28 @@ func (_mr *_MockVMServiceRecorder) UpdateMigration(arg0 interface{}) *gomock.Cal
 	return _mr.mock.ctrl.RecordCall(_mr.mock, "UpdateMigration", arg0)
 }

-func (_m *MockVMService) FetchVM(vmName string) (*v10.VM, bool, error) {
-	ret := _m.ctrl.Call(_m, "FetchVM", vmName)
+func (_m *MockVMService) FetchVM(vmName string, namespace string) (*v10.VM, bool, error) {


please reverse the order of these parameters, as it makes sense for the namespace to be first. Neither should be allowed to be null or empty.

You missed this change request.

admiyo · 2017-06-15T17:15:04Z

pkg/virt-controller/services/vm.go

@@ -48,8 +48,8 @@ type VMService interface {
 	GetRunningMigrationPods(*corev1.Migration) (*v1.PodList, error)
 	CreateMigrationTargetPod(migration *corev1.Migration, vm *corev1.VM) error
 	UpdateMigration(migration *corev1.Migration) error
-	FetchVM(vmName string) (*corev1.VM, bool, error)
-	FetchMigration(migrationName string) (*corev1.Migration, bool, error)
+	FetchVM(vmName string, namespace string) (*corev1.VM, bool, error)


Going to recommend that we hold off on that until a follow on patch. I'd rather see just the straight conversion from default namespace, with the rest of the code the same land first.

vladikr · 2017-06-23T15:36:22Z

Updated with the latest changes. Hope all comments have been addressed.

rmohr

You will also have to change pkg/virt-handler/virtwrap/cache/cache.go#NewDomain. It will otherwise construct domain names which can cause us cache lookup problems on the virt-handler controller layer.

Could you also add a functional test for VM start/delete and a migration in other namespaces then the default one?

rmohr · 2017-06-26T08:24:08Z

pkg/virt-controller/services/generated_mock_vm.go

@@ -103,28 +103,28 @@ func (_mr *_MockVMServiceRecorder) UpdateMigration(arg0 interface{}) *gomock.Cal
 	return _mr.mock.ctrl.RecordCall(_mr.mock, "UpdateMigration", arg0)
 }

-func (_m *MockVMService) FetchVM(vmName string) (*v10.VM, bool, error) {
-	ret := _m.ctrl.Call(_m, "FetchVM", vmName)
+func (_m *MockVMService) FetchVM(vmName string, namespace string) (*v10.VM, bool, error) {


You missed this change request.

rmohr · 2017-06-26T08:28:07Z

pkg/virt-handler/virtwrap/manager.go

 	if len(mappingErrs) > 0 {
 		return errors.NewAggregate(mappingErrs)
 	}
-	dom, err := l.virConn.LookupDomainByName(vm.GetObjectMeta().GetName())
+	// Construct the domain name with a namespace prefix. E.g. namespace>name
+	domName := fmt.Sprintf("%s>%s", vm.GetObjectMeta().GetNamespace(), vm.GetObjectMeta().GetName())


Could we extract this into an extra functions?

Something like a VMNamespaceKeyFunc(obj VM) (string, error) and a counter part SplitVMNamespaceKey(string) (namespace, name, err)? I think it is important enough to get it's own functions.

rmohr · 2017-06-27T08:08:39Z

retest this please

kubevirt-bot · 2017-06-28T10:16:10Z

Can one of the admins verify this patch?

rmohr · 2017-06-29T12:30:34Z

tests/vm_migration_test.go

@@ -168,6 +168,59 @@ var _ = Describe("VmMigration", func() {
 			}, TIMEOUT, POLLING_INTERVAL).Should(Equal(1))
 			close(done)
 		}, 60)
+		It("Should migrate the VM with a non-default namespace", func(done Done) {


We could make the other migration test parametrized and give it different namespaces. https://onsi.github.io/ginkgo/#table-driven-tests

Great, thanks. I was struggling a bit to understand how does it work.

rmohr · 2017-06-29T12:32:05Z

tests/vmlifecycle_test.go

@@ -223,6 +223,55 @@ var _ = Describe("Vmlifecycle", func() {
 				close(done)
 			}, 50)
 		})
+		Context("New VM in a non-deafult namespace", func() {
+			It("Should log libvirt start and stop lifecycle events of the domain", func(done Done) {


I think we can do the same thing here by using a table.

rmohr

On a quick code scan, I found the following files which still contain hardcoded namespace references:

pkg/virt-api/rest/spice.go: would only look up VMs in the default namespace
cmd/virt-handler/virt-handler.go: only logs events to the default namespace
pkg/api/v1/types.go: The spice factory method contains a hardcoded namespace
pkg/informers/virtinformers.go: This contains NamespaceDefault references

@vladikr could you have a look?

rmohr · 2017-07-12T16:10:03Z

@vladikr updated a few things. We are almsot there. Our iscsi dns entries don't work, when our VMs are in a differnt namespace than the iscsi service. We just need to change their names, to include the namespace. After that is done, we can look into the functional tests.

rmohr · 2017-07-13T13:30:20Z

Down to two failing functional tests. I think we have to especially clean up tests/utils.go. I made sure, we now run all tests in a non-default namespace, which included a lot of boiler-plate code for tearing up and down namespaces, PVCs and PVs. I think a more strucutred approach for utils.go (also splitting probably) makes sense.

Looking forward to your feedback! :)

rmohr · 2017-07-13T13:47:40Z

Finally, all functional tests are passing.

fabiand

Independnt of bugs, the omve lgtm.

davidvossel

overall this looks great.

I see room for improving our functional tests now, but there's no reason to block this patch for that. I'm for shipping it as is :D

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Separate the namespace from the libvirt domain name, when constructing the vm object. Adjusting the VM and Migration structures to consider and non-default namespace. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

virt-handler cmd would construct vms with a domain namespace. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

So far the virt-controller was watching for object changes only in the default name space. Switching it to watch for cahnges in all namespaces. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

FetchVM and FetchMigration were only looking for objects in the default namespace. Switching it to use a namespace argument. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Changing virt-controller methods to use the namespace of a relevant object, instead of the deafult one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Changing virt-handler methods to use the namespace of a relevant object, instead of the deafult one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

libvirt domain name will be constructed with a vm namespace prefix and '>' delimiter, E.g. namespace>name This will allow starting similarly named domains in different vm namespaces. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

spice will use vm namespace instead of the default one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Adding functional tests for VM creation in a non-deafult namespace, as well as a migration test. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

* Change console code, to support namespaces * Change qemu-kube script to detect docker containers, by also taking namespaces into account * Switch from '>' to '_' as namespace_name delimiter Signed-off-by: Roman Mohr <rmohr@redhat.com>

Don't log expected stack traces to the test output. Signed-off-by: Roman Mohr <rmohr@redhat.com>

When a VM is not started in the same namespace, as the iscsi demo target service, DNS lookups need to contain the namespace, to get the IP of the service. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Let "virtctl spice" subcommand wrapper in cluseter/kubectl.sh properly forward all passed in arguments to the virtctl subcommand. Signed-off-by: Roman Mohr <rmohr@redhat.com>

In order to find the Domain in libvirt, which should be migrated, the migrator needs the namespace too, to deduce the correct Domain name. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Signed-off-by: Roman Mohr <rmohr@redhat.com>

PVs can only be referenced by one PVC at the same time, and they need to be manually recycled, before they can be bound to another PVC. Therefore recreate PVs and PVCs, exclusive for tests, on every test suite start. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Signed-off-by: Roman Mohr <rmohr@redhat.com>

rmohr · 2017-07-14T08:37:21Z

Rebased and retested. Merging based on the given approvals.

* Pull in expectations code from kubevirt Signed-off-by: David Vossel <davidvossel@gmail.com> * add expectations to datavolume controller Signed-off-by: David Vossel <davidvossel@gmail.com> * Refactor importer pod to use single queue and expectations Signed-off-by: David Vossel <davidvossel@gmail.com> * update utils tests to use single queue Signed-off-by: David Vossel <davidvossel@gmail.com> * re-introduce import controller test suite Signed-off-by: David Vossel <davidvossel@gmail.com> * make clone controllers pass after utils refactor Signed-off-by: David Vossel <davidvossel@gmail.com> * Use log level constants in cdi controllers Signed-off-by: David Vossel <davidvossel@gmail.com> * Remove useless shadow variables in expectations pkg Signed-off-by: David Vossel <davidvossel@gmail.com> * required changes to get owner references working with openshift 1.10 Signed-off-by: David Vossel <davidvossel@gmail.com> * update functional tests to work with importer controller refactor Signed-off-by: David Vossel <davidvossel@gmail.com> * Rename AnnImportPVC to LabelImportPvc in order to reflect its use Signed-off-by: David Vossel <davidvossel@gmail.com> * Add comment about expectations code Signed-off-by: David Vossel <davidvossel@gmail.com>

Signed-off-by: Quique Llorente <ellorent@redhat.com>

davidvossel reviewed Jun 12, 2017

View reviewed changes

rmohr requested changes Jun 13, 2017

View reviewed changes

admiyo reviewed Jun 15, 2017

View reviewed changes

vladikr force-pushed the non_default_namespaces branch from 124cda3 to f1c120c Compare June 23, 2017 14:43

rmohr requested changes Jun 26, 2017

View reviewed changes

vladikr force-pushed the non_default_namespaces branch 2 times, most recently from f5e8e04 to 501dcba Compare June 29, 2017 11:43

rmohr requested changes Jun 29, 2017

View reviewed changes

vladikr force-pushed the non_default_namespaces branch from 501dcba to a096588 Compare July 6, 2017 13:42

This was referenced Jul 7, 2017

Provide reference manifests for deployment on "any" Kubernetes cluster (i.e. minikube, OpenShift) #265

Closed

Put all cloud admin controlled kubevirt resources in kubevirt namespace #300

Closed

fabiand added kind/enhancement size/M labels Jul 10, 2017

vladikr force-pushed the non_default_namespaces branch 2 times, most recently from c432ee1 to 5043e92 Compare July 10, 2017 22:42

rmohr force-pushed the non_default_namespaces branch 4 times, most recently from 55555df to 203c0d8 Compare July 13, 2017 13:27

rmohr added the size/L label Jul 13, 2017

rmohr removed the size/M label Jul 13, 2017

rmohr force-pushed the non_default_namespaces branch from 203c0d8 to 722b7bc Compare July 13, 2017 13:46

fabiand approved these changes Jul 13, 2017

View reviewed changes

davidvossel approved these changes Jul 13, 2017

View reviewed changes

vladikr and others added 18 commits July 14, 2017 09:28

api support lising resources for all namespaces

499ebff

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

construct vm object domain names with namespace

5f1bbce

Separate the namespace from the libvirt domain name, when constructing the vm object. Adjusting the VM and Migration structures to consider and non-default namespace. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Adjust virt-handler cmd to stop using the default namespace

469afdc

virt-handler cmd would construct vms with a domain namespace. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Switch virt-controller to watch for changes in all namespaces

118a4ca

So far the virt-controller was watching for object changes only in the default name space. Switching it to watch for cahnges in all namespaces. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

make FetchVM receive a namespace argument

bbabb5a

FetchVM and FetchMigration were only looking for objects in the default namespace. Switching it to use a namespace argument. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Switch virt-controller methods to use a non-default namespace

cd964bd

Changing virt-controller methods to use the namespace of a relevant object, instead of the deafult one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Switch virt-handler methods to use a non-default namespace

5d333d0

Changing virt-handler methods to use the namespace of a relevant object, instead of the deafult one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Store namespace as a domain name prefix

8f5eb36

libvirt domain name will be constructed with a vm namespace prefix and '>' delimiter, E.g. namespace>name This will allow starting similarly named domains in different vm namespaces. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

adjust spice look up

3583ce3

spice will use vm namespace instead of the default one. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

functional tests for non-default VM namespaces

3004d4d

Adding functional tests for VM creation in a non-deafult namespace, as well as a migration test. Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Varius changes to take namespaces into account

7da12fd

* Change console code, to support namespaces * Change qemu-kube script to detect docker containers, by also taking namespaces into account * Switch from '>' to '_' as namespace_name delimiter Signed-off-by: Roman Mohr <rmohr@redhat.com>

Make middleware unit test less verbose

3942395

Don't log expected stack traces to the test output. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Add namespace to iscsi dns lookups

585fb44

When a VM is not started in the same namespace, as the iscsi demo target service, DNS lookups need to contain the namespace, to get the IP of the service. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Properly forward namespace to virtctl from kubectl.sh

f97ba5a

Let "virtctl spice" subcommand wrapper in cluseter/kubectl.sh properly forward all passed in arguments to the virtctl subcommand. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Add namespace support to VM migrator

50e9d4e

In order to find the Domain in libvirt, which should be migrated, the migrator needs the namespace too, to deduce the correct Domain name. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Move all tests to a separate namespace

2b07f58

Signed-off-by: Roman Mohr <rmohr@redhat.com>

Exit hack/build-go.sh after functional tests ran

7a37a66

Signed-off-by: Roman Mohr <rmohr@redhat.com>

rmohr force-pushed the non_default_namespaces branch from 722b7bc to 7a37a66 Compare July 14, 2017 08:19

rmohr approved these changes Jul 14, 2017

View reviewed changes

rmohr merged commit 239b502 into kubevirt:master Jul 14, 2017

kubevirt-bot pushed a commit to kubevirt-bot/kubevirt that referenced this pull request Dec 7, 2021

Add Petr as approver (kubevirt#249)

6714c18

Signed-off-by: Quique Llorente <ellorent@redhat.com>

		@@ -93,14 +93,14 @@ func (v vmService) DeleteVMPod(vm corev1.VM) error {
		precond.MustNotBeNil(vm)
		precond.MustNotBeEmpty(vm.GetObjectMeta().GetName())

Non-default namespaces #249

Non-default namespaces #249

Conversation

vladikr commented Jun 12, 2017 • edited by rmohr Loading

kubevirt-bot commented Jun 12, 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

Choose a reason for hiding this comment

davidvossel commented Jun 12, 2017

vladikr commented Jun 12, 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

vladikr commented Jun 23, 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

rmohr commented Jun 27, 2017

kubevirt-bot commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr left a comment

Choose a reason for hiding this comment

rmohr commented Jul 12, 2017

rmohr commented Jul 13, 2017

rmohr commented Jul 13, 2017

fabiand left a comment

Choose a reason for hiding this comment

davidvossel left a comment

Choose a reason for hiding this comment

rmohr commented Jul 14, 2017

vladikr commented Jun 12, 2017 •

edited by rmohr

Loading