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.

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-8765 - Add CloudMetadata to MachineStatus #4637

Merged
merged 32 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
07a1e6a
first pass
Kschappacher Nov 19, 2024
184adfa
Merge remote-tracking branch 'upstream/main' into RSDK-8409-add-cloud…
Kschappacher Nov 19, 2024
ffa052c
fix remote name bug
Kschappacher Nov 20, 2024
379e48a
lint and fix tests
Kschappacher Nov 20, 2024
7047e40
Merge branch 'main' into cloud-metadata
cheukt Dec 3, 2024
ca92ca6
move proto conversion, add test
cheukt Dec 3, 2024
5e3c416
fix test
cheukt Dec 3, 2024
2d2663d
some more updates
cheukt Dec 4, 2024
0014486
Merge branch 'main' into cloud-metadata
cheukt Dec 9, 2024
757b405
add simple test
cheukt Dec 9, 2024
96f3bd4
add option
cheukt Dec 10, 2024
8847686
Merge branch 'main' into cloud-metadata
cheukt Dec 11, 2024
4506964
some test update
cheukt Dec 11, 2024
234338e
slight change
cheukt Dec 11, 2024
804c587
Merge branch 'main' into cloud-metadata
cheukt Dec 13, 2024
c3bf357
more
cheukt Dec 16, 2024
c7dc40a
rename node.ResourceStatus to Status
cheukt Dec 16, 2024
3034f19
+1
cheukt Dec 16, 2024
9d26f97
flyby
cheukt Dec 16, 2024
5fadd59
add helper
cheukt Dec 16, 2024
db7c921
add remote tests
cheukt Dec 16, 2024
369a8a8
two remotes test
cheukt Dec 16, 2024
edfa12e
last test
cheukt Dec 16, 2024
47ed7b7
lint
cheukt Dec 16, 2024
8d9a2f8
fix
cheukt Dec 17, 2024
ac22c89
finish up tests
cheukt Dec 18, 2024
ecffe7d
lint + remove unnecessary helper
cheukt Dec 18, 2024
c7e0db8
Merge branch 'main' into cloud-metadata
cheukt Dec 18, 2024
1e00b67
Merge branch 'main' into cloud-metadata
cheukt Dec 23, 2024
ac42a26
pr feedback
cheukt Dec 23, 2024
3d62334
Apply suggestions from code review
cheukt Dec 23, 2024
e8f1c78
more docs
cheukt Dec 23, 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
27 changes: 27 additions & 0 deletions protoutils/messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/golang/geo/r3"
protov1 "github.com/golang/protobuf/proto" //nolint:staticcheck
commonpb "go.viam.com/api/common/v1"
robotpb "go.viam.com/api/robot/v1"
"go.viam.com/utils/protoutils"
"google.golang.org/grpc"
"google.golang.org/protobuf/proto"
Expand All @@ -16,10 +17,36 @@ import (
"google.golang.org/protobuf/types/known/structpb"
"google.golang.org/protobuf/types/known/wrapperspb"

"go.viam.com/rdk/cloud"
"go.viam.com/rdk/resource"
"go.viam.com/rdk/spatialmath"
)

// MetadataFromProto converts a proto GetCloudMetadataResponse to Metadata.
func MetadataFromProto(pbMetadata *robotpb.GetCloudMetadataResponse) cloud.Metadata {
if pbMetadata == nil {
return cloud.Metadata{}
}
return cloud.Metadata{
MachinePartID: pbMetadata.MachinePartId,
MachineID: pbMetadata.MachineId,
PrimaryOrgID: pbMetadata.PrimaryOrgId,
LocationID: pbMetadata.LocationId,
}
}

// MetadataToProto converts a Metadata its proto counterpart.
func MetadataToProto(metadata cloud.Metadata) *robotpb.GetCloudMetadataResponse {
return &robotpb.GetCloudMetadataResponse{
// TODO: RSDK-7181 remove RobotPartId
RobotPartId: metadata.MachinePartID, // Deprecated: Duplicates MachinePartId,
MachinePartId: metadata.MachinePartID,
MachineId: metadata.MachineID,
PrimaryOrgId: metadata.PrimaryOrgID,
LocationId: metadata.LocationID,
}
}

