Introduce sharedmain.WebhookMain* #1084
Conversation
knative-prow-robot
added
the
size/L
pmorie
force-pushed
the
separate-webhooks
branch
from
d9aa06b to
a127aa3
Compare
knative-prow-robot
added
do-not-merge/hold
injection/sharedmain/main.go
Outdated
| if err != nil { | ||
| log.Fatalf("Error building kubeconfig: %v", err) | ||
| } | ||
| cfg := ParseAndGetConfigOrDie() |
There was a problem hiding this comment.
nit, since you don't use the cfg, you could consider just:
MainWithConfig(ctx, component, ParseAndGetConfigOrDie(), ctors...)
but not a big deal either way.
injection/sharedmain/main.go
Outdated
| } | ||
|
|
||
| profilingHandler := profiling.NewHandler(logger, false) | ||
|
|
||
| // Watch the logging config map and dynamically update logging levels. |
There was a problem hiding this comment.
maybe move this and observability die above setting up the controllers so it's grouped by where the pieces necessary to construct it are set up? My reasoning being that if all the dies are as early as possible, just makes it easier to reason about the code?
Again, not a big deal :)
There was a problem hiding this comment.
Ack, I tried to preserve the existing flow as much as possible for lower review overhead, but reading it now it's a bit apart. I'll rejigger
injection/sharedmain/main.go
Outdated
| log.Fatalf("Error reading/parsing logging configuration: %v", err) | ||
| } | ||
| logger, atomicLevel := logging.NewLoggerFromConfig(loggingConfig, component) | ||
| ctx = logging.WithLogger(ctx, logger) |
There was a problem hiding this comment.
You might want to document this in the function comments. I think the intent is that this will also modify the incoming context and attach a logger to it. I think it would be good to document that because it's not (to me) immediately clear that's what's happening. Alternatively, if the intent is for the caller to then create / decorate their context with the logger is to return above with:
return logging.NewLoggerFromConfig(loggingConfig, component).
There was a problem hiding this comment.
I think the code as it stands is even wrong. WithLogger will create a new context that you have no access to in this case.
I agree, we should probably just do the WithLogger call right after calling SetupLoggerOrDie at the calling side.
There was a problem hiding this comment.
Whoops, good catch. I will fix
| if _, err := kubeclient.Get(ctx).CoreV1().ConfigMaps(system.Namespace()).Get(logging.ConfigMapName(), | ||
| metav1.GetOptions{}); err == nil { | ||
| cmw.Watch(logging.ConfigMapName(), logging.UpdateLevelFromConfigMap(logger, atomicLevel, component)) | ||
| } else if !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
What happens if it is not found? Seems like it should also fail? Or put another way, is there a reason not to die on any error?
There was a problem hiding this comment.
This is a copy of the code as it lives in the Main function today. This has been changed not to cause the component to die a while ago.
There was a problem hiding this comment.
Then I think at the very least we should clearly document what's going on. There are cases where this will NOT establish a watch nor die? Seems like if there's any reason (temporary can't reach), this will fail? Or maybe I'm just misunderstanding something :)
There was a problem hiding this comment.
See #872 for the broader reasoning of that behavior. I agree we should add into the comment that this will not watch the logging config if it doesn't exist (but die on any other error)
| cmw.Watch(metrics.ConfigMapName(), | ||
| metrics.UpdateExporterFromConfigMap(component, logger), | ||
| profilingHandler.UpdateFromConfigMap) | ||
| } else if !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
ditto here, what happens if it fails for another reason?
pmorie
force-pushed
the
separate-webhooks
branch
from
a127aa3 to
f7be8ca
Compare
pmorie
force-pushed
the
separate-webhooks
branch
from
f7be8ca to
0ecf3a5
Compare
|
/lgtm |
pmorie
force-pushed
the
separate-webhooks
branch
from
0ecf3a5 to
b3b62da
Compare
pmorie
force-pushed
the
separate-webhooks
branch
2 times, most recently
from
62e0c6f to
7e9c53a
Compare
pmorie
force-pushed
the
separate-webhooks
branch
from
7e9c53a to
c92dafc
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, pmorie The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
knative-prow-robot
removed
the
do-not-merge/hold
WebhookMain* now implements the old main flow - while Main* no longer serves webhooks. This allows us to retain the existing behavior for webhooks - which require additional investigation / decision making on how to deal with HA concerns.
Related to #1007