Make the service name used by a core deployer equal to the deployer name #224
Conversation
This reverts commit 09c4793. Closes projectriff#200
|
Here's an example of the Status of a deployer whose service name already existed: |
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
======================================
Coverage 9.09% 9.09%
======================================
Files 61 61
Lines 4266 4266
======================================
Hits 388 388
Misses 3845 3845
Partials 33 33
Continue to review full report at Codecov.
|
| @@ -89,6 +89,10 @@ func (ds *DeployerStatus) PropagateServiceStatus(ss *corev1.ServiceStatus) { | |||
| deployerCondSet.Manage(ds).MarkTrue(DeployerConditionServiceReady) | |||
| } | |||
|
|
|||
| func (ds *DeployerStatus) MarkServiceReconcileError(err error) { | |||
There was a problem hiding this comment.
| func (ds *DeployerStatus) MarkServiceReconcileError(err error) { | |
| func (ds *DeployerStatus) MarkServiceNotOwned(name string) { |
See an example from riff 0.4:
system/pkg/apis/core/v1alpha1/deployer_lifecycle.go
Lines 89 to 92 in 7fd0a02
There was a problem hiding this comment.
I'll adopt this. I overlooked that method.
There was a problem hiding this comment.
Actually, I see that riff 0.4 was checking services ahead of reconciliation, which is a design we've moved away from. Rather than trying to introspect the service creation error, which is fragile, I think it's sufficient to capture the service creation error in the service ready condition.
| @@ -132,6 +132,7 @@ func (r *DeployerReconciler) reconcile(ctx context.Context, log logr.Logger, dep | |||
| childService, err := r.reconcileChildService(ctx, log, deployer) | |||
| if err != nil { | |||
| log.Error(err, "unable to reconcile child Service", "deployer", deployer) | |||
| deployer.Status.MarkServiceReconcileError(err) | |||
There was a problem hiding this comment.
we only want to update the condition if the service resource fails to be created because the name already exists.
By always setting the condition for any error, transient errors will causes the ready condition of the deployer to turn false. The riff cli with the --tail flag will watch the resource until the ready condition is no longer unknown. This change as proposed will cause the CLI to report failure much more frequently.
There was a problem hiding this comment.
Makes sense. Also, that will make the condition reason createError instead of the rather bland reconcileError.
There was a problem hiding this comment.
Actually made the ServiceReady condition reason serviceCreationFailed so it makes more sense when percolated up to the Ready condition reason.
Diagnose service name collisions by capturing any service creation error in the relevant condition. See projectriff#141 for where dynamically generated service names were first introduced. Fixes projectriff#223
glyn
force-pushed
the
200-service-name
branch
from
a1451bc
to
ac6edf7
Compare
All errors returned by `.Create()` are not equal. Most api errors are transient and should requeue the task. However, create failures because there is an existing resource are special. Only those should mark the resource as not ready, and not requeue.
glyn
force-pushed
the
200-service-name
branch
from
3eaeef4
to
5b0f095
Compare
Diagnose service name collisions by capturing any service creation error in the relevant condition.
See #141 for where dynamically generated service names were first introduced.
Fixes #223
Reverts "Make the service name used by the deployer configurable".
This reverts commit 09c4793.
Closes #200