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

fix(app): sync robot time with app time on connect #6501

Merged
merged 4 commits into from
Sep 10, 2020
Merged

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Sep 8, 2020

Overview

This PR wires the app up to the time endpoints added in #6403. When the app requests to connect over RPC, it will also hit GET /system/time and, if the time is mismatched by more than 60 seconds, the app will send a PUT /system/time request to get things sync'd.

Closes #3872

Changelog

  • feat(app): sync robot time with app time on connect

Review requests

Smoke test:

  1. Ensure your robot is running software that includes the time endpoints added in feat(robot_server): add system/time GET & PUT endpoints #6403
  2. SSH into your robot and mess up the time
    1. systemctl disable systemd-timesyncd
    2. date -s 10:10
  3. Open the Opentrons App
  4. Click the connect toggle or button
    • GET /system/time request goes out
    • PUT /system/time request goes out
    • date in the robot's terminal reports the same time (in UTC) as your computer
  5. Disconnect and connect again
    • GET /system/time request went out
    • PUT /system/time request does not go out because time is already synced

Risk assessment

Medium low. Thoughts + notes:

  • Time is a funny thing to mess with
  • If timesyncd is running and the robot has NTP access, the NTP time will be reasserted quite quickly
    • The app only attempts to sync at connect, so we won't get into an edit war with the system time
  • If the robot is on an older software version, GET /system/time will 404
    • The app will only try to send the PUT if the GET succeeds
    • We could (should?) add systemTime to the health links so we can check capabilities ahead of time
    • Until then, the 404 will show up in the logs like it does when the app tries to fetch calibration data from out of date robots

@mcous mcous added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). labels Sep 8, 2020
@mcous mcous requested review from a team as code owners September 8, 2020 19:51
@mcous mcous requested review from Kadee80 and removed request for a team September 8, 2020 19:51
@mcous mcous requested review from a team, amitlissack and sanni-t and removed request for a team September 8, 2020 19:53
@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (edge@b5a59e0). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #6501   +/-   ##
=======================================
  Coverage        ?   56.56%           
=======================================
  Files           ?     1073           
  Lines           ?    16549           
  Branches        ?        0           
=======================================
  Hits            ?     9361           
  Misses          ?     7188           
  Partials        ?        0           
Impacted Files Coverage Δ
app/src/robot-admin/epic/index.js 100.00% <ø> (ø)
app/src/robot-admin/constants.js 100.00% <100.00%> (ø)
app/src/robot-admin/epic/syncTimeOnConnectEpic.js 100.00% <100.00%> (ø)
app/src/robot-api/constants.js 100.00% <100.00%> (ø)
...reators/atomic/thermocyclerAwaitProfileComplete.js 100.00% <0.00%> (ø)
app/src/pages/Robots/InstrumentSettings.js 0.00% <0.00%> (ø)
...otocol-designer/src/components/ComputingSpinner.js 0.00% <0.00%> (ø)
...ry/src/components/website-navigation/MenuButton.js 0.00% <0.00%> (ø)
...p/src/components/CalibratePanel/CalibrationData.js 100.00% <0.00%> (ø)
...src/components/LabwareDetails/LabwareDetailsBox.js 0.00% <0.00%> (ø)
... and 1067 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5a59e0...cd2e841. Read the comment docs.

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

If the robot is on an older software version, GET /system/time will 404
The app will only try to send the PUT if the GET succeeds
We could (should?) add systemTime to the health links so we can check capabilities ahead of time
Until then, the 404 will show up in the logs like it does when the app tries to fetch calibration data from out of date robots

I'll make a (follow-up) PR to add systemTime to health links now.

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

LGTM!
A robot connection correctly updates time or does nothing, based on robot's time status.
Approved pending that test mentioned by @amitlissack

sanni-t added a commit that referenced this pull request Sep 10, 2020
@mcous mcous changed the title feat(app): sync robot time with app time on connect fix(app): sync robot time with app time on connect Sep 10, 2020
@mcous mcous merged commit 66dc626 into edge Sep 10, 2020
@mcous mcous deleted the app_time-sync branch September 10, 2020 21:40
sanni-t added a commit that referenced this pull request Sep 11, 2020
…e links (#6517)

Addresses #6501
Also added error raising to prohibit setting time on dev server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol run time in app is incorrect if robot's internal clock is wrong
3 participants