Merge ~troyanov/maas:fix-2042645 into maas:master

[Anton Troyanov]

refactor: get_effective_power_parameters only required

This change addresses the disclosure of various power parameters to drivers that do not rely on them.

- 'mac_address' used by IPMI only, but it should be BMC address (which MAAS doesn't now), so this parameter can be simply removed
- 'power_address', 'username', 'power_pass' should be set by the driver parameters
- 'power_id' used only by 'virsh
- 'power_driver' default empty value is not used by any driver='power_off_mode' - used by 'ipmi' only
- 'boot_mode' used by 'amt' only

Resolves LP:2042645

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-2042645 lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: ac6fae857039252029340b1d6494c45e2d9a4610

review: Approve

[Jacopo Rota (r00ta)]

lgtm

review: Approve

[Björn Tillenius (bjornt)]

Why do we send parameters that should be ignored?

I'm a bit against ignoring parameters like that. That means that if we get the parameter wrong for some reason, we will get a subtle error later on, rather than to get a clear error up front.

So why can't we send only the required parameters? We control all the code.

review: Needs Information

[Anton Troyanov (troyanov)]

We send parameters that we fetch from existing model methods (and it used to work this way).
Driver logic ignores unknown parameters, it's just maas-power CLI that chokes (because of ArgParse) and thats why this proposal is removing strict check for the CLI (to be on par with driver logic)

Extra parameters are added here:
`get_effective_power_parameters` from src/maasserver/models/node.py adds power_params["mac_address"] each time

If I understand correctly:
- `mac_address` is used only by IPMI
- `power_off_mode` is also used by IPMI
- `power_id` is used only by virsh?

(I didn't check all of them)

[Björn Tillenius (bjornt)]

Maybe this is a good opportunity to clean things up.

For example, mac_address is interesting. Yes, IPMI accepts a mac_address. But it expects the MAC of the BMC port, which the host won't see. So the mac_address returned by get_effective_power_parameters() is not usable by the IPMI power driver.

[Björn Tillenius (bjornt)]

We had a meet and went over this. The TL;DR is that we should clean up get_effective_power_parameters() to only return what's actually needed.

'system_id' is part of the power API contract, so that one probably has to stay.
  '
'power_address', 'username', 'power_id', 'power_driver', 'power_pass' should be removed, since they are already set when you configure a power driver.

'mac_address' should be removed, since it's only used by IPMI, but as per my comment above, it's the wrong mac address.

'power_off_mode' and 'boot_mode' are a bit special. 'power_off_mode' is kind of part of the power control contract, but only IPMI is actually accepting it. 'boot_mode' is only used for AMT.

We should look into how to deal with 'power_off_mode' and 'boot_mode' better, but for now, let's add them only to the power parameters if the power driver requires them.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-2042645 lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 3388903e9ce22d374260d91b95a8305aada511e9

review: Approve

3f8f2fc... by Anton Troyanov on 2023-11-15

fixup! fix: remove power_off_mode

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-2042645 lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: d95e5217bfbabee6bedf5a5bd02188b251fc98ed

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-2042645 lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/4020/console
COMMIT: 3f8f2fcb7573dabde120711a8a08c035d09ed76c

review: Needs Fixing

[Björn Tillenius (bjornt)]

+1

review: Approve

3f73eb9... by Anton Troyanov on 2023-11-15

fix: fix tests to reflect power param changes

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-2042645 lp:~troyanov/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 3f73eb999cfa32551e48d8b4f2766da6182c97cd

review: Approve

diff --git a/src/maasagent/internal/workflow/power.go b/src/maasagent/internal/workflow/power.go
index f29df39..563bbad 100644
--- a/src/maasagent/internal/workflow/power.go
+++ b/src/maasagent/internal/workflow/power.go
@@ -27,24 +27,6 @@ var (
 	ErrWrongPowerState = errors.New("BMC is in the wrong power state")
 )
 
-var (
-	// power parameters fetched from the DB contain extra
-	// parameters the CLI does not need and will cause errors in the CLI
-	// so we ignore them
-	ignoredPowerOptions = map[string]struct{}{
-		"power_id":       {},
-		"system_id":      {},
-		"boot_mode":      {},
-		"power_off_mode": {},
-	}
-
-	// these driver types do not take a MAC address argument on the CLI
-	ignoredMACDriverTypes = map[string]struct{}{
-		"lxd":   {},
-		"virsh": {},
-	}
-)
-
 // PowerParam is the workflow parameter for power management of a host
 type PowerParam struct {
 	SystemID   string                 `json:"system_id"`
@@ -54,16 +36,6 @@ type PowerParam struct {
 	DriverType string                 `json:"driver_type"`
 }
 
-func shouldIgnoreMACDriverType(driver, key string) bool {
-	_, ignore := ignoredMACDriverTypes[driver]
-	return ignore && key == "mac_address"
-}
-
-func shouldIgnorePowerOption(key string) bool {
-	_, ignore := ignoredPowerOptions[key]
-	return ignore
-}
-
 // powerCLIExecutableName returns correct MAAS Power CLI executable name
 // depending on the installation type (snap or deb package)
 func powerCLIExecutableName() string {
@@ -74,11 +46,13 @@ func powerCLIExecutableName() string {
 	return "maas-power"
 }
 
-func fmtPowerOpts(driver string, opts map[string]interface{}) []string {
+func fmtPowerOpts(opts map[string]interface{}) []string {
 	var res []string
 
 	for k, v := range opts {
-		if shouldIgnoreMACDriverType(driver, k) || shouldIgnorePowerOption(k) {
+		// skip 'system_id' which is added to parameters.
+		// it has nothing to do with power driver parameters
+		if k == "system_id" {
 			continue
 		}
 
@@ -128,7 +102,7 @@ func PowerActivity(ctx context.Context, params PowerActivityParam) (*PowerResult
 		return nil, err
 	}
 
-	opts := fmtPowerOpts(params.DriverType, params.DriverOpts)
+	opts := fmtPowerOpts(params.DriverOpts)
 	args := append([]string{params.Action, params.DriverType}, opts...)
 
 	log.Debug("Executing MAAS power CLI", tag.Builder().KV("args", args).KeyVals...)
diff --git a/src/maasagent/internal/workflow/power_test.go b/src/maasagent/internal/workflow/power_test.go
index 1682595..93a2fcb 100644
--- a/src/maasagent/internal/workflow/power_test.go
+++ b/src/maasagent/internal/workflow/power_test.go
@@ -31,6 +31,10 @@ func TestFmtPowerOpts(t *testing.T) {
 			in:  map[string]interface{}{"key1": "multi\nline\nstring"},
 			out: []string{"--key1", "multi\nline\nstring"},
 		},
+		"ignore system_id": {
+			in:  map[string]interface{}{"system_id": "value1"},
+			out: []string{},
+		},
 	}
 
 	for name, tc := range testcases {
@@ -38,7 +42,7 @@ func TestFmtPowerOpts(t *testing.T) {
 
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
-			res := fmtPowerOpts("driver", tc.in)
+			res := fmtPowerOpts(tc.in)
 			assert.ElementsMatch(t, tc.out, res)
 		})
 	}
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index ba5d7c6..8a330d1 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -3115,33 +3115,21 @@ class Node(CleanSave, TimestampedModel):
         # TODO: This default ought to be in the virsh template.
         if self.bmc is not None and self.bmc.power_type == "virsh":
             power_params.setdefault("power_address", "qemu://localhost/system")
-        else:
-            power_params.setdefault("power_address", "")
-        power_params.setdefault("username", "")
-        power_params.setdefault("power_id", self.system_id)
-        power_params.setdefault("power_driver", "")
-        power_params.setdefault("power_pass", "")
-        power_params.setdefault("power_off_mode", "")
-
-        # The "mac" parameter defaults to the node's boot interace MAC
-        # address, but only if not already set.
-        if "mac_address" not in power_params:
-            boot_interface = self.get_boot_interface()
-            if (
-                boot_interface is not None
-                and boot_interface.mac_address is not None
-            ):
-                power_params["mac_address"] = boot_interface.mac_address
+            power_params.setdefault("power_id", self.system_id)
+
+        if self.bmc is not None and self.bmc.power_type == "ipmi":
+            power_params.setdefault("power_off_mode", "")
 
         # boot_mode is something that tells the template whether this is
         # a PXE boot or a local HD boot.
-        if (
-            self.status == NODE_STATUS.DEPLOYED
-            or self.node_type != NODE_TYPE.MACHINE
-        ):
-            power_params["boot_mode"] = "local"
-        else:
-            power_params["boot_mode"] = "pxe"
+        if self.bmc is not None and self.bmc.power_type == "amt":
+            if (
+                self.status == NODE_STATUS.DEPLOYED
+                or self.node_type != NODE_TYPE.MACHINE
+            ):
+                power_params["boot_mode"] = "local"
+            else:
+                power_params["boot_mode"] = "pxe"
 
         return power_params
 
@@ -5857,7 +5845,8 @@ class Node(CleanSave, TimestampedModel):
             boot_order = []
 
         # Smuggle in a hint about how to power-off the self.
-        power_info.power_parameters["power_off_mode"] = stop_mode
+        if power_info.power_type == "ipmi":
+            power_info.power_parameters["power_off_mode"] = stop_mode
 
         # Request that the node be powered off post-commit.
         d = post_commit()
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 075da6c..eefc848 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1607,31 +1607,18 @@ class TestNode(MAASServerTestCase):
             node.system_id, node.get_effective_power_parameters()["system_id"]
         )
 
-    def test_get_effective_power_parameters_adds_mac_if_no_params_set(self):
-        node = factory.make_Node()
-        mac = factory.make_mac_address()
-        node.add_physical_interface(mac)
-        self.assertEqual(
-            mac, node.get_effective_power_parameters()["mac_address"]
-        )
-
     def test_get_effective_power_parameters_adds_no_mac_if_params_set(self):
         node = factory.make_Node(power_parameters={"foo": "bar"})
         mac = factory.make_mac_address()
         node.add_physical_interface(mac)
         self.assertNotIn("mac", node.get_effective_power_parameters())
 
-    def test_get_effective_power_parameters_adds_empty_power_off_mode(self):
-        node = factory.make_Node()
-        params = node.get_effective_power_parameters()
-        self.assertEqual("", params["power_off_mode"])
-
     def test_get_effective_power_type_no_default_power_address_if_not_virsh(
         self,
     ):
         node = factory.make_Node(power_type="manual")
         params = node.get_effective_power_parameters()
-        self.assertEqual("", params["power_address"])
+        self.assertEqual(None, params.get("power_address"))
 
     def test_get_effective_power_type_defaults_power_address_if_virsh(self):
         node = factory.make_Node(power_type="virsh")
@@ -1639,13 +1626,13 @@ class TestNode(MAASServerTestCase):
         self.assertEqual("qemu://localhost/system", params["power_address"])
 
     def test_get_effective_power_parameters_sets_local_boot_mode(self):
-        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
+        node = factory.make_Node(status=NODE_STATUS.DEPLOYED, power_type="amt")
         params = node.get_effective_power_parameters()
         self.assertEqual("local", params["boot_mode"])
 
     def test_get_effective_power_parameters_sets_pxe_boot_mode(self):
         status = factory.pick_enum(NODE_STATUS, but_not=[NODE_STATUS.DEPLOYED])
-        node = factory.make_Node(status=status)
+        node = factory.make_Node(status=status, power_type="amt")
         params = node.get_effective_power_parameters()
         self.assertEqual("pxe", params["boot_mode"])
 
@@ -2675,7 +2662,6 @@ class TestNode(MAASServerTestCase):
         self.expectThat(node.install_rackd, Is(False))
 
         expected_power_info = node.get_effective_power_info()
-        expected_power_info.power_parameters["power_off_mode"] = "hard"
         node._power_control_node.assert_called_once_with(
             d, "power_off", expected_power_info, []
         )
@@ -10274,7 +10260,7 @@ class TestNode_Stop(MAASServerTestCase):
         d = self.patch_post_commit()
         admin = factory.make_admin()
         stop_mode = factory.make_name("stop")
-        node = self.make_acquired_node_with_interface(admin)
+        node = self.make_acquired_node_with_interface(admin, power_type="ipmi")
         mock_power_control = self.patch_autospec(node, "_power_control_node")
         node.stop(admin, stop_mode=stop_mode)
         expected_power_info = node.get_effective_power_info()
@@ -10287,7 +10273,7 @@ class TestNode_Stop(MAASServerTestCase):
         d = self.patch_post_commit()
         admin = factory.make_admin()
         stop_mode = factory.make_name("stop")
-        node = self.make_acquired_node_with_interface(admin)
+        node = self.make_acquired_node_with_interface(admin, power_type="ipmi")
         mock_power_control = self.patch_autospec(node, "_power_control_node")
         node.stop(stop_mode=stop_mode)
         expected_power_info = node.get_effective_power_info()