Add tests for k0scontrolplane controller #806
Conversation
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
from
a402a53 to
2201865
Compare
apedriza
changed the title
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
8 times, most recently
from
8da9d1f to
6f6bee1
Compare
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
18 times, most recently
from
2ecdcb8 to
6a21e3f
Compare
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
11 times, most recently
from
2ede89b to
53c8370
Compare
| }, | ||
| Client: client.Options{ | ||
| Cache: &client.CacheOptions{ | ||
| DisableFor: []client.Object{ |
There was a problem hiding this comment.
If caching is not disabled in these cases, testing might encounter errors due to delays in cache synchronization, as the cache may not immediately reflect the current state in the API server
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
8 times, most recently
from
d8b18d3 to
717b8d8
Compare
Signed-off-by: Adrian Pedriza <[email protected]>
apedriza
force-pushed
the
add-envtest-for-controllers-testing
branch
from
717b8d8 to
51d1e3c
Compare
|
@makhov sorry for the noise. I didnt realize that unit/integration testing was failing due to new updates in machines reconciliation. Ready for review :) |
| ) | ||
|
|
||
| func (c *K0sController) createMachine(ctx context.Context, name string, cluster *clusterv1.Cluster, kcp *cpv1beta1.K0sControlPlane, infraRef corev1.ObjectReference, failureDomain *string) (*clusterv1.Machine, error) { | ||
| machine, err := c.generateMachine(ctx, name, cluster, kcp, infraRef, failureDomain) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error generating machine: %w", err) | ||
| } | ||
| _ = ctrl.SetControllerReference(kcp, machine, c.Scheme) | ||
| _ = ctrl.SetControllerReference(kcp, machine, c.Client.Scheme()) |
There was a problem hiding this comment.
Out of curiosity, why do we change it?
There was a problem hiding this comment.
writing tests for the controller I saw that we access the schema at this point and I thought about refactoring it using the client schema directly and remove K0scontroller.Scheme field. Both are obtained from manager.Manager so I thought it was safe to make this change
| @@ -210,7 +211,11 @@ func (c *K0sController) generateMachineFromTemplate(ctx context.Context, name st | |||
| return nil, err | |||
| } | |||
|
|
|||
| _ = ctrl.SetControllerReference(kcp, unstructuredMachineTemplate, c.Scheme) | |||
| _ = ctrl.SetControllerReference(cluster, unstructuredMachineTemplate, c.Client.Scheme()) | |||
| err = c.Client.Patch(ctx, unstructuredMachineTemplate, client.Merge, &client.PatchOptions{FieldManager: "k0smotron"}) | |||
There was a problem hiding this comment.
Why do you patch it here? We apply patches in createMachineFromTemplate func
There was a problem hiding this comment.
currently we patch InfraMachine but not InfraMachineTemplate. The current SetControllerReference in (Infra)MachineTemplate is not patched. If I dont patch this ownership, test will fail when checking ownership:
g.Expect(gmt.GetOwnerReferences()).To(ContainElement(metav1.OwnerReference{
APIVersion: clusterv1.GroupVersion.String(),
Kind: "Cluster",
Name: cluster.Name,
Controller: ptr.To(true),
BlockOwnerDeletion: ptr.To(true),
UID: cluster.UID,
}))
These changes add unit and integration tests on the k0scontrolplane controller. They use envtest to spin-up to
kube-apiserver+etcdwithout using a cluster. In some of these tests it is enough to usefakeclientfor the creation of objects and subsequent state check but since for others it is necessary to use envtest (an apiserver is needed) and in order to reduce the different ways of creation of the environment I have chosen to use envtest for any test. For the test implementation I have taken inspiration from how upstream projects do this job.The idea is, after agreeing on how we want this type of testing for the project, to add the rest of the tests for the other controllers.
More detailed testing of the machine reconciliation logic can be added but since it is undergoing changes I have decided to add only basic testing for the time being.