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

feat: Health check and health command #73

Merged
merged 2 commits into from
Dec 3, 2019
Merged

feat: Health check and health command #73

merged 2 commits into from
Dec 3, 2019

Conversation

mprey
Copy link
Contributor

@mprey mprey commented Dec 3, 2019

Added:

  • health check via TCP ping
  • health command given a VM name
  • health status in slim vms

Notes:

  • Should we have health status checks disabled for slim vms but enabled via an optional flag?

lib/commands/vms.js Outdated Show resolved Hide resolved
@ssmirr
Copy link
Contributor

ssmirr commented Dec 3, 2019

I encountered two issues when running the PR:

  1. slim vms only shows one of my vms.
    I think I know the reason for this. The problem is in using async/await inside reduce(). This blog post has a good explanation for what the issue is and how to fix it. I also had another comment about this above ^ (in the code review).

  2. the tcpPing(sshInfo) result is an object like below, so !!result will be true even when it is failing to ping:

    {
      address: '127.0.0.1',
      port: 80,
      attempts: 5,
      avg: NaN,
      max: undefined,
      min: undefined,
      results: [
        { seq: 0, time: undefined, err: [Error] },
        { seq: 1, time: undefined, err: [Error] },
        { seq: 2, time: undefined, err: [Error] },
        { seq: 3, time: undefined, err: [Error] },
        { seq: 4, time: undefined, err: [Error] }
      ]
    }

I think it's reasonable to make health check run with an optional flag for slim vms.

@mprey
Copy link
Contributor Author

mprey commented Dec 3, 2019

Fixed the two issues in most recent commit

@ssmirr ssmirr self-requested a review December 3, 2019 17:23
Copy link
Contributor

@ssmirr ssmirr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and great work @mprey! LGTM.

@ssmirr ssmirr merged commit 0835157 into ottomatica:master Dec 3, 2019
@ssmirr
Copy link
Contributor

ssmirr commented Dec 3, 2019

I thought doing ping for all the vms would be slow, but since it is pretty fast, we changed --health to run as default. 37a89e1

@mprey mprey deleted the feat/health branch December 4, 2019 04:32
@mprey mprey mentioned this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants