Skip to content

Filter out link IPv6 when migrating VMs #5463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Filter out link IPv6 when migrating VMs #5463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions ocaml/xapi/xapi_host.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ let with_lock = Xapi_stdext_threads.Threadext.Mutex.execute

module Unixext = Xapi_stdext_unix.Unixext
open Xapi_host_helpers
open Xapi_pif_helpers
open Db_filter_types
open Workload_balancing

Expand Down Expand Up @@ -2555,14 +2556,10 @@ let migrate_receive ~__context ~host ~network ~options:_ =
let configuration_mode =
Db.PIF.get_ipv6_configuration_mode ~__context ~self:pif
in
match Db.PIF.get_IPv6 ~__context ~self:pif with
match Xapi_pif_helpers.get_non_link_ipv6 ~__context ~pif with
| [] ->
("", configuration_mode)
| ip :: _ ->
(* The CIDR is also stored in the IPv6 field of a PIF. *)
let ipv6 =
match String.split_on_char '/' ip with hd :: _ -> hd | _ -> ""
in
| ipv6 :: _ ->
(ipv6, configuration_mode)
)
in
Expand Down
14 changes: 13 additions & 1 deletion ocaml/xapi/xapi_pif_helpers.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,22 @@ let is_device_underneath_same_type ~__context pif1 pif2 =
in
get_device_info pif1 = get_device_info pif2

let get_non_link_ipv6 ~__context ~pif =
let valid_nonlink ipv6 =
let open Ipaddr.V6 in
ipv6
|> Prefix.of_string
|> Result.to_option
|> Fun.flip Option.bind @@ fun cidr ->
let addr = Prefix.address cidr in
match scope addr with Ipaddr.Link -> None | _ -> Some (to_string addr)
in
List.filter_map valid_nonlink (Db.PIF.get_IPv6 ~__context ~self:pif)

let get_primary_address ~__context ~pif =
match Db.PIF.get_primary_address_type ~__context ~self:pif with
| `IPv4 -> (
match Db.PIF.get_IP ~__context ~self:pif with "" -> None | ip -> Some ip
)
| `IPv6 ->
List.nth_opt (Db.PIF.get_IPv6 ~__context ~self:pif) 0
List.nth_opt (get_non_link_ipv6 ~__context ~pif) 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this and would prefer to match over this and proper error handling for the [] case (which should never happen but will be difficult to find if it does).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, my understanding is that no IPv6 would result in [""] instead of an empty list so i'm not sure what error could be thrown here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want throw a failtwith __LOC__ if you think this can never happen. But we want to make it easy to find the location if it does happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing the code, i'm not sure throwing here is better. For me, the get_non_link helper is used to have reachable IPv6 from the outside and thus can return an empty list in the case there's none.
Then we mimic the IPv4 case above:

    match Db.PIF.get_IP ~__context ~self:pif with "" -> None | ip -> Some ip

And then it's up to the caller to decide if None is an error case or not like here: https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/xapi_network_attach_helpers.ml#L109-L114