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

Jump to bottom

RSDK-3329, RSDK-4070 - Do not restart modules that crash before ready and respect resource configuration timeout #2645

Merged
merged 7 commits into from
Jul 19, 2023
Merged

RSDK-3329, RSDK-4070 - Do not restart modules that crash before ready and respect resource configuration timeout #2645

merged 7 commits into from
Jul 19, 2023

Conversation

benjirewis
Copy link
Member

@benjirewis benjirewis commented Jul 17, 2023 •
edited
Loading

RSDK-3329
RSDK-4070

Only restarts modules in the OUE handler when the handles field has been set on the module (the module responded to a ready request and returned its handler map). Changes hard-coded 30 second listen/ready timeouts to be 60 second defaults that can overridden with VIAM_RESOURCE_CONFIGURATION_TIMEOUT variable). Adds a test that immediate crash of a module does not result in an attempted restart.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jul 17, 2023
Otterverse
Otterverse requested changes Jul 17, 2023
View reviewed changes
Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

Looks good for the most part, but has a race condition to fix.

module/modmanager/manager.go Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2023
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2023
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2023
Otterverse
Otterverse approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

LGTM

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 18, 2023
@benjirewis benjirewis changed the title RSDK-3329 Do not restart modules that crash before being ready Do not restart modules that crash before ready and respect resource configuration timeout Jul 18, 2023
benjirewis
benjirewis commented Jul 18, 2023
View reviewed changes
utils/env.go
// be set to override defaultResourceConfigurationTimeout as the duration
// that resources and modules are allowed to (re)configure and startup
// respectively.
ResourceConfigurationTimeoutEnvVar = "VIAM_RESOURCE_CONFIGURATION_TIMEOUT"
Copy link
Member Author

Choose a reason for hiding this comment

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

@stuqdog consolidated a bit of this env var logic btw. LMK if anything seems off.

@benjirewis benjirewis mentioned this pull request Jul 18, 2023
Closed
cheukt
cheukt requested changes Jul 18, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

looks generally good to me, but would like to see another test or two

nit: would name PR RSDK-3329, RSDK-4070 - XXX

module/modmanager/manager_test.go Show resolved Hide resolved
@benjirewis benjirewis changed the title Do not restart modules that crash before ready and respect resource configuration timeout RSDK-3329, RSDK-4070 - Do not restart modules that crash before ready and respect resource configuration timeout Jul 18, 2023
Otterverse
Otterverse requested changes Jul 18, 2023
View reviewed changes
Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

Several minor nits from me, but @cheukt caught a big issue that needs to be addressed.

module/modmanager/manager.go Outdated Show resolved Hide resolved
resource/errors.go Outdated Show resolved Hide resolved
utils/env.go Outdated Show resolved Hide resolved
module/modmanager/manager_test.go Outdated Show resolved Hide resolved
module/modmanager/manager_test.go Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 18, 2023
Otterverse
Otterverse approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

LGTM!

cheukt
cheukt approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM!

module/modmanager/manager.go Outdated Show resolved Hide resolved
module/modmanager/manager.go Outdated Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 19, 2023
@github-actions
Copy link
Contributor

Code Coverage

