RSDK-10762 Add test for modular optional dependency cycles #5060
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| @@ -617,7 +617,7 @@ func (m *Module) ReconfigureResource(ctx context.Context, req *pb.ReconfigureRes | |||
| return nil, err | |||
| } | |||
|
|
|||
| m.logger.Debugw("rebuilding", "name", conf.ResourceName()) | |||
| m.logger.Debugw("rebuilding", "name", conf.ResourceName().String()) | |||
There was a problem hiding this comment.
[drive-by] Structure-logging this resource name object resulted in
server.go:453: 2025-06-17T12:37:00.294-0400 DEBUG optional-deps module/module.go:620 rebuilding {"nameError":"PANIC=interface conversion: map[string]interface {} is not fmt.Stringer: missing method String","log_ts":"2025-06-17T16:37:00.294Z"}
Just stringified for now. Was this an issue we were aware of, @dgottlieb ? I forget.
There was a problem hiding this comment.
This is an issue we've seen. I thought this was fixed by either something Cheuk or Josh did. But I think this is due to the map[string]interface getting zap encoded over our log entry proto as a "string" type.
Such that the zap encoding code assumes its a fmt.Stringer here.
There was a problem hiding this comment.
Gotcha. Filed https://viam.atlassian.net/browse/RSDK-11007 and will just String() it here for now.
| if resource.IsMustRebuildError(err) { | ||
| r.Logger().CErrorw( | ||
| ctx, | ||
| "non-modular resource uses weak/optional dependencies but is missing a Reconfigure method", |
| }, | ||
| { | ||
| Name: mocName2.Name, | ||
| API: generic.API, |
There was a problem hiding this comment.
Given we're on a path to removing API -- this isn't a critique, just a thought experiment for further understanding. And this is a pretty meaningless scenario.
So we're seeing that creating mocName2 does not reconfigure mocName (but the client handle on mocName presumably will start working under the hood).
If mocName2 were to change its API to say the motor API, would that cause a reconstruction/reconfigure of mocName? Or would mocName be left with a "stale"[1] client object?
[1] The underlying connection and resource name never changes. But the set of methods that could be called "does not update" when it theory it ought to.
There was a problem hiding this comment.
in the current state, I do think that mocName will get reconfigured if mocName2's API changes. mocName2 will get closed and then re-added, which should prompt mocName to get reconfigured as well. but not super high confidence
There was a problem hiding this comment.
So we're seeing that creating mocName2 does not reconfigure mocName (but the client handle on mocName presumably will start working under the hood).
For the new test I've added, when we add moc2, moc will technically get rebuilt module-side (it has no reconfigure method but is able to reconstruct in response to a ReconfigureResourceRequest) and given a gRPC client to moc2. In its initial construction moc was given no gRPC client (deps was empty when passed to constructor). Similary, when moc is removed from the config, moc2 will get rebuilt to pass in no gRPC client (deps will be empty). So, I don't think any gRPC client "starts" or "stops" working under the hood with this test. Maybe I'm misunderstanding your statement here.
There was a problem hiding this comment.
Got it. So when having moc and adding moc2:
- Construct
moc2with dep=[moc] - Close -> Construct
mocwith dep=[moc2]
And even though moc is recreated since moc2 had a handle, moc2 is not recreated. We depend on moc2s existing client object to be continue to be valid.
There was a problem hiding this comment.
And even though moc is recreated since moc2 had a handle, moc2 is not recreated. We depend on moc2s existing client object to be continue to be valid.
That's exactly right (or is my hope), and I don't think the test even exercises that moc2s existing client object is still valid, so let me add some assertions there. All I test right now is that the client object is still set but not usable.
viambot
added
safe to test
RSDK-10762
Adds a test for modular optional dependency cycles that ensures modular resources can optionally depend upon each other without any
Reconfiguremethods (usingAlwaysRebuild).