Merge ~steverydz/maas:fix-static-ip-and-subnet-assignment into maas:master

Proposed by Steve Rydz on 2018-12-11
Status: Rejected
Rejected by: Blake Rouse on 2019-03-20
Proposed branch: ~steverydz/maas:fix-static-ip-and-subnet-assignment
Merge into: maas:master
Diff against target: 102 lines (+17/-7)
2 files modified
src/maasserver/static/js/angular/controllers/node_details_networking.js (+7/-3)
src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js (+10/-4)
Reviewer Review Type Date Requested Status
Blake Rouse Disapprove on 2019-03-20
Mike Pontillo (community) 2018-12-11 Needs Information on 2018-12-11
MAAS Lander Approve on 2018-12-11
Review via email:

Commit message

LP: #1806099 - Make sure static IP is assigned when static mode rather than auto assigned

LP: #1806112 - Send subnet ID when creating a bridge or a bond in machine interfaces

To post a comment you must log in.
MAAS Lander (maas-lander) wrote :

-b fix-static-ip-and-subnet-assignment lp:~steverydz/maas/+git/maas into -b master lp:~maas-committers/maas

COMMIT: 53d1296654513b3d3bc73e9356a18d4fee488365

review: Approve
Mike Pontillo (mpontillo) wrote :

I think the code in this branch is fine, but I think we should discuss the larger picture. Your previous merge proposal stated that back-end changes would be required to get this to work. But I think the reason that setting the IP mode and subnet doesn't work is because it was designed that way for a good reason, and we should discuss further before we change that.

That is, if I am creating a bridge, it should inherit the IP assignment from the interface I created it on. Similarly, if I create a bond, it should inherit the IP assignments from its parent. (The only difference: AUTO and DHCP assignments should be combined, and if a combination of AUTO, DHCP, and Static appear on bond parents, we should prefer static, auto, and DHCP in that order.)

Right now it's not doing that it all. In fact, instead MAAS tends to *remove* all the IP addresses rather than even trying to make a good guess.


With that, here are some important questions about the UX that we need to answer:

Should the user be allowed to configure the IP mode at bond or bridge creation time?

If the answer is "yes", what subset of the configuration of the currently-existing interfaces should be shown by default, so that the UI is consistent with the back-end? (We can use the algorithm I described above, but what about edge cases such as multiple static IP address assignments?)

If the answer is "no", we just need to remove IP assignment from the "add" screen and make sure that when bonds or bridges are added, MAAS does something that is predictable and unsurprising. Then we could just replace the IP assignment editing boxes that appear upon bond or bridge creation with some text that states what MAAS will do with existing IP assignments.

(Additionally, when a bond or bridge is deleted, we should probably take any IP assignments and place them on the first parent interface.)


I tested this with the API as well; given a machine with system_id 'ta3rxy' and two NICs (IDs 6 and 8), the minimum to create a bond is something like the following:

    maas admin interfaces create-bond ta3rxy parents=6 parents=10 name=bond0

(It fails without the `name=bond0` part.)

The documentation states that leaving out the VLAN results in "disconnected", but that turns out to not be true. In MAAS 2.5, leaving the VLAN out results in the bond being connected to the same VLAN as its parent.

Just as is the case with the UI, creating the bond results in no IP addresses being transferred to the new bond.

Similarly, this is the minimum command to create a bridge:

    maas admin interfaces create-bridge ta3rxy parents=6 name=br0

As with bonds, no IP address assignments are transferred to the new bridge.

review: Needs Information
Blake Rouse (blake-rouse) wrote :
review: Disapprove

Unmerged commits

53d1296... by Steve Rydz on 2018-12-11

Merge branch 'static-ip-not-assigned' into fix-static-ip-and-subnet-assignment

10837a3... by Steve Rydz on 2018-12-11

Merge branch 'keep-subnet-state-for-bridge-and-bond-create' into fix-static-ip-and-subnet-assignment

97fe856... by Steve Rydz on 2018-12-11

Fix broken unit tests

5661409... by Steve Rydz on 2018-12-11

Remove trailing whitespace

3605bfc... by Steve Rydz on 2018-12-10

Whitespace change to re-trigger build

d8410e3... by Steve Rydz on 2018-12-07

Use static ip rather than auto assign when selected

fbf3404... by Steve Rydz on 2018-12-10

Fix unit tests

66276f2... by Steve Rydz on 2018-12-10

Update unit tests

1a7d737... by Steve Rydz on 2018-12-07

Update unit tests

3217c10... by Steve Rydz on 2018-12-07

