-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(robot-server): ensure robot-server starts after mosquitto #14729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once this has been tested with the buildroot changes.
@@ -4,7 +4,7 @@ Description=Opentrons Robot HTTP Server | |||
|
|||
Requires=nginx.service | |||
After=nginx.service | |||
|
|||
After=mosquitto.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to add Wants=mosquitto.service
?
If we omit that, and we only have After=mosquitto.service
, the sequencing will be correct any time systemd starts both services, but systemd won't necessarily know that it has to start both services. Concretely: if you're in a state where nothing is started, and then you do systemctl start opentrons-robot-server
, it won't also start mosquitto.service
.
I'm not sure if this will matter in practice, since mosquitto.service
is started by multi-user.target
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't feel strongly about it, but I could see that behavior as being potentially confusing. As long as mosquitto is guaranteed to start on boot (which I believe is always the case), then I think we're good.
If you feel like it should be there, I don't mind adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do also add Wants
it's good practice. systemd is a general-purpose constraint solver and so if another, tighter constraint is active this wants will have no effect, but it's always a good idea to express intent in unit files like this.
@@ -4,7 +4,7 @@ Description=Opentrons Robot HTTP Server | |||
|
|||
Requires=nginx.service | |||
After=nginx.service | |||
|
|||
After=mosquitto.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do also add Wants
it's good practice. systemd is a general-purpose constraint solver and so if another, tighter constraint is active this wants will have no effect, but it's always a good idea to express intent in unit files like this.
Closes EXEC-351
Overview
@SyntaxColoring brought up the valid point that we should ensure mosquitto starts before the robot server, since the robot-server assumes this is true. This doesn't appear to be an issue in practice (so far), but let's fix it.
Risk assessment
low