machine: Add a new command `dvc machine status` #6649

karajan1001 · 2021-09-20T00:41:47Z

add a new command dvc machine status
add a unit test for the cli call.
add a unit test for the shown result.

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

fix#6482 1. add a new command `dvc machien status` 2. add a unit test for the cli call. 3. add a unit test for the shown result.

dvc/command/machine.py

Co-authored-by: Saugat Pachhai <[email protected]>

dberenbaum · 2021-09-20T19:49:34Z

@karajan1001 How much extra work would it take to add an asciinema or other video inside of these PRs as an example? It might also make it easier to demo the feature.

pmrowla · 2021-09-21T02:43:34Z

One thing we should consider is sanitizing auth/private data related fields in the CLI commands. In terraform show they remove things like SSH private keys and startup scripts from their CLI output (and just display something like <sensitive data removed>). They only include those fields when it's in an output format intended for machine/programmatic consumption (i.e. terraform show -json)

We should probably do the same thing and not display those fields by default (and include them with --show-json).

I'm not sure if there's a straightforward way to get the list of what fields terraform considers sensitive from their CLI, or if it's hard-coded in the provider settings somewhere (@iterative/cml team may know?)

casperdcl · 2021-09-21T11:44:09Z

@pmrowla we hardcode a whitelist in CML https://github.com/iterative/cml/blob/2e9a21ddb4b4eb1261a75ba72f89b98b1972d063/bin/cml/runner.js#L191-L212 and also mark sensitive things in TPI.go (e.g. https://github.com/iterative/terraform-provider-iterative/blob/732d0919577867d40b7ff635dd0667caf19d3cb9/iterative/resource_machine.go#L106) for terraform show

karajan1001 · 2021-09-28T08:44:36Z

@pmrowla we hardcode a whitelist in CML https://github.com/iterative/cml/blob/2e9a21ddb4b4eb1261a75ba72f89b98b1972d063/bin/cml/runner.js#L191-L212 and also mark sensitive things in TPI.go (e.g. https://github.com/iterative/terraform-provider-iterative/blob/732d0919577867d40b7ff635dd0667caf19d3cb9/iterative/resource_machine.go#L106) for terraform show

Any way to read this white list? Looks like I need to hardcode it first and sync manually first.

karajan1001 · 2021-09-28T08:59:32Z

@karajan1001 How much extra work would it take to add an asciinema or other video inside of these PRs as an example? It might also make it easier to demo the feature.

tests/func/machine/__init__.py

casperdcl

Any way to read this white list? Looks like I need to hardcode it first and sync manually first.

SENSITIVE_FIELDS = {"ssh_private", "startup_script"} seems enough

dvc/machine/__init__.py

Co-authored-by: Casper da Costa-Luis <[email protected]>

for more information, see https://pre-commit.ci

karajan1001 · 2021-09-30T03:43:46Z

dberenbaum · 2021-09-30T13:35:31Z

UI comments:

It's unclear whether this is intended to show a single machine/instance or all of them. It looks like it's set up to show multiple instances per machine if they exist, which sounds nice for the future. In that case, why not be able to show instances of all machines at once?
Terraform output doesn't conform with our UI guidelines. If we want to have raw terraform output as an option, I think we also need more typical DVC output to summarize the relevant info in a table or something that looks cleaner and easier to parse. We also probably need to consider which fields are actually relevant. For example, the ssh key info is probably too much detail, especially since DVC has a built-in dvc machine ssh command.

cc @skshetry

casperdcl · 2021-09-30T14:24:35Z

@dberenbaum I think suppressing terraform's CLI output may be an upstream feature request related to https://github.com/iterative/tpi/blob/c7259a8fea023797058deaf487700645df5fe210/tpi/__init__.py#L60

dberenbaum · 2021-09-30T15:10:41Z

dvc/command/machine.py

@@ -143,6 +143,22 @@ def run(self):
        return 0


+class CmdMachineStatus(CmdBase):
+    SHOWN_FIELD = ["name", "cloud", "instance_hdd_size", "instance_ip"]


Can we include instance_launch_time?

