Dependent contexts #247
Closed
Dependent contexts #247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
test failures are unexpected, let me fix them before we move further with the review. |
ffromani
force-pushed
the
dependent-context
branch
from
104042a
to
628b874
Compare
jaypipes
requested changes
May 25, 2021
jaypipes
requested changes
May 25, 2021
ffromani
force-pushed
the
dependent-context
branch
2 times, most recently
from
7dbe4eb
to
4bbaaa6
Compare
Fix wrong comment, likely due to trivial copy/paste mistake. No code changes. Signed-off-by: Francesco Romani <fromani@redhat.com>
The PR jaypipes#236 fixes a snapshot directory leakage issue, but also highlight a context lifecycle/ownership issue. In general in ghw each package creates its own context, including the cases on which a package is consumed by another, for example `topology` and `pci`. However, in this case, it would make more sense to reuse the context from the parent package, because the two packages are tightly coupled. Before the introduction of the the transparent snapshot support (jaypipes#202), the above mechanism worked, because each context, each time, was consuming over and over again the same parameters (e.g. environment variables); besides some resource waste, this had no negative effect. When introducing snapshots in the picture, repeatedly unpacking the same snapshot to consume the same data is much more wasteful. So it makes sense now to introduce explicitly the concept of dependent context. To move forward and make the ownership more explicit, we add an internal reference count in context. This is used only to track the context setup/teardown process. When contexts are reused, we always have a parent module -which creates and owns the context- which passes its context to a subordinate module, because the parent wants to consume the service offered by the subordinate. The subordinate (possibly further nested, or more than one) should never attempt Setup() and Teardown(). These are responsabilities of the parent, outer module, which owns the context. It's impractical to conditionally call Setup() and Teardown(), so the functions internally use and check the reference count to do the right thing automatically. The following are are idempotent (and safe and quickly return success) - calling multiple times Setup() on a ready context - calling multiple times Teardown() on a un-ready context Signed-off-by: Francesco Romani <fromani@redhat.com>
Add NewWithContext() functions to initialize modules reusing already prepared contexts. This saves resources and, most important, make sure that dependent package have a consistent view with the topmost, coordinating package. IOW, this change is motivated by the need of making the sharing and ownersjip links more evident, rather than resource optimization, which is "just" a nice bonus. Signed-off-by: Francesco Romani <fromani@redhat.com>
the pci package want to use the topology package, and passes its context down to it. However, nontrivial initialization is performed in pci.Info.load(), when snapshots are consumed - most notably in test. Hence, we need to ensure that `topology.WithContext` is called only after context.Setup() is called for the pci context. This is needed because in turn the pci packaged doesn't know (nor it should) if its context is a root (= top level) context, hence if it owns the resources which are passed to the dependent packages. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
dependent-context
branch
2 times, most recently
from
de7ed04
to
8294271
Compare
`WithChroot` and `WithSnapshot` conflicts to each other, and the users can get surprising results if they try to use them at the same time. Now: chroot is expected to be much more popular than snapshots, being the latter useful mostly for tests or offline debug/troubleshoot, but still the code should detect and handle these conflicts. This is made a bit awkward because the current `context` pkg API. Once we are free to break compatibility, we can make the flow more linear. Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
dependent-context
branch
from
8294271
to
59dde1d
Compare
|
sorry for the churn, uploaded a botched version by mistake, should be fixed now. |
jaypipes
reviewed
Jul 1, 2021
| // and return it later, at the earliest possible occasion, in Setup() | ||
| if ctx.SnapshotPath != "" && ctx.Chroot != option.DefaultChroot { | ||
| // The env/client code supplied a value, but we are will overwrite it when unpacking shapshots! | ||
| ctx.err = fmt.Errorf("Conflicting options: chroot %q and snapshot path %q", ctx.Chroot, ctx.SnapshotPath) |
There was a problem hiding this comment.
@fromanirh instead of the error tracking and refcounting, why don't we simply panic() here? I think panic() is entirely logical considering this situation would occur only when the user has explicitly made a mistake in their argument-passing.
There was a problem hiding this comment.
just a note: refcounting is meant to solve a different facet of the problem (handling nested context in such a way we perform the cleanup in the expected order)
In general I prefer to avoid to panic() inside packages, this should always be an application layer decision - not a library layer decision, so I'd really try hard to find another approach if you don't like the admittedly awkward way I'm using here.
Let me look for other options
|
I'll split this PR. The first part is in #278 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR we revisit how we handle context for dependent packages; a good example is the
pcipackage, which wants to use thetopologypackage internally. But there are more, likegpu(usespciandtopology), and in the futuresriovwants as well to usepci(see #230)While I'm happy with this PR and I believe the gains outweights the risk, let's discuss if this direction is a good one or if we want to solve the same problem in a different way.