Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-9440 Report machine state through GetMachineStatus #4616

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
+465 −119
Open

RSDK-9440 Report machine state through GetMachineStatus #4616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
42db31c
initial impl
benjirewis Dec 10, 2024
c1e816c
atomic
benjirewis Dec 10, 2024
2175276
start config watcher goroutine last
benjirewis Dec 11, 2024
bebeadf
addrderef -> CopyOnlyPublicFields
benjirewis Dec 11, 2024
5485043
update api dependency and lint
benjirewis Dec 11, 2024
8e4e8a3
more lint sigh
benjirewis Dec 11, 2024
aa5808f
basic entrypoint test
benjirewis Dec 11, 2024
9e80ade
reconfigure in goroutine and check for state running in client new
benjirewis Dec 13, 2024
19bce88
typos
benjirewis Dec 13, 2024
441ed47
client test inject fixes
benjirewis Dec 13, 2024
96a1043
move out of initializing in setupLocalRobot
benjirewis Dec 16, 2024
0de679d
add log lines for debugging 32 bit tests
benjirewis Dec 16, 2024
2e0011d
move running check higher
benjirewis Dec 16, 2024
881bde0
potentially simpler API
benjirewis Dec 18, 2024
041f191
more missing injections
benjirewis Dec 18, 2024
39db863
fix more tests sigh
benjirewis Dec 18, 2024
a0810a6
maybe this time
benjirewis Dec 18, 2024
a2d8bd2
actually use value from options
benjirewis Dec 18, 2024
9e68fd6
make configWatcher a method instead of an anonymous function
benjirewis Dec 19, 2024
f7e60f5
put back lints
benjirewis Dec 19, 2024
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
39 changes: 39 additions & 0 deletions robot/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,35 @@ func New(ctx context.Context, address string, clientLogger logging.ZapCompatible
return nil, err
}

// If running in a testing environment, wait for machine to report a state of
// running. We often establish connections in tests and expected resources to
// be immediately available once the web service has started; resources will
// not be available when the machine is still initializing.
//
// It is expected that golang SDK users will handle lack of resource
// availability due to the machine being in an initializing state themselves.
if testing.Testing() {
for {
if ctx.Err() != nil {
return nil, multierr.Combine(ctx.Err(), rc.conn.Close())
}

mStatus, err := rc.MachineStatus(ctx)
if err != nil {
// Allow for MachineStatus to not be injected/implemented in some tests.
if status.Code(err) == codes.Unimplemented {
break
}
return nil, multierr.Combine(err, rc.conn.Close())
}

if mStatus.State == robot.StateRunning {
break
}
time.Sleep(50 * time.Millisecond)
}
}

// refresh once to hydrate the robot.
if err := rc.Refresh(ctx); err != nil {
return nil, multierr.Combine(err, rc.conn.Close())
Expand Down Expand Up @@ -1117,6 +1146,16 @@ func (rc *RobotClient) MachineStatus(ctx context.Context) (robot.MachineStatus,
mStatus.Resources = append(mStatus.Resources, resStatus)
}

switch resp.State {
case pb.GetMachineStatusResponse_STATE_UNSPECIFIED:
rc.logger.CError(ctx, "received unspecified machine state")
mStatus.State = robot.StateUnknown
case pb.GetMachineStatusResponse_STATE_INITIALIZING:
mStatus.State = robot.StateInitializing
case pb.GetMachineStatusResponse_STATE_RUNNING:
mStatus.State = robot.StateRunning
}

return mStatus, nil
}

Expand Down
35 changes: 22 additions & 13 deletions robot/client/client_session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,11 @@ func TestClientSessionOptions(t *testing.T) {
return &dummyEcho{Named: arbName.AsNamed()}, nil
},
ResourceRPCAPIsFunc: func() []resource.RPCAPI { return nil },
LoggerFunc: func() logging.Logger { return logger },
SessMgr: sessMgr,
MachineStatusFunc: func(_ context.Context) (robot.MachineStatus, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I inject an almost identical MachineStatusFunc all over the place in client_session_test.go and client_test.go. client.New now calls MachineStatus when in a testing environment to make sure the robot is in a robot.StateRunning and therefore capable of receiving resource API calls.

I thought about placing this functionality in inject/robot.go, but opted to just put it in every robot inject, since we currently do that for ResourceNamesFunc and ResourceByNameFunc.

return robot.MachineStatus{State: robot.StateRunning}, nil
},
LoggerFunc: func() logging.Logger { return logger },
SessMgr: sessMgr,
}

svc := web.New(injectRobot, logger)
Expand All @@ -129,13 +132,11 @@ func TestClientSessionOptions(t *testing.T) {
Disable: true,
})))
}
roboClient, err := client.New(ctx, addr, logger, opts...)
Copy link
Member Author

