Libguestfs - Feb 2018 - Re: [PATCH] v2v: remove MAC address related information

Remove ties to MAC address because it is likely to change.

The code is based on operations net-hwaddr and udev-persistent-net of
virt-sysprep.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/convert_linux.ml | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index b273785e6..8bba74786 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -88,6 +88,8 @@ let convert (g : G.guestfs) inspect source output rcaps     
unconfigure_kudzu ();
     unconfigure_prltools ();
 
+    sysprep_networking();
+
     let kernel = configure_kernel () in
 
     if output#keep_serial_console then (
@@ -452,6 +454,21 @@ let convert (g : G.guestfs) inspect source output rcaps    
msg
     )
 
+  and sysprep_networking () +    if family = `RHEL_family then
+      (* Remove HWADDR=... entries from ifcfg-* files. *)
+      let paths = g#aug_match
+        "/files/etc/sysconfig/network-scripts/*[label() =~
glob('ifcfg-*')]/HWADDR" in
+      let paths = Array.to_list paths in
+      if paths <> [] then (
+        List.iter (fun path -> ignore (g#aug_rm path)) paths;
+        g#aug_save ()
+      );
+    (* Remove persistent udev rules. *)
+    try g#rm "/etc/udev/rules.d/70-persistent-net.rules"
+    with G.Error _ -> ()
+
+
   and configure_kernel ()      (* Previously this function would try to install
kernels, but we
      * don't do that any longer.
-- 
2.16.1

On Tue, Feb 27, 2018 at 12:35:36PM +0100, Tomáš Golembiovský
wrote:
> Remove ties to MAC address because it is likely to change.
> 
> The code is based on operations net-hwaddr and udev-persistent-net of
> virt-sysprep.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  v2v/convert_linux.ml | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index b273785e6..8bba74786 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -88,6 +88,8 @@ let convert (g : G.guestfs) inspect source output rcaps
>      unconfigure_kudzu ();
>      unconfigure_prltools ();
>  
> +    sysprep_networking();
The function name is a bit obscure.  How about this?

  remove_network_ifcfg_hwaddr_entries ();

Also needs a space between the function name and the parentheses.
>      let kernel = configure_kernel () in
>  
>      if output#keep_serial_console then (
> @@ -452,6 +454,21 @@ let convert (g : G.guestfs) inspect source output
rcaps >              msg
>      )
>  
> +  and sysprep_networking () > +    if family = `RHEL_family then
> +      (* Remove HWADDR=... entries from ifcfg-* files. *)
> +      let paths = g#aug_match
> +        "/files/etc/sysconfig/network-scripts/*[label() =~
glob('ifcfg-*')]/HWADDR" in
> +      let paths = Array.to_list paths in
> +      if paths <> [] then (
> +        List.iter (fun path -> ignore (g#aug_rm path)) paths;
It doesn't really matter but I think you could write this as:

  List.iter (g#aug_rm |> ignore) paths;

but your version may be clearer.
> +        g#aug_save ()
> +      );
> +    (* Remove persistent udev rules. *)
> +    try g#rm "/etc/udev/rules.d/70-persistent-net.rules"
> +    with G.Error _ -> ()
Rather than throwing away the error it's better to use 'g#rm_f' and
don't try to catch G.Error at all.  I guess the same problem exists in
virt-sysprep.

Is this a fix for any particular bug?  If so it needs to have
‘(RHBZ#xxxxxx)’ added to the end of the commit heading.

Patch looks OK otherwise.

Rich.
> +
> +
>    and configure_kernel () >      (* Previously this function would try
to install kernels, but we
>       * don't do that any longer.
> -- 
> 2.16.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tuesday, 27 February 2018 12:35:36 CET Tomáš Golembiovský
wrote:
> Remove ties to MAC address because it is likely to change.
v2v tries to preserve the MAC address of network interfaces; few months
ago we did a fix regarding this:
https://bugzilla.redhat.com/show_bug.cgi?id=1506572

The approach of this patch is IMHO not good, since it removes the MAC
address from the network-scripts, but still the rest of v2v will try
to preserve the MAC addresses.

What's the reason behind this patch?

-- 
Pino Toscano

On Tue, 27 Feb 2018 11:47:38 +0000
"Richard W.M. Jones" <rjones@redhat.com> wrote:
> On Tue, Feb 27, 2018 at 12:35:36PM +0100, Tomáš Golembiovský wrote:
> > Remove ties to MAC address because it is likely to change.
> > 
> > The code is based on operations net-hwaddr and udev-persistent-net of
> > virt-sysprep.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  v2v/convert_linux.ml | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> > index b273785e6..8bba74786 100644
> > --- a/v2v/convert_linux.ml
> > +++ b/v2v/convert_linux.ml
> > @@ -88,6 +88,8 @@ let convert (g : G.guestfs) inspect source output
rcaps > >      unconfigure_kudzu ();
> >      unconfigure_prltools ();
> >  
> > +    sysprep_networking();  
> 
> The function name is a bit obscure.  How about this?
> 
>   remove_network_ifcfg_hwaddr_entries ();
Maybe too long to my taste, but sure, why not.
> 
> Also needs a space between the function name and the parentheses.
> 
> >      let kernel = configure_kernel () in
> >  
> >      if output#keep_serial_console then (
> > @@ -452,6 +454,21 @@ let convert (g : G.guestfs) inspect source output
rcaps > >              msg
> >      )
> >  
> > +  and sysprep_networking () > > +    if family = `RHEL_family
then
> > +      (* Remove HWADDR=... entries from ifcfg-* files. *)
> > +      let paths = g#aug_match
> > +        "/files/etc/sysconfig/network-scripts/*[label() =~
glob('ifcfg-*')]/HWADDR" in
> > +      let paths = Array.to_list paths in
> > +      if paths <> [] then (
> > +        List.iter (fun path -> ignore (g#aug_rm path)) paths;  
> 
> It doesn't really matter but I think you could write this as:
> 
>   List.iter (g#aug_rm |> ignore) paths;
Ah, right. The magical |> operator... I keep forgetting about it.
> 
> but your version may be clearer.
Agreed. I'll leave it as is then.
> 
> > +        g#aug_save ()
> > +      );
> > +    (* Remove persistent udev rules. *)
> > +    try g#rm "/etc/udev/rules.d/70-persistent-net.rules"
> > +    with G.Error _ -> ()  
> 
> Rather than throwing away the error it's better to use 'g#rm_f'
and
> don't try to catch G.Error at all.  I guess the same problem exists in
> virt-sysprep.
> 
> Is this a fix for any particular bug?  If so it needs to have
> ‘(RHBZ#xxxxxx)’ added to the end of the commit heading.
Not yet, but thanks for reminding me. I'll create a bug and add it to
the heading.
> 
> Patch looks OK otherwise.
> 
> Rich.
> 
> > +
> > +
> >    and configure_kernel () > >      (* Previously this function
would try to install kernels, but we
> >       * don't do that any longer.
> > -- 
> > 2.16.1
> > 
> > _______________________________________________
> > Libguestfs mailing list
> > Libguestfs@redhat.com
> > https://www.redhat.com/mailman/listinfo/libguestfs  
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-df lists disk usage of guests without needing to install any
> software inside the virtual machine.  Supports Linux and Windows.
> http://people.redhat.com/~rjones/virt-df/

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Tue, Feb 27, 2018 at 12:53:08PM +0100, Pino Toscano
wrote:
> On Tuesday, 27 February 2018 12:35:36 CET Tomáš Golembiovský wrote:
> > Remove ties to MAC address because it is likely to change.
> 
> v2v tries to preserve the MAC address of network interfaces; few months
> ago we did a fix regarding this:
> https://bugzilla.redhat.com/show_bug.cgi?id=1506572
> 
> The approach of this patch is IMHO not good, since it removes the MAC
> address from the network-scripts, but still the rest of v2v will try
> to preserve the MAC addresses.
We preserve the MAC address in metadata.  On the other hand AIUI this
patch only removes the association in the ifcfg files and the guest
will reassociate it when it boots (albeit it might then mix up the
ethernet interfaces so that's not good).

There's IMHO a bigger problem which is not being addressed:

https://bugzilla.redhat.com/show_bug.cgi?id=1318922
> What's the reason behind this patch?
There's a bit of background which is missing.  Tomáš and I had some
discussions (privately, unfortunately) with the ManageIQ developers
who are integrating virt-v2v into MIQ/CloudForms.  Their existing
software runs a separate virt-sysprep step on guests after they have
been converted by virt-v2v.  They disable all sysprep the operations
except for just a couple, including ‘net-hwaddr’, so the effect is
roughly the same as this patch.

The question was raised why they need to do that as a separate step
and why virt-v2v doesn't do it.  And indeed there was some discussion
about whether or not converted guests need a new MAC address -- it's
at best unclear -- it is thought that VMware might reuse MAC addresses
which have "left" the hypervisor, although no one knows if that's
really true or not.

I don't have much opinion on this.  Maybe it's best for CFME to
continue to run virt-sysprep as a separate step.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, 27 Feb 2018 12:53:08 +0100
Pino Toscano <ptoscano@redhat.com> wrote:
> On Tuesday, 27 February 2018 12:35:36 CET Tomáš Golembiovský wrote:
> > Remove ties to MAC address because it is likely to change.  
> 
> v2v tries to preserve the MAC address of network interfaces; few months
> ago we did a fix regarding this:
> https://bugzilla.redhat.com/show_bug.cgi?id=1506572
> 
> The approach of this patch is IMHO not good, since it removes the MAC
> address from the network-scripts, but still the rest of v2v will try
> to preserve the MAC addresses.
> 
> What's the reason behind this patch?
Reason is that the MAC address is likely to change after the conversion
because the environment uses different MAC prefix. It depends on the
management system. For example the default pool in oVirt has prefix
00:1A:4A:... registered to Qumranet Inc. I don't know what VMware has as
as default. Big companies probably have their own prefix and thus are
OK.

Sure, you can have custom MAC addresses with different prefix on your
network. I can't say if that is good idea or bad idea.

It seems to me that this dual approach is right thing to do. Remove the
reference from guest so it is ready for new MAC, but leave the old MAC
in metadata. That way the management system can decide what is the right
way to do.


    Tomas

-- 
Tomáš Golembiovský <tgolembi@redhat.com>