Extract all data acquisition from ResourceService #1288
Conversation
This creates a new class `KubernetesDataSource` that encapsulates all code related to obtaining data from kubernetes in the `ResourceService`. This cleans the service code up a lot and makes the responsibilities clearer. The use of `Channel<ResourceChange>` has been removed. Instead, code invocations are made directly, skipping queuing. We still have two other channels that ensure front/back are decoupled. The kubernetes channel will ultimately also be removed, leaving just a channel for the subscriber. Added some API docs.
This is the second removal of a `Channel<T>` from this component. Now, updates are passed by direct code invocations, skipping queuing. We still have a downstream channel for the subscriber to decouple front/back.
drewnoakes
force-pushed
the
resource-data-flow
branch
from
caac146
to
2aae053
Compare
A few changes here: - Use more consistent names throughout. They're not "extra" or "additional" arguments. In fact they are a complete replacement. In their absence, the "spec" environment is used. - Move creation of docker inspection task to view model method, making the various "handle * update" methods more uniform, for future refactoring. - Remove special handling in `ProcessKubernetesChange` for one scenario. Simplifies the signature, and the code a fair bit.
We only need to know if the value is "upserted" (updated or inserted), or "deleted". Recent changes to threading/queuing in the code here changed some timing. The arrival of one kind of resouce can trigger the publication of another, and these would always be "modified", however they could arrive before that resource's stream published that instance. The rest of the code handles these happening out-of-order. We just need to treat add and modified the same way, so they've been merged.
There are multiple kubernetes resource types, each with its own monitoring stream. Across these resources, updates arrive concurrently. The update flows for each type of resource can interact with state stored for other resources. Therefore we use a semaphore to ensure that only one resource update can flow through the system at a time. Another option would have been to make the collections concurrent, or use explicit locking. Concurrent collections are heavy. Explicit locking is tricky to get right. This mutual exclusion via top-level semaphore seems like a safe and elegant approach for now.
When this method is passed the same collection twice, the `FromSpec` value will always be true, because every item in the list is in the list. Instead we pass `null` and consider all items as from the spec.
Each instance of `WatchKubernetesResource<T>` produces a single type, that would flow through `ProcessKubernetesChange` to look up the relevant handler. Instead, pass the handler in to `WatchKubernetesResource<T>` so it can be invoked directly.
This object is a snapshot of a service's state, not a service itself. We have another class which is a service. Append "Snapshot" to differentiate. Future work will extend this concept of snapshots more broadly.
Note that it's just the collection that becomes immutable here. The elements are still currently mutable. That will change when we split front/back ends, and have a snapshot on the backend with a view model on the front end.
This data is now available from DCP, so we don't have to launch processes to query docker for this data any more, which simplifies things quite nicely.
Now that project snapshots derive from executable snapshots, we can unify a bunch of construction logic.
We still have some types named ViewModel, but they'll be renamed later in the split.
It's not clear what object the name refers to, so make it more specific. It's internal, so we can always rename it again later. This name pairs nicely with `ObjectChange` too.
The methods that handle executable and container updates are largely the same. Extract that commonality to a new method.
| /// <summary> | ||
| /// Immutable snapshot of project state at a point in time. | ||
| /// </summary> | ||
| public class ProjectViewModel : ExecutableViewModel |
There was a problem hiding this comment.
I guess the idea here is that because, under the hood, projects are just executables with a project path, so we should have these viewmodels structured the same way? Since this doesn't get exposed to the user I don't see an issue there I guess, but I worry a bit about having irrelevant (less relevant? not normally relevant?) things like working directory, etc on the ProjectViewModel.
There was a problem hiding this comment.
That's right. From what I can tell, DCP treats projects as regular executables, and we only differentiate based on some metadata that links them to the original project. The more I looked at the code, the more projects "wanted" to be executables. When I introduced that change, the next commits removed a bunch of code and things got a bit tidier from there.
I can't think of a reason not to show working directory and arguments for projects too. Seeing arguments could be helpful, in particular.
|
|
||
| return new ExecutableViewModel | ||
| { | ||
| Name = executable.Metadata.Name, |
There was a problem hiding this comment.
Oof. It'd be nice if there was some way to reuse the initializers between these, but I can't think of one.
There was a problem hiding this comment.
Agreed. Constructors wouldn't much help. We could wrap an executable with a project, but that also feels fairly contrived and has a runtime cost (polymorphism, delegation). This bit of localised duplication seemed like a decent outcome. Open to ideas of course.
| } | ||
| } | ||
|
|
||
| if (_resourceAssociatedServicesMap.TryGetValue((resourceKind, name), out var resourceServiceMappings)) |
There was a problem hiding this comment.
I'm trying to remember, is the resourceKind part of this mapping because of the original idea that resource names were only guaranteed unique within a particular resource type? But more recently it's been stated that they'll be globally unique. So maybe we could simplify the key for the mapping now?
And by "now" I don't mean this PR since you've already got a lot of changes here. Just making sure someone else reads this thought 😊
There was a problem hiding this comment.
I have wondered the same thing. If we go for unique names, we should make a few other changes throughout. It has an impact on #1274 too, as currently we have a composite key (name/kind) in the protocol.
Fixes #1114
This creates a new class
DcpDataSourcethat encapsulates all code related to obtaining resource data from DCP in theResourceService. This cleans the service code up a lot and makes the responsibilities clearer.Channel<T>forKubernetesObjectandResourceChangeobjects have been replaced with direct code invocations, skipping queuing. We still have a downstream channel for the subscription that ensures front/back are decoupled.Added some API docs.
Before
After