dberenbaum · 2021-09-30T15:16:38Z

dvc/command/machine.py

+
+        all_status = self.repo.machine.status(self.args.name)
+        for i, status in enumerate(all_status, start=1):
+            ui.write(f"instance_num_{i}:")


Could this be a table, more like dvc stage list, to make it easier to parse? We are trying to have default outputs that are both easy to read and parse if possible.

dberenbaum · 2021-09-30T15:19:49Z

@dberenbaum I think suppressing terraform's CLI output may be an upstream feature request related to https://github.com/iterative/tpi/blob/c7259a8fea023797058deaf487700645df5fe210/tpi/__init__.py#L60

Thanks, @casperdcl! Should I open an issue?

Also, is there a "local" option supported for tpi cloud? @karajan1001 @pmrowla Would that be helpful for integration tests and development generally?

casperdcl · 2021-09-30T17:06:20Z

@dberenbaum were you intending to quote #6649 (comment) instead?

dberenbaum · 2021-09-30T17:11:05Z

@dberenbaum were you intending to quote #6649 (comment) instead?

Yup, thanks. I'll edit it to reference the correct quote.

pmrowla · 2021-10-01T02:20:55Z

@dberenbaum Regarding the terraform CLI output, we originally decided to leave it enabled by default (and not suppress it) since there is no other way for us to reflect status while terraform ... commands are running (and we expect them to be slow).

Since we can't hook into the terraform process itself to setup any kind of status callbacks, the only other option on the DVC UI side would be to just display something like a spinner (we can't provide any progressbar-like estimate of when the terraform command will complete).

skshetry · 2021-10-01T02:53:10Z

Another way might be to only show the tail of the output limited to n lines, just like the progress bar (on how they clear up the screen), and dump whatever we have on failure.

dberenbaum · 2021-10-01T17:20:20Z

We don't want to fully suppress the output for commands where progress is important (like dvc machine add), but for dvc machine status, is it needed?

On the other hand, I can imagine some people might want the option to see the raw terraform output if they are familiar with that or want more granular info, so it might not be bad as an option (or maybe include in verbose logging?). If we do show it, it would be helpful to somehow tag it or separate it from output controlled by DVC.

pmrowla · 2021-10-19T08:37:29Z

Going to merge this as-is since it's been on hold for a bit. Suppressing the terraform CLI output should be handled upstream in TPI, and tabular formatting for the status output (and decisions on additional fields to display) can be handled in a follow-up PR

dberenbaum · 2021-10-19T17:24:31Z

What about making name optional so all machines can be shown at once? I'll open a separate issue for this since it's already merged.

machine: Add a new command dvc machine status

e3004fc

fix#6482 1. add a new command `dvc machien status` 2. add a unit test for the cli call. 3. add a unit test for the shown result.

karajan1001 requested a review from a team as a code owner September 20, 2021 00:41

karajan1001 requested a review from pared September 20, 2021 00:41

skshetry reviewed Sep 20, 2021

View reviewed changes

dvc/command/machine.py Outdated Show resolved Hide resolved

skshetry reviewed Sep 20, 2021

View reviewed changes

dvc/command/machine.py Outdated Show resolved Hide resolved

karajan1001 and others added 3 commits September 20, 2021 17:01

Update dvc/command/machine.py

db743f4

Co-authored-by: Saugat Pachhai <[email protected]>

Update dvc/command/machine.py

a4e8d83

Co-authored-by: Saugat Pachhai <[email protected]>

Update machine.py

6ee958f

pared approved these changes Sep 20, 2021

View reviewed changes

karajan1001 mentioned this pull request Sep 21, 2021

Add rename method to TerraformBackend iterative/tpi#12

Closed

pmrowla self-requested a review September 21, 2021 02:43

Remove sensitive fields

d65fdab

casperdcl reviewed Sep 28, 2021

View reviewed changes

tests/func/machine/__init__.py Outdated Show resolved Hide resolved

casperdcl approved these changes Sep 28, 2021

View reviewed changes

casperdcl reviewed Sep 28, 2021

