Show multiple IP's in `list` and `info` commands #1886

luis4a0 · 2020-12-15T13:44:50Z

This PR adds the ability to gather all the IP's used by each instance.

The private bits of this PR are in https://github.com/canonical/multipass-private/pull/338.

codecov · 2020-12-15T14:06:24Z

Codecov Report

Merging #1886 (d281627) into master (f2c175f) will increase coverage by 0.01%.
The diff coverage is 64.13%.

@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
+ Coverage   78.46%   78.47%   +0.01%     
==========================================
  Files         234      234              
  Lines        8768     8856      +88     
==========================================
+ Hits         6880     6950      +70     
- Misses       1888     1906      +18

Impacted Files	Coverage Δ
include/multipass/virtual_machine.h	`100.00% <ø> (ø)`
src/daemon/daemon.cpp	`45.48% <27.90%> (-0.62%)`	⬇️
src/platform/backends/lxd/lxd_virtual_machine.cpp	`98.63% <66.66%> (-0.11%)`	⬇️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp	`76.20% <83.33%> (+2.82%)`	⬆️
src/client/cli/formatter/csv_formatter.cpp	`100.00% <100.00%> (ø)`
src/client/cli/formatter/json_formatter.cpp	`100.00% <100.00%> (ø)`
src/client/cli/formatter/table_formatter.cpp	`96.55% <100.00%> (+1.22%)`	⬆️
src/client/cli/formatter/yaml_formatter.cpp	`100.00% <100.00%> (ø)`
...tform/backends/libvirt/libvirt_virtual_machine.cpp	`72.63% <100.00%> (ø)`
...rc/platform/backends/shared/shared_backend_utils.h	`100.00% <100.00%> (ø)`
... and 11 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 f2c175f...d281627. Read the comment docs.

ricab

Thanks @luis4a0, the approach and code look good! I will do some more testing when reviewing the private side, but everything seems to work well so far.

I have some requests and suggestions for refinement (many are repeated), but the strategy looks solid 😃

src/daemon/daemon.cpp

src/client/cli/formatter/json_formatter.cpp

src/client/cli/formatter/yaml_formatter.cpp

ricab · 2021-01-06T12:14:52Z

tests/qemu/test_qemu_backend.cpp

+    ON_CALL(mock_dnsmasq_server, get_ip_for(_)).WillByDefault([&expected_ip](auto...) {
+        return mp::optional<mp::IPAddress>{expected_ip};
+    });


Why the lambda? Should be able to just ....WillByDefault(Return(expected_ip)).

And might as well EXPECT_CALL, no?

Right, changed the lambda and added the EXPECT_CALL.

Why did you keep the ON_CALL though? You have both ON_CALL and EXPECT_CALL now.

Just read the difference between them. I was just confused on the meaning of ON_CALL. Changed.

ricab · 2021-01-06T12:15:04Z

tests/qemu/test_qemu_backend.cpp

+    mpt::StubVMStatusMonitor stub_monitor;
+    NiceMock<mpt::MockDNSMasqServer> mock_dnsmasq_server{data_dir.path(), bridge_name, subnet};
+
+    ON_CALL(mock_dnsmasq_server, get_ip_for(_)).WillByDefault([](auto...) { return mp::optional<mp::IPAddress>{}; });


idem: return (with nullopt here) and expect call

idem: {ON,EXPECT}_CALL

ricab

Final details inline. You know you can just accept the suggestions (in batches) on GH, right?

ricab · 2021-01-15T15:20:03Z

src/daemon/daemon.cpp

-            info->set_disk_total(
-                run_in_vm("df --output=size `awk '$2 == \"/\" { print $1 }' /proc/mounts` -B1 | sed 1d"));
-            info->set_ipv4(vm->ipv4());
+            for (auto extra_ipv4 : all_ipv4)


Suggested change

for (auto extra_ipv4 : all_ipv4)

for (const auto& extra_ipv4 : all_ipv4)

ricab · 2021-01-15T15:20:30Z

src/daemon/daemon.cpp

+            if (is_ipv4_valid(management_ip))
+                entry->add_ipv4(management_ip);
+
+            for (auto extra_ipv4 : all_ipv4)


Suggested change

for (auto extra_ipv4 : all_ipv4)

for (const auto& extra_ipv4 : all_ipv4)

ricab · 2021-01-15T15:22:33Z

tests/qemu/test_qemu_backend.cpp

+    const std::string expected_ip{"10.10.0.35"};
+    NiceMock<mpt::MockDNSMasqServer> mock_dnsmasq_server{data_dir.path(), bridge_name, subnet};
+
+    EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).Times(1).WillRepeatedly(Return(expected_ip));


Suggested change

EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).Times(1).WillRepeatedly(Return(expected_ip));

EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).WillOnce(Return(expected_ip));

ricab · 2021-01-15T15:22:53Z

tests/qemu/test_qemu_backend.cpp

+    mpt::StubVMStatusMonitor stub_monitor;
+    NiceMock<mpt::MockDNSMasqServer> mock_dnsmasq_server{data_dir.path(), bridge_name, subnet};
+
+    EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).Times(1).WillRepeatedly(Return(mp::nullopt));