Code Coverage
Package Line Rate Delta Health
go.viam.com/rdk/components/arm 59% 0.00% ➖
go.viam.com/rdk/components/arm/fake 27% 0.00% ❌
go.viam.com/rdk/components/arm/universalrobots 41% 0.00% ❌
go.viam.com/rdk/components/arm/wrapper 19% 0.00% ❌
go.viam.com/rdk/components/arm/xarm 22% 0.00% ❌
go.viam.com/rdk/components/arm/yahboom 6% 0.00% ❌
go.viam.com/rdk/components/audioinput 43% 0.00% ❌
go.viam.com/rdk/components/base 60% 0.00% ➖
go.viam.com/rdk/components/base/agilex 61% 0.00% ➖
go.viam.com/rdk/components/base/kinematicbase 41% 0.00% ❌
go.viam.com/rdk/components/base/wheeled 80% 0.00% ✅
go.viam.com/rdk/components/board 60% 0.00% ➖
go.viam.com/rdk/components/board/customlinux 76% 0.00% ✅
go.viam.com/rdk/components/board/fake 40% 0.00% ❌
go.viam.com/rdk/components/board/genericlinux 9% 0.00% ❌
go.viam.com/rdk/components/board/numato 19% 0.00% ❌
go.viam.com/rdk/components/board/pi/impl 11% 0.00% ❌
go.viam.com/rdk/components/camera 56% 0.00% ➖
go.viam.com/rdk/components/camera/align 58% 0.00% ➖
go.viam.com/rdk/components/camera/fake 74% 0.00% ✅
go.viam.com/rdk/components/camera/ffmpeg 82% 0.00% ✅
go.viam.com/rdk/components/camera/replaypcd 90% 0.00% ✅
go.viam.com/rdk/components/camera/rtsp 52% 0.00% ➖
go.viam.com/rdk/components/camera/transformpipeline 77% 0.00% ✅
go.viam.com/rdk/components/camera/ultrasonic 61% 0.00% ➖
go.viam.com/rdk/components/camera/videosource 34% 0.00% ❌
go.viam.com/rdk/components/encoder 57% 0.00% ➖
go.viam.com/rdk/components/encoder/ams 63% 0.00% ➖
go.viam.com/rdk/components/encoder/fake 83% 0.00% ✅
go.viam.com/rdk/components/encoder/incremental 80% 0.00% ✅
go.viam.com/rdk/components/encoder/single 86% 0.00% ✅
go.viam.com/rdk/components/gantry 60% 0.00% ➖
go.viam.com/rdk/components/gantry/multiaxis 82% 0.00% ✅
go.viam.com/rdk/components/gantry/singleaxis 81% -1.67% ✅
go.viam.com/rdk/components/generic 79% 0.00% ✅
go.viam.com/rdk/components/gripper 69% 0.00% ➖
go.viam.com/rdk/components/input 88% 0.00% ✅
go.viam.com/rdk/components/input/fake 93% 0.00% ✅
go.viam.com/rdk/components/input/gpio 85% 0.00% ✅
go.viam.com/rdk/components/motor 71% 0.00% ✅
go.viam.com/rdk/components/motor/dimensionengineering 67% 0.00% ➖
go.viam.com/rdk/components/motor/dmc4000 70% 0.00% ➖
go.viam.com/rdk/components/motor/fake 55% 0.00% ➖
go.viam.com/rdk/components/motor/gpio 64% -1.09% ➖
go.viam.com/rdk/components/motor/gpiostepper 56% -0.68% ➖
go.viam.com/rdk/components/motor/tmcstepper 53% 0.00% ➖
go.viam.com/rdk/components/motor/ulnstepper 53% 0.00% ➖
go.viam.com/rdk/components/movementsensor 69% 0.00% ➖
go.viam.com/rdk/components/movementsensor/adxl345 75% 0.00% ✅
go.viam.com/rdk/components/movementsensor/cameramono 42% 0.00% ❌
go.viam.com/rdk/components/movementsensor/gpsnmea 55% -3.19% ➖
go.viam.com/rdk/components/movementsensor/gpsrtk 24% 0.00% ❌
go.viam.com/rdk/components/movementsensor/merged 91% 0.00% ✅
go.viam.com/rdk/components/movementsensor/mpu6050 84% 0.00% ✅
go.viam.com/rdk/components/posetracker 71% 0.00% ✅
go.viam.com/rdk/components/sensor 51% 0.00% ➖
go.viam.com/rdk/components/sensor/ultrasonic 38% 0.00% ❌
go.viam.com/rdk/components/servo 62% 0.00% ➖
go.viam.com/rdk/components/servo/gpio 72% 0.00% ✅
go.viam.com/rdk/config 79% 0.00% ✅
go.viam.com/rdk/control 57% 0.00% ➖
go.viam.com/rdk/data 77% 0.00% ✅
go.viam.com/rdk/examples/customresources/demos/remoteserver 0% 0.00% ❌
go.viam.com/rdk/grpc 25% 0.00% ❌
go.viam.com/rdk/internal/cloud 100% 0.00% ✅
go.viam.com/rdk/ml 67% 0.00% ➖
go.viam.com/rdk/ml/inference 71% 0.00% ✅
go.viam.com/rdk/module 75% 0.00% ✅
go.viam.com/rdk/module/modmanager 82% +2.07% ✅
go.viam.com/rdk/motionplan 78% +0.11% ✅
go.viam.com/rdk/motionplan/tpspace 79% 0.00% ✅
go.viam.com/rdk/operation 82% 0.00% ✅
go.viam.com/rdk/pointcloud 66% -0.07% ➖
go.viam.com/rdk/protoutils 49% 0.00% ❌
go.viam.com/rdk/referenceframe 65% 0.00% ➖
go.viam.com/rdk/resource 76% +0.08% ✅
go.viam.com/rdk/rimage 55% 0.00% ➖
go.viam.com/rdk/rimage/depthadapter 94% 0.00% ✅
go.viam.com/rdk/rimage/transform 71% 0.00% ✅
go.viam.com/rdk/rimage/transform/cmd/extrinsic_calibration 67% 0.00% ➖
go.viam.com/rdk/robot 85% -0.87% ✅
go.viam.com/rdk/robot/client 82% 0.00% ✅
go.viam.com/rdk/robot/framesystem 36% 0.00% ❌
go.viam.com/rdk/robot/impl 83% +0.54% ✅
go.viam.com/rdk/robot/packages 80% 0.00% ✅
go.viam.com/rdk/robot/server 55% 0.00% ➖
go.viam.com/rdk/robot/web 65% 0.00% ➖
go.viam.com/rdk/robot/web/stream 87% 0.00% ✅
go.viam.com/rdk/services/baseremotecontrol 50% 0.00% ➖
go.viam.com/rdk/services/baseremotecontrol/builtin 81% 0.00% ✅
go.viam.com/rdk/services/datamanager 65% 0.00% ➖
go.viam.com/rdk/services/datamanager/builtin 90% 0.00% ✅
go.viam.com/rdk/services/datamanager/datacapture 73% 0.00% ✅
go.viam.com/rdk/services/datamanager/datasync 0% 0.00% ❌
go.viam.com/rdk/services/mlmodel 83% 0.00% ✅
go.viam.com/rdk/services/mlmodel/tflitecpu 82% 0.00% ✅
go.viam.com/rdk/services/motion 47% 0.00% ❌
go.viam.com/rdk/services/motion/builtin 83% 0.00% ✅
go.viam.com/rdk/services/navigation 54% 0.00% ➖
go.viam.com/rdk/services/navigation/builtin 57% 0.00% ➖
go.viam.com/rdk/services/sensors 81% 0.00% ✅
go.viam.com/rdk/services/sensors/builtin 97% 0.00% ✅
go.viam.com/rdk/services/shell 11% 0.00% ❌
go.viam.com/rdk/services/slam 79% 0.00% ✅
go.viam.com/rdk/services/slam/fake 82% 0.00% ✅
go.viam.com/rdk/services/vision 35% 0.00% ❌
go.viam.com/rdk/services/vision/colordetector 56% 0.00% ➖
go.viam.com/rdk/services/vision/detectionstosegments 67% 0.00% ➖
go.viam.com/rdk/services/vision/mlvision 67% 0.00% ➖
go.viam.com/rdk/services/vision/obstacledistance 77% 0.00% ✅
go.viam.com/rdk/services/vision/radiusclustering 59% 0.00% ➖
go.viam.com/rdk/session 94% 0.00% ✅
go.viam.com/rdk/spatialmath 82% 0.00% ✅
go.viam.com/rdk/utils 68% -0.62% ➖
go.viam.com/rdk/utils/contextutils 38% 0.00% ❌
go.viam.com/rdk/vision 32% 0.00% ❌
go.viam.com/rdk/vision/delaunay 87% 0.00% ✅
go.viam.com/rdk/vision/keypoints 92% 0.00% ✅
go.viam.com/rdk/vision/objectdetection 69% 0.00% ➖
go.viam.com/rdk/vision/odometry 60% 0.00% ➖
go.viam.com/rdk/vision/odometry/cmd 0% 0.00% ❌
go.viam.com/rdk/vision/segmentation 49% 0.00% ❌
go.viam.com/rdk/web/server 26% 0.00% ❌
Summary 64% (21612 / 33905) -0.04% ➖

@benjirewis benjirewis merged commit d554bca into viamrobotics:main Jul 19, 2023
@benjirewis benjirewis deleted the better-restart-logic branch July 19, 2023 17:21
EshaMaharishi pushed a commit that referenced this pull request Jul 25, 2023
@benjirewis @EshaMaharishi
RSDK-3329, RSDK-4070 - Do not restart modules that crash before ready…
31753fb 
… and respect resource configuration timeout (#2645)
bazile-clyde pushed a commit to bazile-clyde/rdk that referenced this pull request Jan 5, 2024
@benjirewis
RSDK-3329, RSDK-4070 - Do not restart modules that crash before ready…
f06e333 
… and respect resource configuration timeout (viamrobotics#2645)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheukt cheukt cheukt approved these changes

@Otterverse Otterverse Otterverse approved these changes

Assignees
No one assigned
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@benjirewis @Otterverse @cheukt @viambot