Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions robot/web/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -971,14 +971,23 @@ func (svc *webService) Stats() any {
// RestartStatusResponse is the JSON response of the `restart_status` HTTP
// endpoint.
type RestartStatusResponse struct {
// RestartAllowed represents whether this instance of the viam-server can be
// RestartAllowed represents whether this instance of the viamserver can be
// safely restarted.
RestartAllowed bool `json:"restart_allowed"`
// DoesNotHandleNeedsRestart represents whether this instance of the viamserver does
// not check for the need to restart against app itself and, thus, needs agent to do so.
// Newer versions of viamserver (>= v0.9x.0) will report true for this value, while
// older versions won't report it at all, and agent should let viamserver handle
// NeedsRestart logic.
DoesNotHandleNeedsRestart bool `json:"does_not_handle_needs_restart,omitempty"`
}

// Handles the `/restart_status` endpoint.
func (svc *webService) handleRestartStatus(w http.ResponseWriter, r *http.Request) {
response := RestartStatusResponse{RestartAllowed: svc.r.RestartAllowed()}
response := RestartStatusResponse{
RestartAllowed: svc.r.RestartAllowed(),
DoesNotHandleNeedsRestart: true,
}

w.Header().Set("Content-Type", "application/json")
// Only log errors from encoding here. A failure to encode should never
Expand Down
10 changes: 10 additions & 0 deletions utils/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,16 @@ const (

// GetImagesInStreamServerEnvVar is the environment variable that enables the GetImages feature flag in stream server.
GetImagesInStreamServerEnvVar = "VIAM_GET_IMAGES_IN_STREAM_SERVER"

// ViamAgentHandlesNeedsRestartChecking is the environment variable that viam-agent will
// set before starting viam-server to indicate that agent is a new enough version to
// have its own background loop that runs NeedsRestart against app.viam.com to determine
// if the system needs a restart. MUST be kept in line with the equivalent value in the
// agent repo.
//
// TODO(RSDK-12057): Remove sensitivity to this environment variable once we fully
// remove all NeedsRestart checking logic from viam-server.
ViamAgentHandlesNeedsRestartChecking = "VIAM_AGENT_HANDLES_NEEDS_RESTART_CHECKING"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now continue to handle checking NeedsRestart in a background goroutine in the server if VIAM_AGENT_HANDLES_NEEDS_RESTART_CHECKING is unset (newer versions of agent will set it unconditionally when launching viam-server). It was determined offline that we do want to support old-agent/new-server setups for a while, and eventually completely remove needs restart checking functionality from the rdk with RSDK-12057.

Thanks to @jmatth for the env var idea 🎉 .

)

// EnvTrueValues contains strings that we interpret as boolean true in env vars.
Expand Down
3 changes: 2 additions & 1 deletion web/cmd/droid/droid.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ var logger = logging.NewDebugLogger("droid-entrypoint")

// DroidStopHook used by android harness to stop the RDK.
func DroidStopHook() { //nolint:revive
server.ForceRestart = true
// We have no users of this droid code, so we no longer support this method.
logger.Error("DroidStopHook is no longer supported")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abe-winter we're not actually removing needs restart checking functionality from viam-server yet, but we will in RSDK-12057. So I don't get confused later, I'm still going to remove the ForceRestart flag and its setting above now if that SGTY.

}

// MainEntry is called by our android app to start the RDK.
Expand Down
5 changes: 4 additions & 1 deletion web/server/entrypoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,10 @@ func (s *robotServer) serveWeb(ctx context.Context, cfg *config.Config) (err err
// This functionality is tested in `TestLogPropagation` in `local_robot_test.go`.
config.UpdateLoggerRegistryFromConfig(s.registry, fullProcessedConfig, s.logger)

if fullProcessedConfig.Cloud != nil {
// Only start cloud restart checker if cloud config is non-nil, and viam-agent is not
// handling restart checking for us (relevant environment variable is unset).
if fullProcessedConfig.Cloud != nil && os.Getenv(rutils.ViamAgentHandlesNeedsRestartChecking) == "" {
s.logger.CInfo(ctx, "Agent does not handle checking needs restart functionality; will handle in server")
cloudRestartCheckerActive = make(chan struct{})
utils.PanicCapturingGo(func() {
defer close(cloudRestartCheckerActive)
Expand Down
6 changes: 0 additions & 6 deletions web/server/restart_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,7 @@ type needsRestartCheckerGRPC struct {
client rpc.ClientConn
}

// ForceRestart lets random other parts of the app request an exit.
var ForceRestart bool

func (c *needsRestartCheckerGRPC) needsRestart(ctx context.Context) (bool, time.Duration, error) {
if ForceRestart {
return true, minNeedsRestartCheckInterval, nil
}
service := apppb.NewRobotServiceClient(c.client)
res, err := service.NeedsRestart(ctx, &apppb.NeedsRestartRequest{Id: c.cfg.ID})
if err != nil {
Expand Down
Loading