Suggested change

EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).Times(1).WillRepeatedly(Return(mp::nullopt));

EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).WillOnce(Return(mp::nullopt));

Also simplified code of some mock returns.

ricab

OK, this is good now, thanks!

bors r+

1886: Show multiple IP's in `list` and `info` commands r=ricab a=luis4a0 This PR adds the ability to gather all the IP's used by each instance. The private bits of this PR are in https://github.com/canonical/multipass-private/pull/338. Co-authored-by: Luis Peñaranda <[email protected]>

ricab · 2021-01-15T16:39:37Z

Canceling so that this does not go away before rebasing the other side (which will need to merge with this)

bors r-

bors · 2021-01-15T16:39:39Z

Canceled.

ricab · 2021-01-15T17:26:32Z

Alright, all's good

bors r+

1886: Show multiple IP's in `list` and `info` commands r=ricab a=luis4a0 This PR adds the ability to gather all the IP's used by each instance. The private bits of this PR are in https://github.com/canonical/multipass-private/pull/338. Co-authored-by: Luis Peñaranda <[email protected]>

bors · 2021-01-15T17:44:59Z

Build failed:

Linux

ricab · 2021-01-15T17:48:04Z

bors retry

bors · 2021-01-15T18:26:32Z

Build failed:

macOS

1886: Show multiple IP's in `list` and `info` commands r=ricab a=luis4a0 This PR adds the ability to gather all the IP's used by each instance. The private bits of this PR are in canonical/multipass-private#338. Co-authored-by: Luis Peñaranda <[email protected]>

luis4a0 marked this pull request as draft December 15, 2020 13:47

luis4a0 marked this pull request as ready for review December 16, 2020 13:48

luis4a0 force-pushed the multiple-ips-list-info branch from 0e9b9cd to 277d920 Compare December 16, 2020 19:26

luis4a0 added 13 commits January 4, 2021 11:36

[rpc] Permit multiple IP's in the command list.

b674f02

[cli] Show multipe IP's in list command.

2126cf0

[tests] Test new output of the list command.

c417c66

[rpc] Permit multiple IP's in the command info.

2d4ca7e

[cli] Show multipe IP's in info command.

9869f29

[tests] Test new output of the info command.

a05c0d8

[vm] Rename ipv4() to management_ipv4().

a9804f5

[vm] Rename ip field to management_ip.

e042dec

[daemon] Naive listing of all the instance IP's.

72184dc

[daemon] Show only IP's with global or link scope.

4e6f9df

[daemon] Only show valid IP addresses.

bdef7d9

[tests] Some IPv6 strings in info and list.

ada1a2d

[tests][qemu] Tests for management_ipv4().

f3d3d4e

luis4a0 force-pushed the multiple-ips-list-info branch from 277d920 to f3d3d4e Compare January 4, 2021 14:39

ricab requested changes Jan 6, 2021

View reviewed changes

luis4a0 added 2 commits January 12, 2021 11:47

[cli] Simplified some formatter code.

7129e5c

[daemon] Improve code for showing multipe IP's.

4563d51

luis4a0 force-pushed the multiple-ips-list-info branch from f66e6ec to f3189ff Compare January 15, 2021 13:35

ricab reviewed Jan 15, 2021

View reviewed changes

luis4a0 added 2 commits January 15, 2021 13:00

[tests][qemu] Expect calls to get_ip_for().

06f3704

Also simplified code of some mock returns.

[daemon] Use const references in some loops.

d281627

luis4a0 force-pushed the multiple-ips-list-info branch from f3189ff to d281627 Compare January 15, 2021 16:06

ricab approved these changes Jan 15, 2021

View reviewed changes

ricab merged commit 8823c92 into master Jan 15, 2021

bors bot deleted the multiple-ips-list-info branch January 15, 2021 19:10

Saviq added this to the v1.6.0 milestone Feb 2, 2021

jmcausing mentioned this pull request Feb 22, 2021

Windows 10 still no IPv4 using Virtual Box (latest version multipass 1.6.2+win) #1979

Closed

	for (auto extra_ipv4 : all_ipv4)
	for (const auto& extra_ipv4 : all_ipv4)

	EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).Times(1).WillRepeatedly(Return(expected_ip));
	EXPECT_CALL(mock_dnsmasq_server, get_ip_for(_)).WillOnce(Return(expected_ip));

Show multiple IP's in list and info commands #1886

Show multiple IP's in list and info commands #1886

Uh oh!

Conversation

luis4a0 commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

ricab commented Jan 15, 2021

Uh oh!

bors bot commented Jan 15, 2021

Uh oh!

ricab commented Jan 15, 2021

Uh oh!

bors bot commented Jan 15, 2021

Uh oh!

ricab commented Jan 15, 2021

Uh oh!

bors bot commented Jan 15, 2021

Uh oh!

Uh oh!

Show multiple IP's in `list` and `info` commands #1886

Show multiple IP's in `list` and `info` commands #1886

luis4a0 commented Dec 15, 2020 •

edited

Loading

codecov bot commented Dec 15, 2020 •

edited

Loading