CA-396743: Don't allow NBD on boot from SAN devices, and fix Network.managed to reflect PIF.managed #5917
Conversation
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
edwintorok
changed the title
This will make it easier to filter out these networks in test code. A PIF is unmanaged when it is a boot from SAN interface for example, as returned by the 'bfs-interfaces' script. Certain operations are not valid on such interfaces, e.g. you cannot use them to export NBD devices. Fixes: 1a9cc76 ("CP-20482: Create network with the specified bridge name") Signed-off-by: Edwin Török <edwin.torok@cloud.com>
We wouldn't be able to add the correct firewall rules, and you're not meant to use the boot from SAN network for NBD. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
There is no bridge for unmanaged devices, return the empty string instead of a non-existent device. Previously we would've returned 'bribft0' for the 'ibft0' interface. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
edwintorok
force-pushed
the
private/edvint/ibft
branch
from
7d1e73f
to
1adffbd
Compare
|
Network.managed refers to whether XAPI manages the bridge (not to whether XAPI manages the Network object, which it does it it created it). This is similar to how PIF.managed refers to the interface, not the object itself. |
|
I'm now getting a proper failure message from the testcase, next step: I'm fixing the testcase so it doesn't attempt to set NBD purpose on boot-from-SAN interfaces, and then it should pass and this PR should be ready. |
|
Apparently the machines were in an unsupported configuration. Meanwhile we should probably merge this change with the improved error message. |
| @@ -362,7 +363,7 @@ let find_or_create_network (bridge : string) (device : string) ~__context = | |||
| Db.Network.create ~__context ~ref:net_ref ~uuid:net_uuid | |||
| ~current_operations:[] ~allowed_operations:[] | |||
| ~name_label:(Helpers.choose_network_name_for_pif device) | |||
| ~name_description:"" ~mTU:1500L ~purpose:[] ~bridge ~managed:true | |||
| ~name_description:"" ~mTU:1500L ~purpose:[] ~bridge ~managed | |||
There was a problem hiding this comment.
Is the managed field in network object a duplication of the field managed of PIF objects?
In other words, if a PIF is managed, its connected network must be managed?
There was a problem hiding this comment.
I believe they are separate, one is for the network bridge which connects things together, while the other one is for configuring the interface (bring the interface up/down, etc). Although it seems that in this case the created pif has the same managed field as the network
| @@ -362,7 +363,7 @@ let find_or_create_network (bridge : string) (device : string) ~__context = | |||
| Db.Network.create ~__context ~ref:net_ref ~uuid:net_uuid | |||
| ~current_operations:[] ~allowed_operations:[] | |||
| ~name_label:(Helpers.choose_network_name_for_pif device) | |||
| ~name_description:"" ~mTU:1500L ~purpose:[] ~bridge ~managed:true | |||
| ~name_description:"" ~mTU:1500L ~purpose:[] ~bridge ~managed | |||
There was a problem hiding this comment.
Previously we only ever introduce a managed network, even if we create an unmanaged PIF, is this ok?
| @@ -667,6 +668,8 @@ let scan ~__context ~host = | |||
| ([], []) | |||
| ) | |||
| in | |||
| debug "non-managed devices=%s" (String.concat "," non_managed_devices) ; | |||
There was a problem hiding this comment.
Should new log lines contain __FUNCTION__ to help locating the context?
This'll make it more obvious if test code attempts to do the wrong thing (such as setting
ibft0with NBD purpose and expecting that to work).It'll also make it easier for test code to filter out these networks, so it doesn't touch them.
Unfortunately we do need to create the Network object, because the PIF object references it, and I wouldn't want to make the reference null (it'll likely cause failures elsewhere in XAPI where it assumes that PIFs always have a network).
And we probably want to keep the PIF with managed=false for reporting purposes (the interface does have an IP address, which is useful to be able to see in the API).
Draft, needs some actual testing, I've only compile tested this so far.