View reviewed changes

dvc/machine/__init__.py Outdated Show resolved Hide resolved

karajan1001 and others added 3 commits September 29, 2021 16:26

Update dvc/machine/__init__.py

74dccc4

Co-authored-by: Casper da Costa-Luis <[email protected]>

Update tests/func/machine/__init__.py

df5d636

Co-authored-by: Casper da Costa-Luis <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

0f2f621

for more information, see https://pre-commit.ci

karajan1001 linked an issue Sep 29, 2021 that may be closed by this pull request

machine: list status of created instances #6482

Closed

1 task

Use white list to limit shown fields

6569b47

casperdcl approved these changes Sep 30, 2021

View reviewed changes

dberenbaum reviewed Sep 30, 2021

View reviewed changes

dberenbaum mentioned this pull request Oct 1, 2021

executor: setup and configure executors #6263

Closed

9 tasks

pmrowla merged commit 966dc8a into iterative:master Oct 19, 2021

karajan1001 deleted the fix6482 branch October 19, 2021 08:50

dberenbaum mentioned this pull request Oct 19, 2021

machine: show all machines in status #6838

Closed

efiop added the feature is a feature label Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machine: Add a new command `dvc machine status` #6649

machine: Add a new command `dvc machine status` #6649

karajan1001 commented Sep 20, 2021 •

edited by pmrowla

Loading

dberenbaum commented Sep 20, 2021

pmrowla commented Sep 21, 2021

casperdcl commented Sep 21, 2021 •

edited

Loading

karajan1001 commented Sep 28, 2021

karajan1001 commented Sep 28, 2021

casperdcl left a comment •

edited

Loading

karajan1001 commented Sep 30, 2021

dberenbaum commented Sep 30, 2021 •

edited

Loading

casperdcl commented Sep 30, 2021

dberenbaum Sep 30, 2021

karajan1001 Oct 1, 2021

dberenbaum Sep 30, 2021

dberenbaum commented Sep 30, 2021 •

edited

Loading

casperdcl commented Sep 30, 2021

dberenbaum commented Sep 30, 2021

pmrowla commented Oct 1, 2021 •

edited

Loading

skshetry commented Oct 1, 2021

dberenbaum commented Oct 1, 2021

pmrowla commented Oct 19, 2021

dberenbaum commented Oct 19, 2021

machine: Add a new command dvc machine status #6649

machine: Add a new command dvc machine status #6649

Conversation

karajan1001 commented Sep 20, 2021 • edited by pmrowla Loading

dberenbaum commented Sep 20, 2021

pmrowla commented Sep 21, 2021

casperdcl commented Sep 21, 2021 • edited Loading

karajan1001 commented Sep 28, 2021

karajan1001 commented Sep 28, 2021

casperdcl left a comment • edited Loading

Choose a reason for hiding this comment

karajan1001 commented Sep 30, 2021

dberenbaum commented Sep 30, 2021 • edited Loading

casperdcl commented Sep 30, 2021

dberenbaum Sep 30, 2021

Choose a reason for hiding this comment

karajan1001 Oct 1, 2021

Choose a reason for hiding this comment

dberenbaum Sep 30, 2021

Choose a reason for hiding this comment

dberenbaum commented Sep 30, 2021 • edited Loading

casperdcl commented Sep 30, 2021

dberenbaum commented Sep 30, 2021

pmrowla commented Oct 1, 2021 • edited Loading

skshetry commented Oct 1, 2021

dberenbaum commented Oct 1, 2021

pmrowla commented Oct 19, 2021

dberenbaum commented Oct 19, 2021

machine: Add a new command `dvc machine status` #6649

machine: Add a new command `dvc machine status` #6649

karajan1001 commented Sep 20, 2021 •

edited by pmrowla

Loading

casperdcl commented Sep 21, 2021 •

edited

Loading

casperdcl left a comment •

edited

Loading

dberenbaum commented Sep 30, 2021 •

edited

Loading

dberenbaum commented Sep 30, 2021 •

edited

Loading

pmrowla commented Oct 1, 2021 •

edited

Loading