Choose a reason for hiding this comment

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

In three of the client session tests, I had to move the instantiation of the robot client to be below the injection of the session manager. client.New now calls MachineStatus in testing environments, which is not exempted from session creation. So, the injected robot will try to Start a session from its session manager, and end up panicking if StartFunc is not defined. I did not leave a comment in-line explaining this.

I'd like to meet offline at some point to walk through TestClientSessionOptions, in particular, and add some documentation to what it's testing.

test.That(t, err, test.ShouldBeNil)

injectRobot.Mu.Lock()
injectRobot.MachineStatusFunc = func(ctx context.Context) (robot.MachineStatus, error) {
session.SafetyMonitorResourceName(ctx, someTargetName1)
return robot.MachineStatus{}, nil
return robot.MachineStatus{State: robot.StateRunning}, nil
}
injectRobot.Mu.Unlock()

Expand Down Expand Up @@ -179,6 +180,8 @@ func TestClientSessionOptions(t *testing.T) {
}
sessMgr.mu.Unlock()

roboClient, err := client.New(ctx, addr, logger, opts...)
test.That(t, err, test.ShouldBeNil)
resp, err := roboClient.MachineStatus(nextCtx)
test.That(t, err, test.ShouldBeNil)
test.That(t, resp, test.ShouldNotBeNil)
Expand Down Expand Up @@ -289,6 +292,9 @@ func TestClientSessionExpiration(t *testing.T) {
ResourceByNameFunc: func(name resource.Name) (resource.Resource, error) {
return &dummyEcho1, nil
},
MachineStatusFunc: func(_ context.Context) (robot.MachineStatus, error) {
return robot.MachineStatus{State: robot.StateRunning}, nil
},
ResourceRPCAPIsFunc: func() []resource.RPCAPI { return nil },
LoggerFunc: func() logging.Logger { return logger },
SessMgr: sessMgr,
Expand All @@ -306,8 +312,6 @@ func TestClientSessionExpiration(t *testing.T) {
Disable: true,
})))
}
roboClient, err := client.New(ctx, addr, logger, opts...)
test.That(t, err, test.ShouldBeNil)

injectRobot.Mu.Lock()
var capSessID uuid.UUID
Expand All @@ -317,7 +321,7 @@ func TestClientSessionExpiration(t *testing.T) {
panic("expected session")
}
capSessID = sess.ID()
return robot.MachineStatus{}, nil
return robot.MachineStatus{State: robot.StateRunning}, nil
}
injectRobot.Mu.Unlock()

Expand Down Expand Up @@ -372,6 +376,8 @@ func TestClientSessionExpiration(t *testing.T) {
}
sessMgr.mu.Unlock()

roboClient, err := client.New(ctx, addr, logger, opts...)
test.That(t, err, test.ShouldBeNil)
resp, err := roboClient.MachineStatus(nextCtx)
test.That(t, err, test.ShouldBeNil)
test.That(t, resp, test.ShouldNotBeNil)
Expand Down Expand Up @@ -480,8 +486,11 @@ func TestClientSessionResume(t *testing.T) {
injectRobot := &inject.Robot{
ResourceNamesFunc: func() []resource.Name { return []resource.Name{} },
ResourceRPCAPIsFunc: func() []resource.RPCAPI { return nil },
LoggerFunc: func() logging.Logger { return logger },
SessMgr: sessMgr,
MachineStatusFunc: func(_ context.Context) (robot.MachineStatus, error) {
return robot.MachineStatus{State: robot.StateRunning}, nil
},
LoggerFunc: func() logging.Logger { return logger },
SessMgr: sessMgr,
}

svc := web.New(injectRobot, logger)
Expand All @@ -496,8 +505,6 @@ func TestClientSessionResume(t *testing.T) {
Disable: true,
})))
}
roboClient, err := client.New(ctx, addr, logger, opts...)
test.That(t, err, test.ShouldBeNil)

var capMu sync.Mutex
var startCalled int
Expand Down Expand Up @@ -536,10 +543,12 @@ func TestClientSessionResume(t *testing.T) {
panic("expected session")
}
capSessID = sess.ID()
return robot.MachineStatus{}, nil
return robot.MachineStatus{State: robot.StateRunning}, nil
}
injectRobot.Mu.Unlock()

roboClient, err := client.New(ctx, addr, logger, opts...)
test.That(t, err, test.ShouldBeNil)
resp, err := roboClient.MachineStatus(nextCtx)
test.That(t, err, test.ShouldBeNil)
test.That(t, resp, test.ShouldNotBeNil)
Expand Down
Loading
Loading