chore: initialization touchups #2676
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
pkg/operator/options.go
Outdated
| // Options exposes shared components that are initialized by the startup.Initialize() call | ||
| type Options struct { | ||
| Ctx context.Context | ||
| // Context is the root context for the operator and exposes shared components used by the entire process |
There was a problem hiding this comment.
We should rename this file to Context if we are renaming the concept
cmd/controller/main.go
Outdated
| KubeClient: ctx.KubeClient, | ||
| StartAsync: ctx.StartAsync, | ||
| }) | ||
| utilruntime.Must(ctx.Manager.AddHealthzCheck("cloud-provider", cloudProvider.LivenessProbe)) |
There was a problem hiding this comment.
Can we remove the HealthCheck interface now?
There was a problem hiding this comment.
Used all over, unfortunately.
| controllers.GetControllers(options, cloudProvider)..., | ||
| ).Start(options.Ctx); err != nil { | ||
| ctx := operator.NewContextOrDie() | ||
| cloudProvider := awscloudprovider.NewCloudProvider(ctx, cloudprovider.Options{ |
There was a problem hiding this comment.
I have some small concerns about exposing the manager through the context in this way. If someone does a typed conversion to the operator.Context object within the cloudprovider, they could then have access to the manager which seems like something we want to avoid
There was a problem hiding this comment.
Agree. Will play with this.
ellistarn
force-pushed
the
init2
branch
2 times, most recently
from
5eb6415
to
d7c62d0
Compare
pkg/operator/manager.go
Outdated
| BaseContext: newRunnableContext(config, opts, logging.FromContext(ctx)), | ||
| MetricsBindAddress: fmt.Sprintf(":%d", options.MetricsPort), | ||
| HealthProbeBindAddress: fmt.Sprintf(":%d", options.HealthProbePort), | ||
| BaseContext: func() context.Context { return ctx }, |
There was a problem hiding this comment.
Base context was introduced to avoid top-level context cancellation from stopping runnable cleanup as shown here. We shouldn't be passing the top-level context in a closure here since this gets back to the issue that controller-runtime was trying to avoid
There was a problem hiding this comment.
Fixed and added a comment
pkg/utils/injection/injection.go
Outdated
| startinformers() | ||
| return ctx | ||
| } | ||
|
|
||
| func WithNamespacedName(ctx context.Context, namespacedname types.NamespacedName) context.Context { |
There was a problem hiding this comment.
Why do these two methods stay in injection?
There was a problem hiding this comment.
They're relied upon in the AWS API which creates a circular dependency. I need to make some other changes to remove this.
ellistarn
force-pushed
the
init2
branch
2 times, most recently
from
885da49
to
751cc3b
Compare
ellistarn
force-pushed
the
init2
branch
2 times, most recently
from
ac9092f
to
09b7a74
Compare
ellistarn
force-pushed
the
init2
branch
2 times, most recently
from
a588493
to
5267d90
Compare
ellistarn
force-pushed
the
init2
branch
3 times, most recently
from
d6bf367
to
0db36e6
Compare
| } | ||
|
|
||
| // Context is the root context for the operator and exposes shared components used by the entire process | ||
| type Context struct { |
There was a problem hiding this comment.
After some thought, I want to discuss this decision a bit more. Primarily because I am concerned with the ramifications of passing fields that would normally be passed through to controller constructors as part of the context.Context struct.
In general, the Context object is used for request-specific context and cancellation and not for instantiation/performing dependency injection.
I'm curious for what your thought is in where else this Context might be passed; otherwise, I'm leaning more towards continuing calling this Options
There was a problem hiding this comment.
I think we need to sort out the neutrality of these bits too, since options currently co-mingles AWS specific and vendor agnostic parameters. Let's spend some time pair programming this.
Fixes #
Description
operatorpackageHow was this change tested?
Does this change impact docs?
Release Note
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.