Implement events #17
Implement events #17
Conversation
openshift-ci-robot
added
the
size/M
michaelgugino
force-pushed
the
event-actuator
branch
from
a336f97
to
dbe9d76
Compare
| CoreClient: fake.NewFakeClient(), | ||
| EventRecorder: recorder, | ||
| }) | ||
| if machineActuator == nil { |
There was a problem hiding this comment.
the way it's right now In a follow up we can may be allow to create a secret with credentials, which will be used to create a computeService which won't work and so we validate failure event.
Then if we want to validate success we'd need to allow computeservice injection at this level
There was a problem hiding this comment.
Need to address the comments left by alberto @ #17 (comment)
|
I also think the location where events are emitted should be at source. So in the implementation calls to (lowercase) create, delete, et al and not in the wrapper calls. That way you could [probably] dispense with "noEventAction" too. |
michaelgugino
force-pushed
the
event-actuator
branch
from
dbe9d76
to
00d7a10
Compare
|
/lgtm |
| @@ -47,10 +68,17 @@ func (a *Actuator) Create(ctx context.Context, cluster *clusterv1.Cluster, machi | |||
| machine: machine, | |||
| }) | |||
| if err != nil { | |||
| return fmt.Errorf(scopeFailFmt, machine.Name, err) | |||
| fmtErr := fmt.Errorf(scopeFailFmt, machine.Name, err) | |||
| return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", fmtErr), createEventAction) | |||
There was a problem hiding this comment.
s/error decoding MachineProviderConfig/error creating machine scope would be more appropriate
There was a problem hiding this comment.
InvalidMachineConfiguration and error decoding MachineProviderConfig are not appropriate here
| @@ -93,7 +128,12 @@ func (a *Actuator) Delete(ctx context.Context, cluster *clusterv1.Cluster, machi | |||
| machine: machine, | |||
| }) | |||
| if err != nil { | |||
| return fmt.Errorf(scopeFailFmt, machine.Name, err) | |||
| fmtErr := fmt.Errorf(scopeFailFmt, machine.Name, err) | |||
| return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration("error decoding MachineProviderConfig: %v", fmtErr), deleteEventAction) | |||
There was a problem hiding this comment.
s/error decoding MachineProviderConfig/error creating machine scope would be more appropriate
There was a problem hiding this comment.
InvalidMachineConfiguration does not seem appropriate here
|
/refresh |
|
/test unit |
michaelgugino
force-pushed
the
event-actuator
branch
from
00d7a10
to
bd9ad6a
Compare
michaelgugino
force-pushed
the
event-actuator
branch
2 times, most recently
from
2570836
to
46d00fa
Compare
| @@ -47,10 +68,15 @@ func (a *Actuator) Create(ctx context.Context, cluster *clusterv1.Cluster, machi | |||
| machine: machine, | |||
| }) | |||
| if err != nil { | |||
| return fmt.Errorf(scopeFailFmt, machine.Name, err) | |||
| fmtErr := fmt.Sprintf(scopeFailFmt, machine.Name, err) | |||
| return a.handleMachineError(machine, apierrors.InvalidMachineConfiguration(fmtErr), createEventAction) | |||
There was a problem hiding this comment.
why InvalidMachineConfiguration ? this seems misleading to me, same for all appearences
There was a problem hiding this comment.
InvalidMachineConfiguration is generic error type (see https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/errors/machines.go#L42). Also see https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/apis/cluster/common/consts.go#L24-L29:
// Represents that the combination of configuration in the MachineSpec
// is not supported by this cluster. This is not a transient error, but
// indicates a state that must be fixed before progress can be made.
//
// Example: the ProviderSpec specifies an instance type that doesn't exist,
InvalidConfigurationMachineError MachineStatusError = "InvalidConfiguration"
newMachineScope might fail due to internal errors. Though, configuration is quite generic and not just about machine configuration. It's fine to consider even internal failures caused by invalid configuration.
There was a problem hiding this comment.
regardless what the comment says, that would print "InvalidConfiguration" which seems misleading to me. I would expect "InvalidConfiguration" to be used by a validate() func that as the comment says check that e.g instance type that doesn't exist. We are not forced to be using apierrors.* if they don't fit. IMHO All the abstractions here obfuscate what's actually being printed to the user for the sake of not duplicating a few lines.
This is not a blocker to me though.
There was a problem hiding this comment.
Personally, I don't think there's much utility in capture negative events. 99/100 they will be transient. We should probably only record successful events as they're the only things that are useful with our current setup. InvalidConfiguration I think is pretty useful here though; we're unlikely to recover if we fail to get a valid scope except for the infrequent case of GCP API outage.
There was a problem hiding this comment.
It's also fine to just use return a.handleMachineError(machine, apierrors.CreateMachine("failed to create scope: %v", err), createEventAction)
michaelgugino
force-pushed
the
event-actuator
branch
from
46d00fa
to
864807b
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/lgtm |
No description provided.