feat(robot_server): add system/time GET & PUT endpoints #6403

sanni-t · 2020-08-21T15:41:03Z

Overview

This PR adds GET & PUT time endpoints to robot server. These will be used by the RunApp to get/ update robot's date & time. This will enable us to update robot's time in absence of an RTC/ internet access.
Addresses #3872

Changelog

added system service to robot_server
added routers, models & errors for the time endpoints in /system
added time component to opentrons/system
added api & robot_server tests

Review requests

Check for code sensibility
Do the tests make sense? Any tests missing?
Upload to a robot & test the endpoints!
- If your robot is ntp synchronized, you should NOT be able to update system time using PUT
- If your robot has not had ntp synchronization since boot, PUT /system/time should successfully set system time
- Do the responses make sense?
- All responses should be in UTC (+00:00 offset)
- Input time value, regardless of its timezone specification, should set the correct UTC time on robot

Risk assessment

Low. The endpoints are not being used by anything right now & the code is isolated.

amitlissack · 2020-08-21T20:05:31Z

robot-server/robot_server/service/system/router.py

+async def get_time() -> time_models.SystemTimeResponse:
+    try:
+        res = await time.get_system_time()
+    except Exception as e:


You should catch/raise a specific exception or remove this.

amitlissack · 2020-08-21T20:05:58Z

robot-server/robot_server/service/system/router.py

+    try:
+        time_dt, res_status = await time.set_system_time(
+            new_time.data.attributes.systemTime)
+    except Exception as e:


Same as above regarding exception

amitlissack · 2020-08-21T20:09:08Z

api/src/opentrons/system/time.py

+    res_dict = {}
+    for line in lines:
+        if line:
+            res_dict[line.split('=')[0]] = line.split('=')[1]


It is wasteful to split the line twice here. I recommend you have a temp var for the result of line.split('=')

amitlissack · 2020-08-21T20:10:36Z

api/src/opentrons/system/time.py

+log = logging.getLogger(__name__)
+
+
+def _str_to_dict(res_str):


I don't see any tests for this function. Is every line guaranteed to have an = in it?

amitlissack · 2020-08-21T20:12:11Z

api/src/opentrons/system/time.py

+
+def _str_to_dict(res_str):
+    lines = res_str.split('\n')
+    res_dict = {}


Perhaps instead of returning a Dict[str, str] you can return a Dataclass?

At least it would be nice to convert "yes/no" values to bools.

Converting the string items to Dataclass objects seemed a bit too cumbersome esp when the objects would be used just once in the code. (But if you think that's not so then I'm open to suggestions on how to do the conversions)
Converted the the dict yes/no items to bools.

amitlissack · 2020-08-21T20:14:35Z

api/src/opentrons/system/time.py

