search

airshipit / sip

4 stars 1 forks source link

envtest control plane is insufficient #14

Open seaneagan opened 3 years ago

seaneagan commented 3 years ago

Problem description (if applicable) The envtest control plane only has apiserver and etcd, and so lacks the ability to actually reconcile workloads, such as: 1) The services produced by SIP 2) Dependency workloads of 1) such as cert-manager and helm-controller.

This prevents us from interacting with these workloads in the code. For example we should be ensuring that 1) is reconciled as part of the SIPCluster reconciliation, but that would cause the tests to fail every time, as they will never be reconciled. We would also need to be able to reconcile 2) in order for 1) to reconcile in the first place.

Proposed change Two options: 1) Implement a UseExistingCluster approach: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/envtest#APIServer a) automatic reconciliation by cluster (easier) b) heavyweight/fragile 2) Implement a FakeClient approach: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/client/fake a) see limitations in above link b) requires manual reconciliation in code (harder) c) lightweight/reliable

seaneagan commented 3 years ago

It looks like fake.Client will not meet our needs, as it can't "react" to objects being created to for example update their status to mock reconciliation, however this appears to be in progress: https://github.com/kubernetes-sigs/controller-runtime/pull/1325

teoyaomiqui commented 3 years ago

@seaneagan, I think the fake client (option 2) is a way to go for us. There is no way to create pods in a Kubernetes cluster without nodes. While there are limitations with the fake client, and it can't mimic the exact behavior of the real k8s cluster, we can try to cover these limitations with integration tests.

Using pre-existing Kubernetes cluster with nodes is basically the same as running tests with bash (you will need to bootstrap it, tear down, check the status...), but much more complex.

So I incline to use fake client for unit-tests and use integration tests with bash and standalone k8s cluster.

seaneagan commented 3 years ago

@teoyaomiqui The problem is the fake client doesn't provide a hook to be notified once the objects have been created by the code under test, so that the test code can update their status, in order for the code under test to actually verify that the status of these objects becomes successful. However, if we can at least put the wait logic at the very end of the SIPCluster reconciliation, for example by creating all services in parallel and then waiting for them all in parallel, then the only thing that couldn't be tested would be the subsequent updating of the SIPCluster status to indicate that the services reconciled successfully. So this may be an acceptable limitation for SIP tests. For Vino, the BMH creation logic depends on the DaemonSet creating the pods first, which we cannot accomplish with fake.Client. So I think we would be very limited in what we could test there (basically just the creation of the DaemonSet).

seaneagan commented 3 years ago

I do agree that the UseExistingCluster probably does not buy us much over just augmenting our integration tests.

teoyaomiqui commented 3 years ago

@seaneagan If we are talking about unit-tests, I think were-arrange functions in such a way that we test them one by one.

But you are right; we can't test Reconcile function end to end with unit tests. But we can do this in integration tests, outside go runtime.

The requirement of an external k8s cluster for unit-tests seems like a huge limitation. If we could run part of the tests with it, and part of them without? :)

seaneagan commented 3 years ago

+1, we could try to extract as much as possible into separate package(s) which do not depend on interaction with the API client (or backing API server), making them easy to unit test.