// ResourceNameToProto converts a resource.Name to its proto counterpart.
func ResourceNameToProto(name resource.Name) *commonpb.ResourceName {
return &commonpb.ResourceName{
Expand Down
64 changes: 64 additions & 0 deletions protoutils/messages_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,74 @@ import (
"strconv"
"testing"

robotpb "go.viam.com/api/robot/v1"
"go.viam.com/test"
"google.golang.org/protobuf/types/known/wrapperspb"

"go.viam.com/rdk/cloud"
)

func TestMetadataFromProto(t *testing.T) {
expected := cloud.Metadata{}
observed := MetadataFromProto(nil)
test.That(t, observed, test.ShouldResemble, expected)

partID := "123"
machineID := "abc"
orgID := "456"
locID := "def"

samePartID := &robotpb.GetCloudMetadataResponse{
RobotPartId: partID,
MachinePartId: partID,
MachineId: machineID,
PrimaryOrgId: orgID,
LocationId: locID,
}
expected = cloud.Metadata{
MachinePartID: partID,
MachineID: machineID,
PrimaryOrgID: orgID,
LocationID: locID,
}
observed = MetadataFromProto(samePartID)
test.That(t, observed, test.ShouldResemble, expected)

robotPartID := "789"
diffPartID := &robotpb.GetCloudMetadataResponse{
RobotPartId: robotPartID,
MachinePartId: partID,
MachineId: machineID,
PrimaryOrgId: orgID,
LocationId: locID,
}
observed = MetadataFromProto(diffPartID)
test.That(t, observed, test.ShouldResemble, expected)
}

func TestMetadataToProto(t *testing.T) {
partID := "123"
machineID := "abc"
orgID := "456"
locID := "def"

metadata := cloud.Metadata{
MachinePartID: partID,
MachineID: machineID,
PrimaryOrgID: orgID,
LocationID: locID,
}
expected := &robotpb.GetCloudMetadataResponse{
RobotPartId: partID,
MachinePartId: partID,
MachineId: machineID,
PrimaryOrgId: orgID,
LocationId: locID,
}
observed := MetadataToProto(metadata)
test.That(t, observed, test.ShouldResembleProto, expected)
}

func TestStringToAnyPB(t *testing.T) {
anyVal, err := ConvertStringToAnyPB("12")
test.That(t, err, test.ShouldBeNil)
Expand Down
27 changes: 10 additions & 17 deletions resource/graph_node.go
Original file line number Diff line number Diff line change
Expand Up @@ -555,37 +555,30 @@ func (w *GraphNode) transitionTo(state NodeState) {
w.transitionedAt = time.Now()
}

// ResourceStatus returns the current [Status].
func (w *GraphNode) ResourceStatus() Status {
// Status returns the current [NodeStatus].
func (w *GraphNode) Status() NodeStatus {
w.mu.RLock()
defer w.mu.RUnlock()

return w.resourceStatus()
return w.status()
}

func (w *GraphNode) resourceStatus() Status {
var resName Name
Copy link
Member Author

Choose a reason for hiding this comment

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

the issue here is that test objects are not very flexible and there is not a good way to pass in the correct node name to the resource (especially if going across "remotes")

Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate on why this won't match the resource name? Is it only in the case of remote resources? Just want an example of a mismatch so I better understand in which cases the original config.Name or current.Name() are incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

mock remote resources in tests - especially how I've opted to not actually spin up real servers, the "remote" resources are real resources and no way to get the names to be accurate.

if w.current == nil {
resName = w.config.ResourceName()
} else {
resName = w.current.Name()
}

func (w *GraphNode) status() NodeStatus {
err := w.lastErr
logger := w.Logger()

// check invariants between state and error
switch {
case w.state == NodeStateUnhealthy && w.lastErr == nil:
logger.Warnw("an unhealthy node doesn't have an error", "resource", resName)
logger.Warnw("an unhealthy node doesn't have an error")
case w.state == NodeStateReady && w.lastErr != nil:
logger.Warnw("a ready node still has an error", "resource", resName, "error", err)
logger.Warnw("a ready node still has an error", "error", err)
// do not return leftover error in status if the node is ready
err = nil
}

return Status{
Name: resName,
// TODO (RSDK-9550): Node should have the correct notion of its name
return NodeStatus{
State: w.state,
LastUpdated: w.transitionedAt,
Revision: w.revision,
Expand Down Expand Up @@ -623,8 +616,8 @@ func (w *GraphNode) Stats() any {
return ret
}

// Status encapsulates a resource name along with state transition metadata.
type Status struct {
// NodeStatus encapsulates a resource name along with state transition metadata.
type NodeStatus struct {
Name Name
Copy link
Member Author

Choose a reason for hiding this comment

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

could also move Name out of NodeStatus and into resource.Status, but keeping it there for now since I think it would be good to have the name come from the node and not the graph

Copy link
Member Author

Choose a reason for hiding this comment

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

https://viam.atlassian.net/browse/RSDK-9550 is the ticket, marked it as low since it's low-impact

State NodeState
LastUpdated time.Time
Expand Down
12 changes: 6 additions & 6 deletions resource/graph_node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func TestUninitializedLifecycle(t *testing.T) {
expectedState := resource.NodeStateUnconfigured
test.That(t, node.State(), test.ShouldEqual, expectedState)

status := node.ResourceStatus()
status := node.Status()
test.That(t, status.State, test.ShouldResemble, expectedState)

lifecycleTest(t, node, []string(nil))
Expand Down Expand Up @@ -71,8 +71,8 @@ func TestUnconfiguredLifecycle(t *testing.T) {
expectedState := resource.NodeStateConfiguring
test.That(t, node.State(), test.ShouldEqual, expectedState)

status := node.ResourceStatus()
test.That(t, status.Name.Name, test.ShouldEqual, "foo")
status := node.Status()
test.That(t, status.Name.Name, test.ShouldEqual, "")
test.That(t, status.State, test.ShouldResemble, expectedState)

lifecycleTest(t, node, initialDeps)
Expand Down Expand Up @@ -102,8 +102,8 @@ func TestConfiguredLifecycle(t *testing.T) {
expectedState := resource.NodeStateReady
test.That(t, node.State(), test.ShouldEqual, expectedState)

status := node.ResourceStatus()
test.That(t, status.Name, test.ShouldResemble, resName)
status := node.Status()
test.That(t, status.Name, test.ShouldResemble, resource.Name{})
test.That(t, status.State, test.ShouldResemble, resource.NodeStateReady)

lifecycleTest(t, node, []string(nil))
Expand Down Expand Up @@ -149,7 +149,7 @@ func lifecycleTest(t *testing.T, node *resource.GraphNode, initialDeps []string)
// Attempt to change status to [NodeStateUnhealthy]
ourErr = errors.New("whoops")
node.LogAndSetLastError(ourErr)
status := node.ResourceStatus()
status := node.Status()
// Ensure that error is set and node stays in [NodeStateUnhealthy]
// since state transition [NodeStateUnhealthy] -> [NodeStateRemoving] is blocked
test.That(t, status.Error.Error(), test.ShouldContainSubstring, "whoops")
Expand Down
7 changes: 7 additions & 0 deletions resource/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/jhump/protoreflect/desc"
"github.com/pkg/errors"

"go.viam.com/rdk/cloud"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/utils"
)
Expand Down Expand Up @@ -304,3 +305,9 @@ func NewCloseOnlyResource(name Name, closeFunc func(ctx context.Context) error)
func (r *closeOnlyResource) Close(ctx context.Context) error {
return r.closeFunc(ctx)
}

// Status is a combination of a resources node status and the cloudMetadata associated with that resource.
type Status struct {
NodeStatus
CloudMetadata cloud.Metadata
}
12 changes: 8 additions & 4 deletions resource/resource_graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -695,13 +695,17 @@ func (g *Graph) MarkReachability(node Name, reachable bool) error {
}

// Status returns a slice of all graph node statuses.
func (g *Graph) Status() []Status {
func (g *Graph) Status() []NodeStatus {
g.mu.Lock()
defer g.mu.Unlock()

var result []Status
for _, node := range g.nodes {
result = append(result, node.ResourceStatus())
var result []NodeStatus
for name, node := range g.nodes {
// TODO (RSDK-9550): Node should have the correct notion of its name
// but they don't, so fill it in here
status := node.Status()
status.Name = name
result = append(result, status)
}

return result
Expand Down
18 changes: 8 additions & 10 deletions robot/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1014,17 +1014,12 @@ func (rc *RobotClient) Log(ctx context.Context, log zapcore.Entry, fields []zap.
//
// metadata, err := machine.CloudMetadata(ctx.Background())
func (rc *RobotClient) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
cloudMD := cloud.Metadata{}
req := &pb.GetCloudMetadataRequest{}
resp, err := rc.client.GetCloudMetadata(ctx, req)
if err != nil {
return cloudMD, err
return cloud.Metadata{}, err
}
cloudMD.PrimaryOrgID = resp.PrimaryOrgId
cloudMD.LocationID = resp.LocationId
cloudMD.MachineID = resp.MachineId
cloudMD.MachinePartID = resp.MachinePartId
return cloudMD, nil
return rprotoutils.MetadataFromProto(resp), nil
}

// RestartModule restarts a running module by name or ID.
Expand Down Expand Up @@ -1090,9 +1085,12 @@ func (rc *RobotClient) MachineStatus(ctx context.Context) (robot.MachineStatus,
mStatus.Resources = make([]resource.Status, 0, len(resp.Resources))
for _, pbResStatus := range resp.Resources {
resStatus := resource.Status{
Name: rprotoutils.ResourceNameFromProto(pbResStatus.Name),
LastUpdated: pbResStatus.LastUpdated.AsTime(),
Revision: pbResStatus.Revision,
NodeStatus: resource.NodeStatus{
Name: rprotoutils.ResourceNameFromProto(pbResStatus.Name),
LastUpdated: pbResStatus.LastUpdated.AsTime(),
Revision: pbResStatus.Revision,
},
CloudMetadata: rprotoutils.MetadataFromProto(pbResStatus.CloudMetadata),
}

switch pbResStatus.State {
Expand Down
66 changes: 50 additions & 16 deletions robot/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1984,8 +1984,10 @@ func TestMachineStatus(t *testing.T) {
Config: config.Revision{Revision: "rev1"},
Resources: []resource.Status{
{
Name: arm.Named("badArm"),
Revision: "rev0",
NodeStatus: resource.NodeStatus{
Name: arm.Named("badArm"),
Revision: "rev0",
},
},
},
},
Expand All @@ -1997,9 +1999,11 @@ func TestMachineStatus(t *testing.T) {
Config: config.Revision{Revision: "rev1"},
Resources: []resource.Status{
{
Name: arm.Named("goodArm"),
State: resource.NodeStateConfiguring,
Revision: "rev1",
NodeStatus: resource.NodeStatus{
Name: arm.Named("goodArm"),
State: resource.NodeStateConfiguring,
Revision: "rev1",
},
},
},
},
Expand All @@ -2011,17 +2015,23 @@ func TestMachineStatus(t *testing.T) {
Config: config.Revision{Revision: "rev1"},
Resources: []resource.Status{
{
Name: arm.Named("goodArm"),
State: resource.NodeStateConfiguring,
Revision: "rev1",
NodeStatus: resource.NodeStatus{
Name: arm.Named("goodArm"),
State: resource.NodeStateConfiguring,
Revision: "rev1",
},
},
{
Name: arm.Named("badArm"),
Revision: "rev0",
NodeStatus: resource.NodeStatus{
Name: arm.Named("badArm"),
Revision: "rev0",
},
},
{
Name: arm.Named("anotherBadArm"),
Revision: "rev-1",
NodeStatus: resource.NodeStatus{
Name: arm.Named("anotherBadArm"),
Revision: "rev-1",
},
},
},
},
Expand All @@ -2033,10 +2043,34 @@ func TestMachineStatus(t *testing.T) {
Config: config.Revision{Revision: "rev1"},
Resources: []resource.Status{
{
Name: arm.Named("brokenArm"),
State: resource.NodeStateUnhealthy,
Error: errors.New("bad configuration"),
Revision: "rev1",
NodeStatus: resource.NodeStatus{
Name: arm.Named("brokenArm"),
State: resource.NodeStateUnhealthy,
Error: errors.New("bad configuration"),
Revision: "rev1",
},
},
},
},
0,
},
{
"cloud metadata",
robot.MachineStatus{
Config: config.Revision{Revision: "rev1"},
Resources: []resource.Status{
{
NodeStatus: resource.NodeStatus{
Name: arm.Named("arm1"),
State: resource.NodeStateReady,
Revision: "rev1",
},
CloudMetadata: cloud.Metadata{
MachinePartID: "123",
MachineID: "456",
PrimaryOrgID: "789",
LocationID: "abc",
},
},
},
},
Expand Down
Loading
Loading