+    return _str_to_dict(out.decode())
+
+
+async def _set_time(time: str,


How is caller to know what the values in the Tuple mean?

amitlissack · 2020-08-21T20:16:49Z

api/src/opentrons/system/time.py

+    :return: Tuple specifying current date read and error message, if any.
+    """
+    status = await _time_status(loop)
+    if status['LocalRTC'] == "yes" or status['NTPSynchronized'] == "yes":


Would be nice if these were True/False instead of magic "yes" string.

sfoster1

It kinda seems like opentrons.system.time should be robot_server.(something).time. None of its functions are called from the rest of the opentrons module, and it isn't really related to robot operation. I know there's other system components in the opentrons package, but that's just because the robot_server package didn't exist when they were written, and I think we should move them as well (not necessarily in this pr though)

sfoster1 · 2020-08-24T16:43:56Z

api/src/opentrons/system/time.py

+    return out.decode(), err.decode()
+
+
+async def get_system_time(loop: asyncio.AbstractEventLoop = None) -> datetime:


This function is going to be called pretty frequently so the app can make sure that the time stays in sync, right? I don't think we should be shelling out for something like that, or that frequently. datetime.datetime.now() should work a lot better with just a single syscall and file access.

No really. That was indeed what we had drafted, but after giving it some thought, we decided to update system time only when user initiates connection to the robot (reason being that the system clock stays correct after it's once set).
Although, that does not take into account any time getters in order to show robot time in the app, if we decide to do that. In which case, you are right, we'll need to switch to datetime.now().

Thought about it a bit more, you're right, even if we won't be calling the function frequently right now, there's no harm in making the switch to datetime.now(). That way we won't have make the change later.

sfoster1 · 2020-08-24T16:45:17Z

api/src/opentrons/system/time.py

+    now = await get_system_time(loop)
+    # TODO: check with team whether to return whole error or
+    #  just an error status
+    return now, err


we can definitely log the error message if it exists at error level (unless that gets too spammy)

sfoster1 · 2020-08-24T16:45:44Z

api/src/opentrons/system/time.py

+        new_time_str = new_time_dt.strftime("%Y-%m-%d %H:%M:%S")
+        log.info(f'Setting time to {new_time_str} UTC')
+        out, err = await _set_time(new_time_str)
+        log.info(f'{out if out else err}')


This might be a little spammy

sfoster1 · 2020-08-24T16:46:28Z

robot-server/robot_server/service/system/models.py

+
+class SystemTimeAttributes(BaseModel):
+    systemTime: datetime
+    setTimeError: Optional[str]


Should we do this or should we use an error response?

Good point. Changed to error response.

sanni-t · 2020-08-25T23:40:50Z

It kinda seems like opentrons.system.time should be robot_server.(something).time. None of its functions are called from the rest of the opentrons module, and it isn't really related to robot operation. I know there's other system components in the opentrons package, but that's just because the robot_server package didn't exist when they were written, and I think we should move them as well (not necessarily in this pr though)

Agreed about moving system components out of opentrons module and into robot_server. Since there's other components that need to be moved too, I'll put up a different PR to move all of them at once.

…me comments -added router tests, added response links -raise error instead of returning error in regular response

codecov · 2020-08-26T17:11:29Z

Codecov Report

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

@@           Coverage Diff           @@
##             edge    #6403   +/-   ##
=======================================
  Coverage        ?   80.25%           
=======================================
  Files           ?      230           
  Lines           ?    19934           
  Branches        ?        0           
=======================================
  Hits            ?    15999           
  Misses          ?     3935           
  Partials        ?        0

Impacted Files	Coverage Δ
robot-server/robot_server/system/time.py	`82.05% <82.05%> (ø)`
robot-server/robot_server/service/system/router.py	`90.47% <90.47%> (ø)`
robot-server/robot_server/service/app.py	`90.47% <100.00%> (ø)`
robot-server/robot_server/service/system/models.py	`100.00% <100.00%> (ø)`
robot-server/robot_server/system/errors.py	`100.00% <100.00%> (ø)`
opentrons/commands/tree.py	`100.00% <0.00%> (ø)`
...ice/session/command_execution/hardware_executor.py	`96.96% <0.00%> (ø)`
...ssion_types/protocol/execution/command_executor.py	`89.13% <0.00%> (ø)`
opentrons/legacy_api/__init__.py	`100.00% <0.00%> (ø)`
opentrons/protocol_api/labware.py	`89.72% <0.00%> (ø)`
... and 225 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 f7da768...da033b9. Read the comment docs.

sfoster1

Some nits - I think some of the server tests should actually be tavern integration tests - but also I really think that opentrons.system.time should really be robot_server.system.time. There's a couple reasons for this:

The point of the robot server is to move the stuff that's unrelated to the OT-2 as a robot and protocol execution system out of the opentrons package. We already pulled out all the HTTP stuff and the RPC protocol stuff; the lines get blurry around the orchestration components that run protocols on the robot, but we're in the process of pulling those out too. Things like management of timesyncd (and for that matter network connectivity) really have no place in the opentrons package conceptually.
Pulling it into robot_server would let you significantly clean up its interface around errors. You can't use the nice server exceptions in the opentrons package because the opentrons package can't rely on robot_server, so your options are to raise some generic exception that will later be caught and reraised, or do this thing where you return the error string. Neither is a great choice - the first is just confusing, and the second makes robot_server (and the service subpackage at that!) have to understand the output of timesyncd, breaking the encapsulation of opentrons.system.time. If the time package is in robot_server, it can do all its own parsing and raise the appropriate exception (if necessary) directly.

sfoster1 · 2020-08-27T18:08:59Z

api/src/opentrons/system/time.py

+                prop, val = line.split('=')
+                res_dict[prop] = val if val not in ['yes', 'no'] \
+                    else val == 'yes'  # Convert yes/no to boolean value
+    except (ValueError, IndexError) as e:


[nit]: the most probably failure case here is that we didn't anticipate a particular kind of output from timedatectl, but the output we did anticipate is the same. given that, should we maybe do this kind of checking on a line-by-line basis and not raise the exception? instead, we can log with a specific tag and look for it on log monitoring.

sfoster1 · 2020-08-27T18:13:55Z

robot-server/robot_server/service/system/router.py

+    sys_time, err = await time.set_system_time(
+        new_time.data.attributes.systemTime)
+    if err:
+        if 'already synchronized with NTP or RTC' in err:


To me, this is the wrong place to do this. It leaks implementation details - the robot server endpoint function now has to care about the exact details of how timesyncd formats error messages.

sfoster1 · 2020-08-27T18:14:46Z

robot-server/tests/service/system/test_router.py

+    assert response.status_code == 500
+
+
+def test_get_system_time(api_client, mock_system_time, response_links):


This probably wants to be a tavern integration test

sfoster1 · 2020-08-27T18:14:56Z

robot-server/tests/service/system/test_router.py

+    assert response.status_code == 200
+
+
+def test_set_with_missing_field(api_client, mock_system_time):


This probably wants to be a tavern integration test

sfoster1 · 2020-08-27T18:15:26Z

robot-server/tests/service/system/test_router.py

+    assert response.status_code == 422
+
+
+def test_set_system_time(api_client, mock_system_time, response_links):


this probably wants to be a tavern integration test, but given the degree of mocking maybe it wouldn't be worth it.

…rors

sanni-t · 2020-09-02T17:50:04Z

@sfoster1 Made all the changes requested. Talked with Amit offline about tavern tests and we decided to only add those tests that don't require mocking the system components.

amitlissack

Looks good to me!

amitlissack · 2020-09-02T19:15:05Z

robot-server/robot_server/service/system/router.py

+async def get_time() -> time_models.SystemTimeResponse:
+    res = await time.get_system_time()
+    return _create_response(res)
+


This warning is not quite accurate. We need to configure tests to recognize coverage of tavern tests. I will make a ticket now.

That's a really cool feature to display it inline though!

amitlissack · 2020-09-02T19:28:07Z

robot-server/robot_server/system/time.py

+                res_dict[prop] = val if val not in ['yes', 'no'] \
+                    else val == 'yes'  # Convert yes/no to boolean value
+            except (ValueError, IndexError) as e:
+                log.error("Error converting timedatectl string: {}".format(e))


I think it's worth logging the actual line contents too.

sfoster1

Looks good to me! Love it being integrated into the robot server

amitlissack reviewed Aug 21, 2020

View reviewed changes

sfoster1 requested changes Aug 24, 2020

View reviewed changes

sanni-t force-pushed the robot-server_add_time_endpoints branch from 037342d to ba52ebc Compare August 25, 2020 23:00

sanni-t changed the title ~~Robot server add time endpoints~~ feat(robot_server): add system/time GET & PUT endpoints Aug 25, 2020

sanni-t marked this pull request as ready for review August 26, 2020 14:59

sanni-t requested review from a team as code owners August 26, 2020 14:59

sanni-t requested a review from mcous August 26, 2020 15:17

-added GET & PUT time endpoints, added tests for system, addressed so…

6526c88

…me comments -added router tests, added response links -raise error instead of returning error in regular response

sanni-t force-pushed the robot-server_add_time_endpoints branch from 2484cbf to 6526c88 Compare August 26, 2020 16:00

add docstring & fix linter error

9b1cc58

sanni-t added api Affects the `api` project ready for review robot server Affects the `robot-server` project labels Aug 26, 2020

sanni-t requested a review from sfoster1 August 26, 2020 17:26

sfoster1 requested changes Aug 27, 2020

View reviewed changes

move api/system/time to robot-server. Tests will fail.

f13c8d5

mcous mentioned this pull request Sep 1, 2020

test(app): refactor robot-admin epic tests to user newer test fixtures #6470

Merged

added tavern tests, updated error response, suppressed str_to_dict er…

a3ae977

…rors

forgot to remove redundant test

fbdad1b

sanni-t requested review from sfoster1 and amitlissack September 2, 2020 19:13

amitlissack approved these changes Sep 2, 2020

View reviewed changes

mcous approved these changes Sep 2, 2020

View reviewed changes

better error message

da033b9

sfoster1 approved these changes Sep 2, 2020

View reviewed changes

sanni-t merged commit c3e5b46 into edge Sep 2, 2020

mcous mentioned this pull request Sep 8, 2020

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

Merged

5 tasks

sanni-t deleted the robot-server_add_time_endpoints branch September 25, 2020 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(robot_server): add system/time GET & PUT endpoints #6403

feat(robot_server): add system/time GET & PUT endpoints #6403

sanni-t commented Aug 21, 2020 •

edited

Loading

amitlissack Aug 21, 2020

amitlissack Aug 21, 2020

amitlissack Aug 21, 2020

amitlissack Aug 21, 2020

amitlissack Aug 21, 2020

sanni-t Aug 26, 2020 •

edited

Loading

amitlissack Aug 21, 2020

amitlissack Aug 21, 2020

sfoster1 left a comment

sfoster1 Aug 24, 2020

sanni-t Aug 24, 2020 •

edited

Loading

sanni-t Aug 24, 2020

sfoster1 Aug 24, 2020

sfoster1 Aug 24, 2020

sfoster1 Aug 24, 2020

sanni-t Aug 25, 2020

sanni-t commented Aug 25, 2020

codecov bot commented Aug 26, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Aug 27, 2020

sfoster1 Aug 27, 2020

sfoster1 Aug 27, 2020

sfoster1 Aug 27, 2020

sfoster1 Aug 27, 2020

sanni-t commented Sep 2, 2020

amitlissack left a comment

amitlissack Sep 2, 2020

sfoster1 Sep 2, 2020

amitlissack Sep 2, 2020

sfoster1 left a comment

		return _str_to_dict(out.decode())


		async def _set_time(time: str,

		return out.decode(), err.decode()


		async def get_system_time(loop: asyncio.AbstractEventLoop = None) -> datetime:

		assert response.status_code == 500


		def test_get_system_time(api_client, mock_system_time, response_links):

		assert response.status_code == 200


		def test_set_with_missing_field(api_client, mock_system_time):

		assert response.status_code == 422


		def test_set_system_time(api_client, mock_system_time, response_links):

feat(robot_server): add system/time GET & PUT endpoints #6403

feat(robot_server): add system/time GET & PUT endpoints #6403

Conversation

sanni-t commented Aug 21, 2020 • edited Loading

Overview

Changelog

Review requests

Risk assessment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t commented Aug 25, 2020

codecov bot commented Aug 26, 2020 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t commented Sep 2, 2020

amitlissack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t commented Aug 21, 2020 •

edited

Loading

sanni-t Aug 26, 2020 •

edited

Loading

sanni-t Aug 24, 2020 •

edited

Loading

codecov bot commented Aug 26, 2020 •

edited

Loading