Send subnet id when creating bridge or bond

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/static/js/angular/controllers/node_details_networking.js b/src/maasserver/static/js/angular/controllers/node_details_networking.js
2index b4c2eac..06dd9fa 100644
3--- a/src/maasserver/static/js/angular/controllers/node_details_networking.js
4+++ b/src/maasserver/static/js/angular/controllers/node_details_networking.js
5@@ -33,6 +33,7 @@ angular.module('MAAS').filter('filterByUnusedForInterface', function() {
6 });
10 // Filter that is specific to the NodeNetworkingController. Filters the
11 // list of interfaces to not include the current parent interfaces being
12 // bonded together.
13@@ -1525,7 +1526,8 @@ angular.module('MAAS').controller('NodeNetworkingController', [
14 vlan: vlan_id,
15 bond_mode: $scope.newBondInterface.bond_mode,
16 bond_lacp_rate: $scope.newBondInterface.lacpRate,
17- bond_xmit_hash_policy: $scope.newBondInterface.xmitHashPolicy
18+ bond_xmit_hash_policy: $scope.newBondInterface.xmitHashPolicy,
19+ subnet: $
20 };
21 $scope.$parent.nodesManager.createBondInterface(
22 $scope.node, params).then(null, function(error) {
23@@ -1668,7 +1670,8 @@ angular.module('MAAS').controller('NodeNetworkingController', [
24 vlan:,
25 fabric:,
26 bridge_stp: $scope.newBridgeInterface.bridge_stp,
27- bridge_fd: $scope.newBridgeInterface.bridge_fd
28+ bridge_fd: $scope.newBridgeInterface.bridge_fd,
29+ subnet: $
30 };
31 $scope.$parent.nodesManager.createBridgeInterface(
32 $scope.node, params).then(null, function(error) {
33@@ -1757,7 +1760,8 @@ angular.module('MAAS').controller('NodeNetworkingController', [
34 function(tag) { return tag.text; }),
35 mac_address: $scope.newInterface.mac_address,
36 vlan: $,
37- mode: $scope.newInterface.mode
38+ mode: $scope.newInterface.mode,
39+ ip_address: $scope.newInterface.ip_address
40 };
41 }
42 if(angular.isObject($scope.newInterface.subnet)) {
43diff --git a/src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js b/src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js
44index 85d1678..fd4be12 100644
45--- a/src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js
46+++ b/src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js
47@@ -3754,7 +3754,8 @@ describe("NodeNetworkingController", function() {
48 vlan:,
49 bond_mode: "active-backup",
50 bond_lacp_rate: "fast",
51- bond_xmit_hash_policy: "layer2"
52+ bond_xmit_hash_policy: "layer2",
53+ subnet: undefined
54 });
55 expect($scope.interfaces).toEqual([]);
56 expect($scope.newBondInterface).toEqual({});
57@@ -3802,7 +3803,8 @@ describe("NodeNetworkingController", function() {
58 vlan: null,
59 bond_mode: "active-backup",
60 bond_lacp_rate: "fast",
61- bond_xmit_hash_policy: "layer2"
62+ bond_xmit_hash_policy: "layer2",
63+ subnet: undefined
64 });
65 expect($scope.interfaces).toEqual([]);
66 expect($scope.newBondInterface).toEqual({});
67@@ -4033,6 +4035,7 @@ describe("NodeNetworkingController", function() {
68 vlan: vlan,
69 fabric: fabric
70 };
72 $scope.interfaces = [nic1];
73 $scope.interfaceLinksMap = {};
74 $scope.interfaceLinksMap[] = {};
75@@ -4045,6 +4048,7 @@ describe("NodeNetworkingController", function() {
76 spyOn($scope, "cannotAddBridge").and.returnValue(false);
77 $ = "br0";
78 $scope.newBridgeInterface.mac_address = "00:11:22:33:44:55";
79+ $scope.newBridgeInterface.subnet = {};
80 $scope.addBridge();
82 expect(MachinesManager.createBridgeInterface).toHaveBeenCalledWith(
83@@ -4056,7 +4060,8 @@ describe("NodeNetworkingController", function() {
84 vlan:,
85 fabric:,
86 bridge_stp: false,
87- bridge_fd: 15
88+ bridge_fd: 15,
89+ subnet: undefined
90 });
91 expect($scope.interfaces).toEqual([]);
92 expect($scope.newBridgeInterface).toEqual({});
93@@ -4296,7 +4301,8 @@ describe("NodeNetworkingController", function() {
94 tags: [],
95 vlan:,
96 subnet:,
97- mode: "auto"
98+ mode: "auto",
99+ ip_address: undefined
100 });
101 expect($scope.newInterface).toEqual({});
102 expect($scope.selectedMode).toBeNull();


People subscribed via source and target branches

to all changes: