-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
server: make DI practical for controllers #4250
Changes from 1 commit
b3218f8
b420b9c
ac10512
855ad35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package controllers | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
|
||
ctrl "sigs.k8s.io/controller-runtime" | ||
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
|
||
"github.com/tilt-dev/tilt/internal/store" | ||
) | ||
|
||
type Controller interface { | ||
reconcile.Reconciler | ||
SetClient(client ctrlclient.Client) | ||
SetupWithManager(mgr ctrl.Manager) error | ||
} | ||
|
||
type ControllerBuilder struct { | ||
tscm *TiltServerControllerManager | ||
controllers []Controller | ||
} | ||
|
||
func NewControllerBuilder(tscm *TiltServerControllerManager, controllers []Controller) *ControllerBuilder { | ||
return &ControllerBuilder{ | ||
tscm: tscm, | ||
controllers: controllers, | ||
} | ||
} | ||
|
||
var _ store.Subscriber = &ControllerBuilder{} | ||
var _ store.SetUpper = &ControllerBuilder{} | ||
|
||
func (c *ControllerBuilder) OnChange(_ context.Context, _ store.RStore) {} | ||
|
||
func (c *ControllerBuilder) SetUp(_ context.Context, _ store.RStore) error { | ||
mgr := c.tscm.GetManager() | ||
client := c.tscm.GetClient() | ||
|
||
if mgr == nil || client == nil { | ||
return errors.New("controller manager not initialized") | ||
} | ||
|
||
for _, controller := range c.controllers { | ||
controller.SetClient(client) | ||
if err := controller.SetupWithManager(mgr); err != nil { | ||
return fmt.Errorf("error initializing %T controller: %v", controller, err) | ||
} | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package core | ||
|
||
import ( | ||
"context" | ||
|
||
ctrl "sigs.k8s.io/controller-runtime" | ||
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
||
"github.com/tilt-dev/tilt/internal/store" | ||
filewatches "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1" | ||
) | ||
|
||
// FileWatchController reconciles a FileWatch object | ||
type FileWatchController struct { | ||
ctrlclient.Client | ||
Store store.RStore | ||
} | ||
|
||
func NewFileWatchController(store store.RStore) *FileWatchController { | ||
return &FileWatchController{ | ||
Store: store, | ||
} | ||
} | ||
|
||
// +kubebuilder:rbac:groups=,resources=filewatches,verbs=get;list;watch;create;update;patch;delete | ||
// +kubebuilder:rbac:groups=,resources=filewatches/status,verbs=get;update;patch | ||
// +kubebuilder:rbac:groups=,resources=filewatches/finalizers,verbs=update | ||
|
||
func (r *FileWatchController) Reconcile(_ context.Context, _ ctrl.Request) (ctrl.Result, error) { | ||
// this is currently a no-op stub | ||
return ctrl.Result{}, nil | ||
} | ||
|
||
func (r *FileWatchController) SetClient(client ctrlclient.Client) { | ||
r.Client = client | ||
} | ||
|
||
func (r *FileWatchController) SetupWithManager(mgr ctrl.Manager) error { | ||
return ctrl.NewControllerManagedBy(mgr). | ||
For(&filewatches.FileWatch{}). | ||
Owns(&filewatches.FileWatch{}). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't think this is right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops I'll take that off, this controller is in a weird state because it just exists to ensure DI stuff is functional. (It might be worth making a "sample" entity in |
||
Complete(r) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,45 +6,54 @@ import ( | |
"fmt" | ||
|
||
"k8s.io/apimachinery/pkg/runtime" | ||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
"k8s.io/client-go/rest" | ||
ctrl "sigs.k8s.io/controller-runtime" | ||
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
|
||
"github.com/tilt-dev/tilt/internal/hud/server" | ||
"github.com/tilt-dev/tilt/internal/store" | ||
corev1alpha1 "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1" | ||
"github.com/tilt-dev/tilt/pkg/logger" | ||
) | ||
|
||
type TiltServerControllerManager struct { | ||
config *rest.Config | ||
cancel context.CancelFunc | ||
config *rest.Config | ||
scheme *runtime.Scheme | ||
manager ctrl.Manager | ||
cancel context.CancelFunc | ||
} | ||
|
||
var _ store.SetUpper = &TiltServerControllerManager{} | ||
var _ store.Subscriber = &TiltServerControllerManager{} | ||
var _ store.TearDowner = &TiltServerControllerManager{} | ||
|
||
func NewTiltServerControllerManager(config *server.APIServerConfig) *TiltServerControllerManager { | ||
func NewTiltServerControllerManager(config *server.APIServerConfig, scheme *runtime.Scheme) (*TiltServerControllerManager, error) { | ||
return &TiltServerControllerManager{ | ||
config: config.GenericConfig.LoopbackClientConfig, | ||
scheme: scheme, | ||
}, nil | ||
} | ||
|
||
func (m *TiltServerControllerManager) GetManager() ctrl.Manager { | ||
return m.manager | ||
} | ||
|
||
func (m *TiltServerControllerManager) GetClient() ctrlclient.Client { | ||
if m.manager == nil { | ||
return nil | ||
} | ||
return m.manager.GetClient() | ||
} | ||
|
||
func (m *TiltServerControllerManager) SetUp(ctx context.Context, st store.RStore) error { | ||
scheme := runtime.NewScheme() | ||
|
||
ctx, m.cancel = context.WithCancel(ctx) | ||
|
||
// TODO(milas): we should provide a logr.Logger facade for our logger rather than using zap | ||
w := logger.Get(ctx).Writer(logger.DebugLvl) | ||
ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(w))) | ||
|
||
utilruntime.Must(corev1alpha1.AddToScheme(scheme)) | ||
|
||
mgr, err := ctrl.NewManager(m.config, ctrl.Options{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we dependency-inject the manager? i think that would avoid a lot of the circular initialization issues you're running into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately the manager is the problem -- Another possibility would be to create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Take a look at cc45e52 and see if you think that's an improvement? It really just lets controllers pretend to get created as expected by getting a Not sure that the extra indirection is worth saving on needing to call |
||
Scheme: scheme, | ||
Scheme: m.scheme, | ||
// controller manager lazily listens on a port if a webhook is registered; this functionality | ||
// is currently NOT used; to prevent it from listening on a default port (9443) and potentially | ||
// causing conflicts running multiple instances of tilt, this is set to an invalid value | ||
|
@@ -58,15 +67,18 @@ func (m *TiltServerControllerManager) SetUp(ctx context.Context, st store.RStore | |
LeaderElectionID: "tilt-apiserver-ctrl", | ||
}) | ||
if err != nil { | ||
return fmt.Errorf("unable to start manager: %v", err) | ||
return fmt.Errorf("unable to create controller manager: %v", err) | ||
} | ||
|
||
go func() { | ||
if err := mgr.Start(ctx); err != nil && !errors.Is(err, context.Canceled) { | ||
err = fmt.Errorf("controller manager stopped unexpectedly: %v", err) | ||
st.Dispatch(store.NewErrorAction(err)) | ||
} | ||
}() | ||
|
||
m.manager = mgr | ||
|
||
return nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package controllers | ||
|
||
import ( | ||
"k8s.io/apimachinery/pkg/runtime" | ||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
|
||
corev1alpha1 "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1" | ||
) | ||
|
||
func NewScheme() *runtime.Scheme { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think GenericConfig in apiserver.Config should have a scheme on it now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yep, it's on
The two versions are (Since it's kind of weird/indirect, when there are multi versions of an entity, the storage version is always going to get the LMK if I'm missing something or doing something wrong - I am not familiar with the depths of the API server There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't know, let's just check this in and i can debug what's going on... |
||
scheme := runtime.NewScheme() | ||
|
||
// modify pkg/apis/core/v1alpha1/register.go to get new types within core group registered | ||
utilruntime.Must(corev1alpha1.AddToScheme(scheme)) | ||
|
||
return scheme | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,28 @@ | ||
package controllers | ||
|
||
import "github.com/google/wire" | ||
import ( | ||
"github.com/google/wire" | ||
|
||
"github.com/tilt-dev/tilt/internal/controllers/core" | ||
) | ||
|
||
var controllerSet = wire.NewSet( | ||
core.NewFileWatchController, | ||
|
||
ProvideControllers, | ||
) | ||
|
||
func ProvideControllers(fileWatch *core.FileWatchController) []Controller { | ||
return []Controller{ | ||
fileWatch, | ||
} | ||
} | ||
|
||
var WireSet = wire.NewSet( | ||
NewTiltServerControllerManager, | ||
|
||
NewScheme, | ||
NewControllerBuilder, | ||
|
||
controllerSet, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was assuming we'd put each controller in its own package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - this was mostly an artifact of running kubebuilder/apiserver-boot at some point, I'll move it