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-7903 Include unhealthy remotes in resource list with status #4273

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

maximpertsov
Copy link
Member

@maximpertsov maximpertsov commented Aug 8, 2024

Summary

Keep remote resources in the ResourceNames and MachineStatus APIs when their remote gets disconnected. In the latter API, mark such resources with a state indicate that they are disconnected.

Details

Robot clients now fetch and cache resource statuses via the GetMachineStatus API instead of resource names via ResourceNames API.

Open questions

  • Should we add a new state for disconnected nodes? (e.g. NodeStateDisconnected), or is it sufficient to reuse the unhealthy state with a corresponding error (e.g. NodeStateUnhealthy[reason:disconnected])?
    • FWIW, I slightly prefer a new state given the proposed implementation, since a healthy but disconnected remote resource can appear as "disconnected" or "ready" depending on whether a client calls MachineStatus on the local or remote machine, respectively. Having a specialized state that can only appear for remote resources feels like the less confusing option in this case.
  • Shall we bubble up remote update errors? (see this comment and this comment) - this will make the PR much noisier but might be the correct thing to do.

Depends on

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Aug 8, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from aeeba0d to 36dbd45 Compare August 8, 2024 03:39
@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 Aug 8, 2024
@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 Aug 9, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from 73cae61 to 4153c0c Compare August 9, 2024 21:08
@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 Aug 9, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from fb91e32 to c30937d Compare August 9, 2024 21:17
@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 Aug 9, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from c30937d to e2243e1 Compare August 20, 2024 20:12
@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 Aug 20, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from bbc5e4c to e2243e1 Compare August 20, 2024 22:07
@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 Aug 20, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from e2243e1 to fd0784b Compare August 21, 2024 19:36
@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 Aug 21, 2024
@maximpertsov maximpertsov force-pushed the RSDK-7903-unhealthy-remotes-replace-names branch from 05ec75f to a0fc8d8 Compare September 24, 2024 20:06
@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 Sep 24, 2024
Copy link
Member Author

@maximpertsov maximpertsov Sep 24, 2024

Choose a reason for hiding this comment

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

An alternative approach to the changes in this file is to define a new state (e.g. NodeStateDisconnected) - see the "Open questions" section in the PR description for more details and see the revert of this commit for a general idea of how this would look.

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 high-level change here is that a robot client now caches the results of MachineStatus rather than just ResourceNames. As a result, multiple tests that result on injecting a mock function for ResourceNames now need to mock MachineStatus.

@maximpertsov maximpertsov marked this pull request as ready for review September 24, 2024 20:11
@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 Sep 24, 2024
Comment on lines +39 to +41

// NodeStateDisconnected denotes a resource is disconnected.
NodeStateDisconnected
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative to defining a new state would be to mark disconnected resource nodes as "unhealthy" with an appropriate error message. See "Open questions" in the description of this PR for more details and revert this commit to see how this alternative approach would look.

@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 Sep 24, 2024
@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 Sep 24, 2024
@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 Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants