Reconciler configs are snapshotted at the start of each reconciliation#2009
Conversation
knative-prow-robot
added
the
size/XXL
|
This will need to be done for other reconciler's |
dprotaso
force-pushed
the
reconcile-with-stable-configs
branch
from
293d83c to
b28b3d1
Compare
|
/test pull-knative-serving-integration-tests |
dprotaso
force-pushed
the
reconcile-with-stable-configs
branch
4 times, most recently
from
6ec9663 to
6b461cf
Compare
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
Did a pass over this, and left a handful of comments.
Generally, I think this is a net positive for the readability of the controller code, and would like to see us head more in this direction.
The use of reflection makes me a little uncomfortable about painful hard to debug bugs. I also left one comment about potentially making more direct use of Context.
I'd also suggest trying to break a couple pieces off of this that I think are separable and independently useful to distill this PR a bit.
Thanks!
-M
| func NewUntypedStore( | ||
| name string, | ||
| logger Logger, | ||
| constructors ...interface{}) *UntypedStore { |
There was a problem hiding this comment.
The reflection here makes me a bit uncomfortable, and I wonder if we can get close to this without it?
type CfgCtor func(...)...
type CfgCtors map[string]CfgCtor
func NewUntypedStore(
name string,
logger Logger,
ctors CfgCtors) *UntypedStore {The map enforces the argument pairing, and can be called with only slightly more code:
_ = NewUntypedStore(name, logger, store.CfgCtors{
"foo": fooCtor,
"bar": barCtor,
})There may be value in a map (e.g. explicit pairing, string is strongly typed) over the variadic convention even if the map's value has to be interface{} (I suspect at least some of the motivation for reflection is that Go doesn't allow[1] covariant return types?).
[1] - I was curious: golang/go#7512
There was a problem hiding this comment.
The reflection here makes me a bit uncomfortable
It's solely needed for the constructor methods - not the keys
The map enforces the argument pairing, and can be called with only slightly more code:
Yeah I was optimizing for succinctness. Plus being comfortable with the non-idiomatic Go since it's pattern in Objective-C example
I'll switch to the map if that makes it clearer.
I suspect at least some of the motivation for reflection is that Go doesn't allow
Yup exactly - the typing is in go is very strict.
| } | ||
|
|
||
| func (s *UntypedStore) registerConfig(name string, constructor interface{}) { | ||
| // TODO(dprotaso) assert constructor type |
There was a problem hiding this comment.
I feel like if this were possible we wouldn't need reflection (see my comment about covariant return types above).
There was a problem hiding this comment.
Given the type system you cannot do this without reflection.
| cfgs.Observability, | ||
| cfgs.Autoscaler, | ||
| cfgs.Controller, | ||
| ) |
There was a problem hiding this comment.
(a train of thought)
My first thought here was:
Should we just pass
cfgs?
However, that led me to wonder:
If we're just passing one additional thing, should we just pass
ctx?
Which leads me to wonder:
Should we just be snapshotting individual configs to the context? Would that save us some code? Might there be ways to push config attachment to context into the common Reconciler infrastructure (like we have with the logger)?
There was a problem hiding this comment.
Should we just pass cfgs?
In this specific example all configs are used so we could.
If we're just passing one additional thing, should we just pass ctx?
No - I'm wary of doing this because it makes inferring a function's actual arguments difficult
Passing a context.Context also signals that there's some 'cancelable' operation that might be occurring. So I wouldn't want to pass that to any of the resource.Make* methods.
Should we just be snapshotting individual configs to the context? Would that save us some code?
For the specific example above it would be more code since you'd load N configs from the context vs. one. It's something to consider
Might there be ways to push config attachment to context into the common Reconciler infrastructure (like we have with the logger)?
Maybe - baby steps first :)
dprotaso
force-pushed
the
reconcile-with-stable-configs
branch
2 times, most recently
from
9ab66f8 to
9564e22
Compare
dprotaso
changed the title
dprotaso
force-pushed
the
reconcile-with-stable-configs
branch
from
9564e22 to
d9e1721
Compare
|
/lgtm Can you doc comment the public parts of |
knative-prow-robot
added
lgtm
|
The following is the coverage report on pkg/.
|
|
godoc updated! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, 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 |
|
/hold cancel |
knative-prow-robot
removed
the
do-not-merge/hold
5 participants
Currently, when reconciling a revision it's possible that when creating different dependent resources (ie. k8s services, kpa, deployments) different configuration values are read.
This race can be addressed by snapshotting the configs needed at the start of the reconciliation.