Fix subsets inheriting service chaining requirement from whole app #3093
Conversation
|
This turned out to be bad and wrong because it breaks wildcard service chaining. I will look at other ways. |
itowlson
force-pushed
the
fix-subsets-not-needing-service-chaining
branch
3 times, most recently
from
a1fcfb7 to
3e03d0a
Compare
|
Okay I think this is fixed although still unlovely. I'm not sure how to unit-test it though - open to advice! |
| @@ -213,6 +215,8 @@ impl UpCommand { | |||
| )?; | |||
| } | |||
|
|
|||
| self.update_locked_app(&mut locked_app); | |||
There was a problem hiding this comment.
This moved up because the borrow of the trigger type strings (in the next stanza) now lasts longer, so I have to get the mutable borrow out the way first, or the compiler gets mad.
|
A specific concern I have here is that, as I customise the lockfile to each trigger (needed to modify the host requirements), I subset the components to be just the ones needed for that trigger. This worries me because I know Till wants to have "untriggered" components in the manifest that are used e.g. for composition. So maybe I should not filter in the lockfile, but only temporarily (in memory) as part of updating the host requirements. That said all this makes me lean towards Kate's original proposal of putting host requirements on components rather than apps. This would, I think, remove the need for per-trigger lockfiles, but oh man I am not eager to go through the whole lockfile-revving must-understand dance again... (Plus who knows if we would later find some unavoidably app-level host requirement after all and have to go through all this guff anyway.) |
For this scenario, are those types of components always pulled in via component dependencies/oci artifacts rather than being a stand alone top level component. (aside: this is where the word "component" is hard in a |
I wonder if we need a SIP that defines the unit of an "app" and the unit of a "component". Now that we have app splitting, what are app requirements vs component requirements and is there a third "splice" requirement level too? |
Signed-off-by: itowlson <[email protected]>
itowlson
force-pushed
the
fix-subsets-not-needing-service-chaining
branch
from
3e03d0a to
c2407a9
Compare
|
@lann Do you have a moment to glance over this please? I am still not sure how comfortable I feel about its radicalism, and I think you are more au fait with LockedApps and potential pitfalls in the loader-trigger-engine handoffs than anyone! |
Fixes #2915 and #3088
When an application uses local service chaining, this is noted in the lockfile, so that if a host can't provide the service chaining guarantees, it can reject the application at load time.
Unfortunately, this does not play nicely with application splitting or multi-trigger applications. If a host doesn't support service chaining, it's perfectly safe for that host to run a subset of the app that doesn't use service chaining. But currently this doesn't work because the lockfile's "host requirements" section is calculated on the basis of the whole app not the subset.
I don't much love the fix in this PR. I've added a pass to app splitting/subsetting to recalculate if service chaining is needed, but this duplicates a lot of the existing validation pass. (Perhaps they could/should be combined? Allowing a validator to mutate the validatee revolts my gentle spirit though.) The PR also, of necessity, creates separate lockfiles to pass to different trigger binaries, each with only the components used by the relevant trigger type. This feels like a substantial and complicating change to a simple protocol that has served us well since the early days. I'm also anxious about unintended consequences of deferring the creation of the lockfile.
Anyway putting it up as a draft so that knowledgeable folks can weigh in on better approaches and possible impacts. (And so I can see if it blows up the integration tests!)
cc @kate-goldenring as the app splitting expert!