Discard watch event if a (valid) Model instance can't be created from its resource payload #112
Conversation
There was a problem hiding this comment.
This probably makes sense given where fiaas is currently, but ultimately this is the sort of problem that could be handled by adding a proper schema to the CRDs we create, which marks the config field as required [1]. Then those resources would be rejected by the API server before being added to the cluster.
I agree that a complete schema on the CRDs would be the proper way to solve this. ApplicationStatus already has a full schema, but Application currently only has a partial schema, since as far as I know, defining a valid full schema for that resource would require making some changes that break backwards compatibility (see fiaas/fiaas-deploy-daemon#38 (comment) for details). |
- Valid events are mapped to WatchEvent which contain the event type and Model instance - Events which do not unmarshal from json string to dict cleanly are dropped (exception is logged)
watch_list is a generator function intended to be used as a continuous stream of updates on a resource kind. When it fails to create an instance of the Model type from the payload of a watch event because the resource in the payload is not valid according to the Model class, a TypeError is raised. This stops the stream of watch events and the client has to try again/restart it by calling watch_list again. Since the watch API returns all/most resources on the initial call, if the resource that caused the exception to be raised is still present, the same thing will happen again. In this case the client will not be able to process all resources using watch_list until the invalid resource is removed. With this change, when creating a Model object from the payload of an event raises TypeError, the exception is logged at ERROR level and the event is discarded. This means that clients will be able to process all valid resources if there is one or more resources that are not valid according to the Model type. A drawback is that clients can't set their own error handling for this failure mode. Since watch_list already does the same thing if the payload of an event can not be parsed as JSON, and a watch event without the object on it isn't particularly useful, I think that is probably okay. If it becomes necessary to support custom error handling later, one option might perhaps be to let watch_list take function(s) as optional parameters, which called in the except blocks for these failure modes. The default can be the current behavior (logging the exception).
oyvindio
force-pushed
the
log-typeerror-watch_list
branch
from
7869631 to
95ae77e
Compare
This includes the change added in fiaas/k8s#112 to discard watch events if a valid Model instance can't be created. This should solve the issue described in #193.
watch_listis a generator function intended to be used as a continuous stream of updates on a resource kind. When it fails to create an instance of theModeltype from the payload of a watch event because the resource in the payload is not valid according to theModelclass, aTypeErroris raised. This stops the stream of watch events and the client has to try again/restart it by callingwatch_listagain. Since the watch API returns all/most resources on the initial call, if the resource that caused the exception to be raised is still present, the same thing will happen again. In this case the client will not be able to process all resources usingwatch_listuntil the invalid resource is removed. This is what seems to happen in fiaas/fiaas-deploy-daemon#193.With this change, when creating a
Modelobject from the payload of an event raisesTypeError, the exception is logged at ERROR level and the event is discarded. This means that clients will be able to process all valid resources if there is one or more resources that are not valid according to theModeltype. A drawback is that clients can't set their own error handling for this failure mode. Sincewatch_listalready does the same thing if the payload of an event can not be parsed as JSON, and a watch event without the object on it isn't particularly useful, I think that is probably okay.If it becomes necessary to support custom error handling later, one option might perhaps be to let
watch_listtake function(s) as optional parameters, which called in the except blocks for these failure modes. The default can then be logging the exception.