Add tests for actuator events #107

alexander-demicev · 2020-07-23T12:28:17Z

This PR adds tests for actuator events, but this required couple of other changes.

Pass context to machine scope
Improve error formatting and make it consistent with other providers
Add GCP client builder, similar to what we have in AWS. This allows mocking the client in a cleaner way.
Validate machine labels in update()

https://github.com/openshift/machine-api-operator/blob/master/pkg/controller/vsphere/actuator_test.go
https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/actuator_test.go

Danil-Grigorev · 2020-07-23T12:39:44Z

pkg/cloud/gcp/actuators/machine/actuator_test.go

+	doneMgr := make(chan struct{})
+
+	go func() {
+		g.Expect(mgr.Start(doneMgr)).To(Succeed())


Does it mean that if the manager fails to start, the channel could potentially stay open? https://github.com/openshift/cluster-api-provider-gcp/pull/107/files#diff-1842c9950ada81a773ba54f690df1cefR58 defer should belong here - https://github.com/openshift/cluster-api-provider-gcp/pull/107/files#diff-1842c9950ada81a773ba54f690df1cefR54

The channel should be closed when the test exits, I don't think there's any issues with the manager not starting right here, apart from the fact we can't detect whether it has started/stopped. If you want to be thorough I would suggest the use of a waitgroup to ensure the goroutine finishes before the tests end

Danil-Grigorev · 2020-07-23T12:47:22Z

pkg/cloud/gcp/actuators/machine/machine_scope.go

 		klog.Errorf("Failed to patch machine %q: %v", s.machine.GetName(), err)
 		return err
 	}

 	s.machine.Status = statusCopy

 	// patch status
-	if err := s.coreClient.Status().Patch(context.Background(), s.machine, s.machineToBePatched); err != nil {


You could simply pass s as a first argument.

I'd leave s.Context for better readability.

Danil-Grigorev · 2020-07-23T12:52:21Z

pkg/cloud/gcp/actuators/machine/machine_scope.go

@@ -45,6 +49,10 @@ type machineScope struct {
 // newMachineScope creates a new MachineScope from the supplied parameters.
 // This is meant to be called for each machine actuator operation.
 func newMachineScope(params machineScopeParams) (*machineScope, error) {
+	if params.Context == nil {


I think we should default to context.Background here, as I can't yet see real uses for other context values

Danil-Grigorev · 2020-07-23T13:04:03Z

pkg/cloud/gcp/actuators/machine/actuator_test.go

-	eventsChannel := make(chan string, 1)
-	recorder := &record.FakeRecorder{
-		Events: eventsChannel,
+func TestMachineEvents(t *testing.T) {


I think this method should be called TestActuatorScopeEvents or just TestActuatorEvents

enxebre · 2020-07-24T10:38:07Z

pkg/cloud/gcp/actuators/machine/actuator.go

@@ -61,12 +62,14 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error
 		machine:    machine,
 	})
 	if err != nil {
-		return a.handleMachineError(machine, err, createEventAction)
+		fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err)


wouldn't this shadow the error type breaking consumers?

In Go 1.13 it shouldn't, https://blog.golang.org/go1.13-errors

Consumers will need to make sure that they use errors.Is or errors.As from the errors standard library if they want to check the type, I believe we are already doing this in MAO but worth checking if there's anywhere else expecting a particular error type

enxebre · 2020-07-24T10:44:50Z

pkg/cloud/gcp/actuators/machine/reconciler.go

@@ -168,6 +168,10 @@ func (r *Reconciler) create() error {
 }

 func (r *Reconciler) update() error {
+	if err := validateMachine(*r.machine, *r.providerSpec); err != nil {


what's the rationale behind introducing this in the PR?

We have this validation in other providers, and tests for actuator events cover this case. I'd like to keep these tests consistent among all providers.

JoelSpeed · 2020-07-29T13:53:43Z

pkg/cloud/gcp/actuators/machine/actuator.go

@@ -61,12 +62,14 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error
 		machine:    machine,
 	})
 	if err != nil {
-		return a.handleMachineError(machine, err, createEventAction)
+		fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err)


Consumers will need to make sure that they use errors.Is or errors.As from the errors standard library if they want to check the type, I believe we are already doing this in MAO but worth checking if there's anywhere else expecting a particular error type

JoelSpeed · 2020-07-29T14:01:00Z

pkg/cloud/gcp/actuators/machine/actuator_test.go

+	doneMgr := make(chan struct{})
+
+	go func() {
+		g.Expect(mgr.Start(doneMgr)).To(Succeed())


The channel should be closed when the test exits, I don't think there's any issues with the manager not starting right here, apart from the fact we can't detect whether it has started/stopped. If you want to be thorough I would suggest the use of a waitgroup to ensure the goroutine finishes before the tests end

JoelSpeed · 2020-07-29T14:15:02Z

pkg/cloud/gcp/actuators/machine/machine_scope.go

@@ -18,12 +17,17 @@ import (

 // machineScopeParams defines the input parameters used to create a new MachineScope.
 type machineScopeParams struct {
-	coreClient controllerclient.Client
-	machine    *machinev1.Machine
+	context.Context


Out of interest, why embed the context rather than have it as an explicit field?

I don't think there is any benefit in it, but we have the same in vsphere https://github.com/openshift/machine-api-operator/blob/master/pkg/controller/vsphere/machine_scope.go#L33 + I saw a similar thing in upstream https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/master/pkg/context/controller_manager_context.go#L35

JoelSpeed · 2020-07-29T14:15:30Z

pkg/cloud/gcp/actuators/machine/machine_scope.go

 }

 // machineScope defines a scope defined around a machine and its cluster.
 type machineScope struct {
+	context.Context


Same as above, is there a benefit to embedding?

JoelSpeed · 2020-07-31T12:36:24Z

/approve
/retest

openshift-ci-robot · 2020-07-31T12:36:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enxebre · 2020-08-03T09:27:54Z

/lgtm

openshift-bot · 2020-08-03T09:32:37Z

/retest