Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include individual resource sync errors in Sync events #970

Merged
merged 3 commits into from Mar 12, 2018

Conversation

@squaremo
Copy link
Member

commented Mar 1, 2018

A sync can be a partial success, in that we skip lightly over individual resources failing to be applied. This means that bogus manifests don't get noticed, and the symptoms can be difficult to pin down if you are used to thinking cluster == git.

This PR surfaces those errors by including them in the Sync event, as described in #756. (It fixes #756, in fact.)

There are other measures we could take, e.g., annotating offending commits (maybe? we may not be able to pin it down); keeping a gauge of sync failures which can be alerted on. But this is a fair start.

Error error
}

type SyncErrors []SyncError

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 5, 2018

Contributor

Should this be its own commit? Could do with an explanation for what motivated the change, I'm sure future-us would agree.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

The change from type SyncError map[string]error to type SyncErrors []SyncError? It is somewhat tied up with "Use Resources throughout", since it motivates that change.

@@ -204,6 +204,8 @@ type SyncEventMetadata struct {
// policy changes, and "other" (meaning things we didn't commit
// ourselves)
Includes map[string]bool `json:"includes,omitempty"`
// Per-resource errors
Errors map[string]string `json:"errors,omitempty"`

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 5, 2018

Contributor

Can we give this a better type, perhaps? map[T]error where T is some meaningful struct type with a good String() implementation would help make this self-documenting.

I think the ResourceID code is a good example of making something that is stringy not be so stringy.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

Yes, probably just

type ErrorSource struct {
    ResourceID string
    Source string
}

(those are the two bits of information that get encoded in here). I don't think we much care about unpacking it at the other side.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

Or,

type ResourceError struct {
  ID string
  Path string
  Error string
}

type SyncEventMetadata struct {
...
  Errors []ResourceError
}

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 6, 2018

Contributor

The intended result here is that consumers of this struct (ui, notifications) don't need to search through the code to find out what kind of string we build and put in here.

The most-general, least-informative types we can JSONify are map[string]string and map[string]interface{}, both of which are basically Object. In this case the V in our map[K]V can't really be narrowed down to more than just arbitrary error text, but the K certainly can be.

If there's a way we can build a key from resource.Resource that'd probably be perfect :)

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

I'd rather have a slice of structs than have to pack things into a string so it can be a JSON-able map.

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 6, 2018

Contributor

Indeed if we don't actually need a map, let's scrap the map! Now let's figure out what a struct { string, string, string } needs... 😄

(ResourceError up there doesn't have the same issue because it has named fields, but maybe we can give ID similar treatment to the map keys. resource.ID?)

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

maybe we can give ID similar treatment to the map keys

At some point we have to bottom out to JSON types. This is that point.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

Technically we could probably use a ResourceID there, since it'll get serialised to a string anyway.

@@ -21,7 +21,7 @@ type Manifests interface {
// spec given.
UpdateDefinition(def []byte, container string, newImageID image.Ref) ([]byte, error)
// Load all the resource manifests under the path given
LoadManifests(paths ...string) (map[string]resource.Resource, error)
LoadManifests(base, first string, rest ...string) (map[string]resource.Resource, error)

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 5, 2018

Contributor

What do base, first, rest mean?

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

base is provided so that source paths can be calculated with respect to it. Although all the arguments are string, we require at least two: the base and at least one path to look in (otherwise it is easy to introduce a bug where no resources are returned for no obvious reason)

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

Worth a comment, do you think?

This comment has been minimized.

Copy link
@sambooo

sambooo Mar 6, 2018

Contributor

Worth a comment, do you think?

I do. That or a change to baseDir so it's more obvious that it's a directory. The problem with string is you can put anything in it and it's basically just []byte with the implication it might be readable when printed.

This comment has been minimized.

Copy link
@squaremo

squaremo Mar 6, 2018

Author Member

The problem with string is you can put anything in it

I shall use the filesystem path types from Go's standard lib. OK done.

@squaremo squaremo force-pushed the feature/756-include-sync-errors branch 2 times, most recently from 93bfd7c to a141155 Mar 6, 2018
@squaremo

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2018

PTAL @sambooo

@rade rade referenced this pull request Mar 9, 2018
squaremo added 3 commits Feb 28, 2018
Before we had an interface representing resources, they were generally
passed around as a pair of (id string, definition []byte). We can cut
down on the number of representations of resources by just using
`resource.Resource`.

This includes returning synchronisation errors with the whole
resource, rather than simply by name. Doing so means we will be able
to better surface sync problems with individual resources.
Include an error report in the sync notification, with resources
v. errors (with the latter coming from `kubectl` most likely).

The most useful bit of information when a resource fails to sync --
more useful than the error from kubectl, even -- is the file that had
a problem. Include that in the notification.

Secondarily: to avoid having a long, tmpfile path in messages, make
the source of resources relative to the repo.
Instead of using a map, which forces us to give a glommed-together
string as the key (so that it can be JSONed), use a slice of structs
with the unglommed data.
@squaremo squaremo force-pushed the feature/756-include-sync-errors branch from a141155 to cdca842 Mar 9, 2018
@squaremo squaremo merged commit 1a860b1 into master Mar 12, 2018
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@squaremo squaremo deleted the feature/756-include-sync-errors branch Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.