Move the Service controller to use the new controller model.#1208
Move the Service controller to use the new controller model.#1208google-prow-robot merged 1 commit intoknative:masterfrom
Conversation
google-prow-robot
added
do-not-merge/work-in-progress
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
google-prow-robot
added
size/L
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
This moves the service controller to the more level-based model started in #1202. This also makes the service controller start to listen to Route and Configuration events to trigger reconciliation, so that we can reflect a basic overall readiness condition based on the combination of these two sub-resources readiness conditions (I don't expect this will be the final model, but is a start). Fixes: knative/serving#1134
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
I'm going to remove the WIP since I think this is incremental progress. The service controller parts of this lack coverage, but partly because the entire service controller is uncovered. I'm going to start working on that in a fresh commit / PR based on this. /assign @vaikas-google |
mattmoor
changed the title
google-prow-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonjohnsonjr, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Can each of these be on a new line - for when we add new conditions in the future
There was a problem hiding this comment.
I will push a change to normalize to that style across *_types.go
There was a problem hiding this comment.
I prefer PropagateConfigurationStatus
There was a problem hiding this comment.
I will push a change with this rename.
There was a problem hiding this comment.
I will push a change with this rename.
There was a problem hiding this comment.
I would suggest more granularity and test the scenarios below. Let me know if it's too pedantic. I'm suggesting this because there's no test for when a configuration fails and recovers. And there's some overlap testing the route failure flow in TestTypicalServiceFlow and TestRouteFailurePropagation
Each bullet point would be a scenario to test given the conditions when ...
service happy flow
when the service ready condition is unknown
- route ready condition becomes true
- configuration ready condition becomes true
- both route and configuration ready condition becomes true
failure propagation
when the service ready condition is true
and both route & configuration ready conditions are true
- route ready condition becomes false
- configuration ready condition becomes false
configuration recovery
when the service ready condition is false
and route ready condition is true
and configuration ready condition is false
- configuration ready condition becomes true
route recovery
when the service ready condition is false
and route ready condition is false
and configuration ready condition is true
- route ready condition becomes true
There was a problem hiding this comment.
I opened #1224 to track this. This is a great list, thanks.
There was a problem hiding this comment.
Can we name this serviceInformer for clarity
There was a problem hiding this comment.
Use https://github.com/kubernetes/apimachinery/blob/master/pkg/api/equality/semantic.go to make sure they are semantically the same
One reason being cmp.Diff doesn't seem to equate empty and nil maps as equal. Hence the need for these cmp options https://godoc.org/github.com/google/go-cmp/cmp/cmpopts
There was a problem hiding this comment.
Agreed, though IMO we should continue using cmp.Diff in tests.
There was a problem hiding this comment.
I left the cmp.Diff for logging to the controller logs, but switched the actual comparison the equality.Semantic.DeepEqual. Will add this to my change incorporating feedback.
There was a problem hiding this comment.
avoid cmp.Diff as mentioned above
There was a problem hiding this comment.
I'm not opposed to this solution, but seems like defaulter-gen might be cleaner. If only defaulter-gen were documented...
There was a problem hiding this comment.
This appears to skip reconciling the configuration when it's just been created. I'm curious if there's a reason not to reconcile the configuration immediately. The informer cache won't have it, but that's likely not a problem because we already have the latest state as returned by Create.
It's true that the service will be re-added to the workqueue almost immediately (depending on latency of the configuration informer), but if the workqueue is long there might be a noticeable delay before it comes back.
There was a problem hiding this comment.
I believe that all this would end up reconciling is changes made by mutating webhooks (or the API Server). It's a big problem if webhooks make changes that disagree with our reconciliation (we'll flip-flop every resync), so I'm not sure I see the value in this. I may be naive here, but what would we miss by not doing this?
There was a problem hiding this comment.
My concern is that I don't see any documentation or test that createConfiguration, reconcileConfiguration, and the webhook must all result in exactly the same object, but the code relies on that being true in subtle ways. It would be easy to change only one of the create or reconcile methods without changing the other, since things would still appear to work in practice (because of the immediate second reconcile).
Probably the easiest solution for this (minus the webhook issue) is to add a unit test that diffs the output of createConfiguration and reconcileConfiguration.
An alternative is to allow reconcileConfiguration to take a nil Configuration:
func (c *Controller) reconcileConfiguration(service *v1alpha1.Service, config *v1alpha1.Configuration) (*v1alpha1.Configuration, error) {
desiredConfig := MakeServiceConfiguration(service)
if config == nil {
return c.ElaClientSet.ServingV1alpha1().Configurations(service.Namespace).Create(config)
else {
desiredConfig.Spec.Generation = config.Spec.Generation
if equality.Semantic.DeepEqual(desiredConfig.Spec, config.Spec) != {
config.Spec = desiredConfig.Spec
return c.ElaClientSet.ServingV1alpha1().Configurations(service.Namespace).Update(config)
}
}
return config, nil
}The webhook issue is trickier to solve fully, but could at least be mitigated using defaulter-gen (because the generated defaulters would be defined in one place and used by both webhook and controller).
There was a problem hiding this comment.
Same question as above re: not reconciling the route immediately.
There was a problem hiding this comment.
Agreed, though IMO we should continue using cmp.Diff in tests.
There was a problem hiding this comment.
I think I'd collapse these into one line without setting configClient, but I'm fine with this approach too.
There was a problem hiding this comment.
Looks like I already did this in configuration.go, will add this to the change I'm pushing.
There was a problem hiding this comment.
This function (and createRoute below it) do so little I wonder if it might be clearer to inline them into the reconcile.
There was a problem hiding this comment.
I think I have a mild preference for this slightly more readable method name for readability of the core reconciliation method, even if it is now one line :)
7 participants
Fixes: #1134