Return sync errors in ListServices #1410

rndstr · 2018-09-28T23:34:39Z

When applying the Kubernetes YAML fails we now return this error in
ListServices as part of the rollout messages.

daemon/daemon.go

+	JobStatusCache     *job.StatusCache
+	EventWriter        event.EventWriter
+	Logger             log.Logger
+	ResourceSyncErrors map[flux.ResourceID]error


daemon/daemon.go

@@ -136,6 +138,10 @@ func (d *Daemon) ListServicesWithOptions(ctx context.Context, opts v11.ListServi
 		case service.IsSystem:
 			readOnly = v6.ReadOnlySystem
 		}
+		if syncerr, ok := d.ResourceSyncErrors[service.ID]; ok {
+			service.Rollout.Messages = append(service.Rollout.Messages, syncerr.Error())


daemon/loop.go

@@ -192,6 +192,11 @@ func (d *Daemon) doSync(logger log.Logger) (retErr error) {
 	}

 	var syncErrors []event.ResourceError
+	// Since Sync() applies *all* resources we clear all errors here.
+	// If there is a recurring issue it will show up every time when
+	// this code is run.


cluster/cluster.go

@@ -17,7 +17,7 @@ type Cluster interface {
 	SomeControllers([]flux.ResourceID) ([]Controller, error)
 	Ping() error
 	Export() ([]byte, error)
-	Sync(SyncDef) error
+	Sync(SyncDef, bool) error


squaremo · 2018-10-09T10:01:18Z

What's happening with this one? Looks like there's only a couple of things holding it back:

the tests don't compile, because there's a fake Applier with an out of date method signature
it needs some thought over how sync errors are interpreted with respect to rollout progress

squaremo · 2018-10-10T21:32:01Z

That latest commit looks good 🍊

Regarding the sync error field -- we could think of (or even implement) the sync status as a Condition, in which case we'd probably treat it like we would Available or whatever. And then it'd end up in the messages, as it does in the above. I reckon it would still need some interpretation, at least because of the scenario I mentioned above. wdyt @rndstr ?

squaremo · 2018-10-17T15:14:07Z

Regarding the sync error field -- we could think of (or even implement) the sync status as a Condition, in which case we'd probably treat it like we would Available or whatever. And then it'd end up in the messages, as it does in the above. I reckon it would still need some interpretation, at least because of the scenario I mentioned above.

Thinking this through a little more, if the sync status is a condition, then it should be considered as part of the rollout status calculation. That would need control flow to change, since that calculation is all done purely from the cluster resource, at present.
A simple way to do it would be to replace the RolloutStatus field with an interface (or just a procedure) that can be invoked to get the rollout status given the sync status.

squaremo · 2018-10-23T13:00:39Z

Alternative: remove the bit that adds the sync error to the messages for now, since that's the controversial bit, and the rest is useful without it.

When applying the Kubernetes YAML fails we now record this and make it available within the daemon.

`fluxsync.Sync()` applies everything as a multidoc by default. If any apply fails, the next `Sync()` will apply the failed ones one by one while the rest is still applied as a multidoc.

rndstr · 2018-10-25T02:01:09Z

I finally came to the realization that the sync errors should not be part of the rollout status. The rollout basically provides the current state of a workload. The sync error is from whenever the yaml file last changed (and has been sync'ed past) and has no relation to the current rollout status.
Maybe that's what you already meant @squaremo?

Creating a release does commit & sync, while sync will fail during an user triggered release and provide semi-instant feedback (through polling jobs).

Providing the information in ListServices would not necessarily be useful for determining the rollout status but just the general state of the YAML.

I removed the part where it copies it into the rollout status for now and will add this directly as sub field for the Controller as SyncError.

The cluster delivers information about controllers and also performs the sync action. It is standing to reason that it should also fill in the blanks for sync errors.

squaremo · 2018-10-26T15:29:07Z

I finally came to the realization that the sync errors should not be part of the rollout status. The rollout basically provides the current state of a workload. The sync error is from whenever the yaml file last changed (and has been sync'ed past) and has no relation to the current rollout status.
Maybe that's what you already meant @squaremo?

Not quite, but this is at least as good a line of reasoning :-)

I removed the part where it copies it into the rollout status for now and will add this directly as sub field for the Controller as SyncError.

I like the moving it fully into the cluster implementation -- nice work there.

rndstr requested a review from squaremo September 28, 2018 23:34

squaremo suggested changes Sep 29, 2018

View reviewed changes

rndstr force-pushed the issue/1010-sync-errors-in-listservices branch from a2a3a39 to 231a0d3 Compare October 2, 2018 01:09

squaremo mentioned this pull request Oct 4, 2018

Flux applies changes to the cluster very slowly #1422

Closed

squaremo reviewed Oct 4, 2018

View reviewed changes

cluster/cluster.go Outdated

@@ -17,7 +17,7 @@ type Cluster interface {

SomeControllers([]flux.ResourceID) ([]Controller, error)

Ping() error

Export() ([]byte, error)

Sync(SyncDef) error

Sync(SyncDef, bool) error

This comment was marked as abuse.

Sign in to view

rndstr added 2 commits October 24, 2018 18:13

Record apply errors during Sync()

ed0d947

When applying the Kubernetes YAML fails we now record this and make it available within the daemon.

Skip multi sync if previously failed

936d121

`fluxsync.Sync()` applies everything as a multidoc by default. If any apply fails, the next `Sync()` will apply the failed ones one by one while the rest is still applied as a multidoc.

rndstr force-pushed the issue/1010-sync-errors-in-listservices branch from c818454 to 936d121 Compare October 26, 2018 00:25

Move sync error tracking to Cluster

39e2da1

The cluster delivers information about controllers and also performs the sync action. It is standing to reason that it should also fill in the blanks for sync errors.

squaremo approved these changes Oct 26, 2018

View reviewed changes

rndstr merged commit 706adb1 into master Oct 26, 2018

rndstr deleted the issue/1010-sync-errors-in-listservices branch October 26, 2018 16:25

rndstr mentioned this pull request Nov 16, 2018

Actually return the SyncError for controllers #1521

Merged

rndstr mentioned this pull request Dec 14, 2018

Update flux to pull in SyncError support weaveworks/service#2427

Merged

squaremo mentioned this pull request Aug 19, 2021

reset sync errors on successful sync #3156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return sync errors in ListServices #1410

Return sync errors in ListServices #1410

rndstr commented Sep 28, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

squaremo commented Oct 9, 2018

squaremo commented Oct 10, 2018

squaremo commented Oct 17, 2018

squaremo commented Oct 23, 2018

rndstr commented Oct 25, 2018

squaremo commented Oct 26, 2018

Return sync errors in ListServices #1410

Return sync errors in ListServices #1410

Conversation

rndstr commented Sep 28, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

squaremo commented Oct 9, 2018

squaremo commented Oct 10, 2018

squaremo commented Oct 17, 2018

squaremo commented Oct 23, 2018

rndstr commented Oct 25, 2018

squaremo commented Oct 26, 2018