engine: make podlogmanager a reconciler #4394
Conversation
|
This pull request has been linked to Clubhouse Story #11687: move podlogmanager to be an apiserver reconciler. |
| "github.com/tilt-dev/tilt/pkg/logger" | ||
| ) | ||
|
|
||
| // Helper struct that captures Pod changes and queues up a Reconcile() |
There was a problem hiding this comment.
there's definitely some big abstraction here that I'm missing
The controller-runtime has great tools for "if object X creates object Y, then trigger a Reconcile of X every time Y changes".
I feel like we have a common pattern of "object X has a field that references object Y, which means we need a reconcile every time Y changes", and then need lots of custom mapping code to do that efficiently.
not sure what the right answer is yet...
There was a problem hiding this comment.
I get the sense that we might be working against the desired pattern here - since owner references are actually stored on the child, that's how it can map back efficiently on the .Owns() (which is more or less syntactic sugar over .Watches()).
The Kubebuilder book does have an example of adding an index on a field for client lookups, so I guess that could be a possibility - index your pod field and then in the map func when the pod changes, look up the PodLogStreams using the index and emit the requests based off hits.
There was a problem hiding this comment.
it's even more complicated than that, because the Pod comes from a different API server than the PodLogStream
but I think you're right that the long-term goal should be to have something like Owns() that handles all of this generically....there's nothing really Pod-specific here.
nicks
force-pushed
the
nicks/ch11687
branch
3 times, most recently
from
8fef11a
to
a31e90f
Compare
| @@ -250,7 +258,7 @@ func TestLogReconnection(t *testing.T) { | |||
|
|
|||
| _, _ = writer.Write([]byte("hello world!")) | |||
| f.AssertOutputContains("hello world!") | |||
| assert.Equal(t, startTime, f.kClient.LastPodLogStartTime) | |||
| assert.Equal(t, startTime.Truncate(time.Second), f.kClient.LastPodLogStartTime) | |||
There was a problem hiding this comment.
Just curious - is this result of (intentionally) reduced precision or something else?
There was a problem hiding this comment.
ya, this is not documented super-well, but metav1.Time only provides second-level granularity https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Time.Rfc3339Copy
so all the underlying Log apis truncate this anyway
this test changed because we're serializing the API, which is where the truncation happens
|
i'm going to wait on tilt-dev/tilt-apiserver#29 before merging (though tbh i don't totally understand why that bug doesn't affect things now) |
Hello @milas,
Please review the following commits I made in branch nicks/ch11687:
ca5eb0f (2021-03-31 19:38:50 -0400)
engine: make podlogmanager a reconciler
Code review reminders, by giving a LGTM you attest that: