Repository: iterative/dvc

Issue:
machine: show all machines in `status`
Make the `name` argument optional in `dvc machines status` so that all machines may be shown at once. Extracted from https://github.com/iterative/dvc/pull/6649#issuecomment-946940057.

Discussion:
jorgeorpinel: Your suggestions make sense @dberenbaum. I'd need to see a sample UI interaction of this command for specifics on argument naming, etc. (browsed GH and Notion and couldn't easily find the latest info.)

BTW @karajan1001 this doesn't need formal documentation yet but please try to keep a basic doc updated guys, is https://github.com/iterative/dvc/wiki/Remote-executors the main one you've been writing? Thanks

> We need a separate issue to review this for all commands.

There's this epic https://github.com/iterative/dvc/issues/5392
Other older related ones: https://github.com/iterative/dvc/issues/4172 (may apply here directly), https://github.com/iterative/dvc/issues/3642
karajan1001: @jorgeorpinel , @dberenbaum 
Any suggestions on this PR? The tabulation part had already been in another one.
karajan1001: > Are the `--global/system/project/local` flags used in this command? They are shown in the help message but not sure they make sense or do anything for this command.

They are useful in `--show-origin` mode for now.  But in a normal mode, because I melt all levels of config together, it gives no difference in results. Btw, there are still some discussions on how we deal with the machine config on multi-levels, one machine config might be spread in all of them. And makes it hard to maintain. Should we restrict that one machine config can only be in one level (cross level deduplication) and maybe make it only a local config ( can't be in `system/global` level)
dberenbaum: > They are useful in `--show-origin` mode for now.

Is this an option for `dvc machine status`? And how would `--global/system/project/local` be useful with that option? Wouldn't that be more useful without those options so you could see where each machine config came from?

> Btw, there are still some discussions on how we deal with the machine config on multi-levels, one machine config might be spread in all of them. And makes it hard to maintain. Should we restrict that one machine config can only be in one level (cross level deduplication) and maybe make it only a local config ( can't be in `system/global` level)

Do you have an example? My assumption would be that it would follow the same priority as other config options: local > project > global > system. How does this work for remotes? Can someone define some options in local and some in project?
karajan1001: > Is this an option for `dvc machine status`? And how would `--global/system/project/local` be useful with that option? Wouldn't that be more useful without those options so you could see where each machine config came from?
> 
> >

`machine status` shows the machine running status which is decoupled with the original machine config. So no `--global/system/project/local` for it. But for now, it shows is in the help UI, we need to remove them. Btw, this UI issue also exists in some other machine commands, `create`, `ssh` etc. We can create an issue for it.


> Do you have an example?

I first notice this problem when I was working on the `rename` of remote config. Here is the example, imagine that we have a remote name myremote in `global/repo/local`. What should we do if we rename it on repo level, should it affect the `local` and `global` config. If we don't do that then an original same remote will split, causing some strange behavior some of the config stored in global levels might be missed in the new remote config. But if we do this, it might affect some other repo that is using this remote. Its remote setup might be missing, because we can't rename all of the repos in the local machine. Same as the machine config here. 

> My assumption would be that it would follow the same priority as other config options: local > project > global > system.
 Yes it is. 

> How does this work for remotes? 
Machine and remote are the same for now.

> Can someone define some options in local and some in the project?
Yes, we can define a config in all of the four levels and they will merge into one with the priority defined above.


For now I have two solutions

1. Restrict that one machine config can only be in one level (cross level deduplication) 
2. Make it only a local config ( can't be in system/global level)

Both of these solutions can block the cross repo affection.
dberenbaum: > For now I have two solutions
> 
>     1. Restrict that one machine config can only be in one level (cross level deduplication)
> 
>     2. Make it only a local config ( can't be in system/global level)
> 
> 
> Both of these solutions can block the cross repo affection.

My concern is cases where you might want to define the machine/remote at a repo level to share, but the credentials are stored in the local config.

> I first notice this problem when I was working on the `rename` of remote config. Here is the example, imagine that we have a remote name myremote in `global/repo/local`. What should we do if we rename it on repo level, should it affect the `local` and `global` config. If we don't do that then an original same remote will split, causing some strange behavior some of the config stored in global levels might be missed in the new remote config. But if we do this, it might affect some other repo that is using this remote. Its remote setup might be missing, because we can't rename all of the repos in the local machine. Same as the machine config here.

Are there issues other than renaming? Looking at usage stats, the `rename` commands are very infrequently used, and I'm not convinced they are necessary. Similar to how we ask people to edit stages in `dvc.yaml` now, I think it's reasonable to rename through manual editing. If we support a `rename` command, I think it's fine if it's imperfect, and it's up to the user to make sure they are not breaking their config across multiple repos. We can warn about it, but I don't think it's worth trying to prevent it.
karajan1001: > > For now I have two solutions
> > ```
> > 1. Restrict that one machine config can only be in one level (cross level deduplication)
> > 
> > 2. Make it only a local config ( can't be in system/global level)
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > Both of these solutions can block the cross repo affection.
> 
> My concern is cases where you might want to define the machine/remote at a repo level to share, but the credentials are stored in the local config.

There are some differences between `machine` and `remote`.It might cause a machine missing if we rename a global / system machine, especially for multi-users.
https://github.com/iterative/dvc/pull/6633#discussion_r731614307

> 
> > I first notice this problem when I was working on the `rename` of remote config. Here is the example, imagine that we have a remote name myremote in `global/repo/local`. What should we do if we rename it on repo level, should it affect the `local` and `global` config. If we don't do that then an original same remote will split, causing some strange behavior some of the config stored in global levels might be missed in the new remote config. But if we do this, it might affect some other repo that is using this remote. Its remote setup might be missing, because we can't rename all of the repos in the local machine. Same as the machine config here.
> 
> Are there issues other than renaming? Looking at usage stats, the `rename` commands are very infrequently used, and I'm not convinced they are necessary. Similar to how we ask people to edit stages in `dvc.yaml` now, I think it's reasonable to rename through manual editing. If we support a `rename` command, I think it's fine if it's imperfect, and it's up to the user to make sure they are not breaking their config across multiple repos. We can warn about it, but I don't think it's worth trying to prevent it.

Only `rename` for now
karajan1001: @Mergifyio rebase
mergify[bot]: > rebase

#### ✅ Branch has been successfully rebased



<!--
DO NOT EDIT
-*- Mergify Payload -*-
{"command": "rebase", "conclusion": "success"}
-*- Mergify Payload End -*-
-->


Patch:
diff --git a/dvc/command/machine.py b/dvc/command/machine.py
index a6541ef40..2de8d48f9 100644
--- a/dvc/command/machine.py
+++ b/dvc/command/machine.py
@@ -202,16 +202,32 @@ class CmdMachineStatus(CmdBase):
         "instance_gpu",
     ]
 
+    def _show_machine_status(self, name: str):
+        ui.write(f"machine '{name}':")
+        all_status = list(self.repo.machine.status(name))
+        if not all_status:
+            ui.write("\toffline")
+        for i, status in enumerate(all_status, start=1):
+            ui.write(f"\tinstance_num_{i}:")
+            for field in self.SHOWN_FIELD:
+                value = status.get(field, None)
+                ui.write(f"\t\t{field:20}: {value}")
+
     def run(self):
         if self.repo.machine is None:
             raise MachineDisabledError
 
-        all_status = self.repo.machine.status(self.args.name)
-        for i, status in enumerate(all_status, start=1):
-            ui.write(f"instance_num_{i}:")
-            for field in self.SHOWN_FIELD:
-                value = status.get(field, None)
-                ui.write(f"\t{field:20}: {value}")
+        if self.args.name:
+            self._show_machine_status(self.args.name)
+        else:
+            name_set = set()
+            for level in self.repo.config.LEVELS:
+                conf = self.repo.config.read(level)["machine"]
+                name_set.update(conf.keys())
+            name_list = list(name_set)
+            for name in sorted(name_list):
+                self._show_machine_status(name)
+
         return 0
 
 
@@ -390,7 +406,9 @@ def add_parser(subparsers, parent_parser):
     )
     machine_create_parser.set_defaults(func=CmdMachineCreate)
 
-    machine_STATUS_HELP = "List the status of a running machine."
+    machine_STATUS_HELP = (
+        "List the status of running instances for one/all machines."
+    )
     machine_status_parser = machine_subparsers.add_parser(
         "status",
         parents=[parent_config_parser, parent_parser],
@@ -399,7 +417,7 @@ def add_parser(subparsers, parent_parser):
         formatter_class=argparse.RawDescriptionHelpFormatter,
     )
     machine_status_parser.add_argument(
-        "name", help="Name of the running machine."
+        "name", nargs="?", help="(optional) Name of the machine."
     )
     machine_status_parser.set_defaults(func=CmdMachineStatus)
 


Test patch:
diff --git a/tests/func/machine/test_machine_status.py b/tests/func/machine/test_machine_status.py
index 9bea45e55..33b474be7 100644
--- a/tests/func/machine/test_machine_status.py
+++ b/tests/func/machine/test_machine_status.py
@@ -8,6 +8,7 @@ def test_status(
     status = machine_instance
     assert main(["machine", "status", "foo"]) == 0
     cap = capsys.readouterr()
-    assert "instance_num_1:" in cap.out
+    assert "machine 'foo':\n" in cap.out
+    assert "\tinstance_num_1:\n" in cap.out
     for key in CmdMachineStatus.SHOWN_FIELD:
-        assert f"\t{key:20}: {status[key]}" in cap.out
+        assert f"\t\t{key:20}: {status[key]}\n" in cap.out
diff --git a/tests/unit/command/test_machine.py b/tests/unit/command/test_machine.py
index 4177a89ab..049db1022 100644
--- a/tests/unit/command/test_machine.py
+++ b/tests/unit/command/test_machine.py
@@ -1,4 +1,5 @@
 import configobj
+from mock import call
 
 from dvc.cli import parse_args
 from dvc.command.machine import (
@@ -19,6 +20,8 @@ DATA = {
             "[feature]\n"
             "  machine = true\n"
             "['machine \"foo\"']\n"
+            "  cloud = aws\n"
+            "['machine \"myaws\"']\n"
             "  cloud = aws"
         )
     }
@@ -42,7 +45,7 @@ def test_remove(tmp_dir):
     cmd = cli_args.func(cli_args)
     assert cmd.run() == 0
     config = configobj.ConfigObj(str(tmp_dir / ".dvc" / "config"))
-    assert list(config.keys()) == ["feature"]
+    assert list(config.keys()) == ["feature", 'machine "myaws"']
 
 
 def test_create(tmp_dir, dvc, mocker):
@@ -58,7 +61,8 @@ def test_create(tmp_dir, dvc, mocker):
     m.assert_called_once_with("foo")
 
 
-def test_status(tmp_dir, dvc, mocker):
+def test_status(tmp_dir, scm, dvc, mocker):
+    tmp_dir.gen(DATA)
     cli_args = parse_args(["machine", "status", "foo"])
     assert cli_args.func == CmdMachineStatus
 
@@ -66,10 +70,18 @@ def test_status(tmp_dir, dvc, mocker):
     m = mocker.patch.object(
         cmd.repo.machine, "status", autospec=True, return_value=[]
     )
-
     assert cmd.run() == 0
     m.assert_called_once_with("foo")
 
+    cli_args = parse_args(["machine", "status"])
+    cmd = cli_args.func(cli_args)
+    m = mocker.patch.object(
+        cmd.repo.machine, "status", autospec=True, return_value=[]
+    )
+    assert cmd.run() == 0
+    assert m.call_count == 2
+    m.assert_has_calls([call("foo"), call("myaws")])
+
 
 def test_destroy(tmp_dir, dvc, mocker):
     cli_args = parse_args(["machine", "destroy", "foo"])