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 18 commits into
base: main
Choose a base branch
from
+378 −49
Open

RSDK-9440 Report machine state through GetMachineStatus #4616

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fe92e07
initial impl
benjirewis Dec 10, 2024
bc21b4f
atomic
benjirewis Dec 10, 2024
28b89db
start config watcher goroutine last
benjirewis Dec 11, 2024
a634d19
addrderef -> CopyOnlyPublicFields
benjirewis Dec 11, 2024
f7dd78c
update api dependency and lint
benjirewis Dec 11, 2024
48e6714
more lint sigh
benjirewis Dec 11, 2024
70a5096
basic entrypoint test
benjirewis Dec 11, 2024
a4d585d
reconfigure in goroutine and check for state running in client new
benjirewis Dec 13, 2024
c6374ad
typos
benjirewis Dec 13, 2024
53dde49
client test inject fixes
benjirewis Dec 13, 2024
eaa1285
move out of initializing in setupLocalRobot
benjirewis Dec 16, 2024
e6b8b44
add log lines for debugging 32 bit tests
benjirewis Dec 16, 2024
d2dc450
move running check higher
benjirewis Dec 16, 2024
b932fdf
potentially simpler API
benjirewis Dec 18, 2024
aa1020e
more missing injections
benjirewis Dec 18, 2024
60ab517
fix more tests sigh
benjirewis Dec 18, 2024
4d29ba8
maybe this time
benjirewis Dec 18, 2024
afcfbee
actually use value from options
benjirewis Dec 18, 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
2 changes: 1 addition & 1 deletion cli/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func TestTabularDataByFilterAction(t *testing.T) {
var dataRequested bool
//nolint:deprecated,staticcheck
tabularDataByFilterFunc := func(ctx context.Context, in *datapb.TabularDataByFilterRequest, opts ...grpc.CallOption,
//nolint:deprecated
//nolint:deprecated,staticcheck
Copy link
Member Author

Choose a reason for hiding this comment

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

Lint won't pass locally without this for me.

) (*datapb.TabularDataByFilterResponse, error) {
if dataRequested {
//nolint:deprecated,staticcheck
Expand Down
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