[RSDK-8291] Use Global Connection to App in Config Watcher #4773
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
removing myself to get another set of eyes on these changes and added @benjirewis |
config/reader.go
Outdated
| ) (*Config, error) { | ||
| buf, err := envsubst.ReadFile(filePath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return FromReader(ctx, filePath, bytes.NewReader(buf), logger) | ||
| return FromReader(ctx, filePath, bytes.NewReader(buf), logger, conn) | ||
| } | ||
|
|
||
| // ReadLocalConfig reads a config from the given file but does not fetch any config from the remote servers. | ||
| func ReadLocalConfig( |
There was a problem hiding this comment.
ReadLocalConfig should never need a connection, right? can just pass an empty connection into fromReader by default
There was a problem hiding this comment.
Mm yeah this is smart it'll be more clear that the conn isn't used
| @@ -392,6 +371,7 @@ func fromReader( | |||
| r io.Reader, | |||
| logger logging.Logger, | |||
| shouldReadFromCloud bool, | |||
| conn rpc.ClientConn, | |||
There was a problem hiding this comment.
can you take a look and see if we can replace shouldReadFromCloud with conn entirely? so the existence of a connection itself is the indicator whether a we should be reading from cloud or not
totally ok if not a straight swap, just curious
viambot
added
safe to test
viambot
added
safe to test
config/watcher.go
Outdated
robot/impl/local_robot.go
Outdated
There was a problem hiding this comment.
Why is this file a "core change"? Is it because it's part of the "passing down" of the global app conn to config watching? Whereas other files are test changes?
There was a problem hiding this comment.
ya sorry should've been more clear, you're right it's just a "non-test" change
There was a problem hiding this comment.
refactor mentioned earlier
| @@ -45,7 +46,8 @@ func writeTempConfig(cfg *config.Config) (string, error) { | |||
| // make a fake robot with a vision service. | |||
| func buildRobotWithFakeCamera(logger logging.Logger) (robot.Robot, error) { | |||
| // add a fake camera to the config | |||
| cfg, err := config.Read(context.Background(), artifact.MustPath("components/camera/transformpipeline/vision.json"), logger) | |||
| cfg, err := config.Read(context.Background(), artifact.MustPath("components/camera/transformpipeline/vision.json"), logger, | |||
| &grpc.AppConn{}) | |||
There was a problem hiding this comment.
[nit] You might've used this pattern elsewhere, but I often see nil passed instead of an "empty" struct when we want to communicate to another function that we have no special object for that function to use and it should create its own. Hypothetically, I'd have a small preference for passing nil here and not &grpc.AppConn.
There was a problem hiding this comment.
Done! I like this I for some reason glossed over this as an option when trying to figure how to make it apparent that the code path followed by the tests with an empty connection struct didn't actually need/use the conn so I like this
robot/impl/local_robot.go
Outdated
There was a problem hiding this comment.
Why is this file a "core change"? Is it because it's part of the "passing down" of the global app conn to config watching? Whereas other files are test changes?
utils/contextutils/context.go
Outdated
| @@ -23,8 +29,20 @@ const ( | |||
| // TimeReceivedMetadataKey is optional metadata in the gRPC response header that correlates | |||
| // to the time right after the point cloud was captured. | |||
| TimeReceivedMetadataKey = "viam-time-received" | |||
|
|
|||
| readTimeout = 5 * time.Second | |||
There was a problem hiding this comment.
Now that we've moved these three timeout variables to context utils to avoid the import cycle, I think we'll need a) a small sentence describing what "read" they're governing (config reading) and b) to rename the variables to make it more clear what "read" they're governing.
utils/contextutils/context.go
Outdated
| ) | ||
|
|
||
| // ViamDotDir is the directory for Viam's cached files. | ||
| var ViamDotDir string |
There was a problem hiding this comment.
The config read timeout vars' presence in this package is acceptable IMO, but I don't love ViamDotDir being here. It's only barely related to context creation; do you have any ideas for a better location @cheukt ?
There was a problem hiding this comment.
utils/env.go maybe? but ya, not an easy var to find a location for
There was a problem hiding this comment.
Let's do utils/env.go if that SGTY @bashar-515 , I like that better. Thanks, @cheukt .
web/server/entrypoint.go
Outdated
| @@ -542,7 +542,7 @@ func (s *robotServer) serveWeb(ctx context.Context, cfg *config.Config) (err err | |||
| }() | |||
|
|
|||
| // watch for and deliver changes to the robot | |||
| watcher, err := config.NewWatcher(ctx, cfg, s.logger.Sublogger("config")) | |||
| watcher, err := config.NewWatcher(ctx, cfg, s.logger.Sublogger("config"), s.conn) // TODO(bashar) | |||
There was a problem hiding this comment.
TODO(bashar)
Is this not correct as it is?
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
| @@ -173,9 +173,11 @@ func RunServer(ctx context.Context, args []string, _ logging.Logger) (err error) | |||
| defer pprof.StopCPUProfile() | |||
| } | |||
|
|
|||
| var appConn rpc.ClientConn | |||
There was a problem hiding this comment.
To clarify, appConn remains nil until a connection can be made to app yes? And, you're saying that all code paths you've seen do not try to call *appConn or appConn.foo without verifying that it's non-nil first?
This PR propagates the global connection to App that's instantiated in viam-server's entrypoint down to the function actually used by the config watcher to retrieve a new config. Before, this function would create and manage its own connection to App. Now, it receives a preexisting connection as a function parameter. The changes made here resulted in many modified function signatures, so lots of tests had to be edited to pass a connection even if not ultimately used (i.e., a config is being retrieved locally). In these test files, I simply pass in an empty
&grpc.AppConn{}. This suffices because, again, the connection does not get used in the code path used by the test. In the few instances that it is used (usually when a mock server is spun up to deliver the config), I use the proper AppConn constructor and defer its closing.For what's its worth, I've gone through the PR and marked the actual core functionality changes (i.e., not the test changes that are a mere side effect).
Testing RDK successfully reconfigures after config update following various combinations of connecting to/disconnecting from the internet.