Dan, what do you suggest?

I'd suggest to file a bug on vmware: I think that MAC address should have been exposed in the OVA.

We have an RFE for allocating a MAC address when registering a storage domain (bug 1226206). We can have a similar RFE for v2v. Note that the APIs and UIs are quite different, so implementing this RFE would not be a trivial outcome of the the existing one.

Also would like to add that this works fine in 3.6 environment. Importing VMware VMs from ova was working fine in 3.6.

Dan, then I would prefer to treat it as a regression. It's not realistic to fix VMware's side, even more so because an OVA can be exported already and no one is going to update it.
We can either make it up in the import code or you can fix the behavior to hide that detail inside networking code(which is I guess how it worked in 3.6 then - would it then be applicable to bug 1226206 as well?)

I don't understand how this could have worked in 3.6.

nijin, could you double-check that you've tested an OVA with no MAC address with 3.6? Which MAC address was assigned by Engine to the newly-imported VM? Which MAC is assigned if you re-import the same OVA?

I tested this again in RHEV 3.6 with the same ova file which is used in RHV 4.0. The ovf configuration file in the export domain was not having MAC address. However the VM was imported without any issue and RHEV assigns a MAC address for the VM NIC.

Is the issue caused by bug 1209795 which was fixed in RHV 4.0 ?

the issue is caused by unfortunate interplay in NumberFormatException (produced by invalid input), java.lang.reflect.Proxy which will 'upgrade' runtime exception to controlled exception, and finally jdk 8 lambas, one of which fails to compile due to undeclared controlled exception.

I'm looking how to fix it, but by reading stacktrace it seems, that some code is asking, if mac address ""  (ie. empty string) is used. Empty string is not valid MAC address and failure of mac pools is correct. (it can be worthy to check, whether there's truly not MAC address, since it seems, that empty string is being sent as one.)

looking closely, this is relevant code:

final List<VmNetworkInterface> interfaces = vm.getInterfaces();
        if (interfaces == null) {
            return Collections.emptySet();
        }

…

        return interfaces
                .stream()
                .map(VmNetworkInterface::getMacAddress)
                .filter(Objects::nonNull)
                .filter(((Predicate<String>) macPool::isMacInRange).negate())
                .collect(Collectors.toSet());

So. Records from vm_interface table are read, if there are some rows, then we 'strip' result set only to column mac_addr, discard all null valued rows, and then we test if such mac is in range. So far ok.

Column 'mac_addr' is defined as 'character varying(20)' following our maxim: "db need not to test incoming values, because engine is sending them correctly"

But he's not. Please check your DB(and confirm), there must be '' as mac_addr in your vm_interface table.

==
Result: 
1) I will fix as a part of this bug throwing java.lang.reflect.UndeclaredThrowableException. That's evident bug, but fixing it will only change exception for you, and you'll endup with NumberFormatException. (that's also incorrect, InvalidMAC or something should be used instead, but that's different story). But some exception should be raised in this situation, since it reveals bugs in other parts of engine.

2) I can fix ExternalVmMacsFinder, from which you just saw excerpt, so that it filters out empty strings and saves a day in this current situation, but that's good for nothing. It just gives us some more time until we have to really fix this issue. Therefore I'd skip this and proceed directly to real fix. That would be:

- abandoning maxim, that one application layer should trust other one.
- alter all places in DB to where MACs stored, so that given column has either CHECK constraint or uses macaddr data type.

Doing so will lead us to buggy place, which stores invalid MACs into DB.
Please decide, what solution you want for point 2).

Martin, can you explain what caused the 3.6/4.0 regression?

On master branch, the right thing is to have the DB protect itself, and make sure v2v code never writes empty mac. Depending on the intrusiveness of the fix, we should decide whether it should be backported, or to add a little plaster to 4.0.

No, I can't. 

I suspect, that he has invalid db content, '' as mac in table vm_interface. Once we have this reported, the regression is *somewhere* where vm_interface is created / updated.

First we need confirmation. Then we can start the search, to be sure if it fixes this bug. Or we can fix the db, which is in invalid state for sure.

btw. backport is super easy as well. Just drop all invalid mac addresses (which will kill engine anyways, so there's no real use for them) and fix db design.

nijin, can you attach a lightweight OVA to simplify reproduction of the issue?

Martin, can you post a sql query for nijin to perform in order to check whether his db is indeed corrupt?

Hello Dan,

I don't think it's specific to an environment as two customers was also having the same issue. For both the case, adding the MAC address manually in the ovf file help them to import the VM.

to have confirmed cause of this issue, I'd like to ask to do this:

1) login to db of engine having this issue
2) issue following sql query: select mac_addr from  vm_interface;
3) send me an output of this query or identify, if there's an empty string row (ie. string is not null, but its length is zero), or other invalid MAC adress string.

or you can provide me with connection to db and I'll check myself.

Hello Martin,

Sorry for the delay. I have uploaded the VM to export domain using virt-v2v command and while importing, I was able to get the same error.

This is the output which you have requested.

engine=# select mac_addr from  vm_interface;
     mac_addr      
-------------------
 00:16:3e:5c:f4:af
(1 row)

There is only 1 VM which is HostedEngine VM.

ok, thanks. 

Question: when importing VM. Lets say, that ova file for some reason contains '' as invalid mac address. What would be desired approach? Reject invalid input (ie. import will fail) or fix this invalid mac address address to null (which would be correct value for unset mac address)?

——————————————————————————————
Notes (probably for me): Sole place calling this function looks like this:

vm.getInterfaces()
.stream()
.map(VmNetworkInterface::getMacAddress)
.filter(Objects::nonNull)
.filter(((Predicate<String>) macPool::isMacInRange).negate())
.collect(Collectors.toSet());

and 
macPool::isMacInRange
can fail for invalid mac address with 
java.lang.NumberFormatException: For input string: ""

So it has to be some place, from which data gets into vm.getInterfaces(). But for ImportVmCommand in log it's ony db function GetVmNetworkInterfaceViewByVmId. But ImportVmCommand overrides loading data from db

@Override
    public Guid getVmId() {
        if (getParameters().isImportAsNewEntity()) {
            return getParameters().getVm().getId();
        }
        return super.getVmId();
    }

    @Override
    public VM getVm() {
        if (getParameters().isImportAsNewEntity()) {
            return getParameters().getVm();
        }
        return super.getVm();
    }

so in certain case it will be VM passed by user and not one from db. There's slightly overgrown validator for mac addresses (org.ovirt.engine.core.bll.validator.ImportValidator#validateMacAddress), but that considers empty string as unset mac address, while rest of code does not. So this is another possible source of problems.

(In reply to Martin Mucha from comment #17)
> Question: when importing VM. Lets say, that ova file for some reason
> contains '' as invalid mac address. What would be desired approach? Reject
> invalid input (ie. import will fail) or fix this invalid mac address address
> to null (which would be correct value for unset mac address)?

rather set it to null so it is initialized as a new one from the pool first time it runs

(In reply to Michal Skrivanek from comment #18)
> (In reply to Martin Mucha from comment #17)
> > Question: when importing VM. Lets say, that ova file for some reason
> > contains '' as invalid mac address. What would be desired approach? Reject
> > invalid input (ie. import will fail) or fix this invalid mac address address
> > to null (which would be correct value for unset mac address)?
> 
> rather set it to null so it is initialized as a new one from the pool first
> time it runs

As Michal said in comment 5, we should maintain the pre-4.0 behavior: a fresh mac address should be allocated to the VM.

Unlike Michael, I think that it would be better to perform the allocation during import - I hate to have garbage nulls lying in the DB. But if it is considerably simpler for you Martin - do as you please.

about "maintaining pre-4.0 behavior" — that's what we probably don't want to maintain. 

I don't want to be bearer of bad news, but there isn't bll/db validation/healing for empty mac addresses in master, and it is equally missing in 3.6 (and probably previous versions as well). The code is quite wild without sufficient validity checks, so I cannot be sure, but reading code it seems, that this command(ImportVmCommand) can be used in 3.6 to import "" mac addresses. And this will probably lead to further failures (different ones than which are reported here).

following #18 we should update ImportVmCommand to 'heal' "" macaddresses in input to null, and setup db constraints (as discussed with emesika, he agrees with using macaddr postgres data type). Consider, whether this should be backported, or how we should test 3.6 if such errors can occur in 3.6 as well.

I've tried to do it on my setup and it is possible to import test.ova from https://bugzilla.redhat.com/show_bug.cgi?id=1378045#c14 and run the imported VM.

Scenario:
1. from webadmin -> virtual machines tab -> import, select "VMware virtual aplliance (OVA)" source and enter OVA path, click load. 
2. continue to next import dialog and set OS type, click OK. 
3. Run VM after import progress completed, and verify VM is running properly.

Version:
rhevm-4.0.5-0.1.el7ev
vdsm-4.18.15-1.el7ev.x86_64
libvirt-client-1.2.17-13.el7_2.6.x86_64
qemu-kvm-rhev-2.3.0-31.el7_2.22.x86_64
virt-v2v-1.28.1-1.55.el7_2.4.x86_64

nashok, could you try importing the attached OVA with rhevm-4.0.5?
Could you later help testing the suggested patch? (as QE did not reproduce the issue)

(In reply to Dan Kenigsberg from comment #22)
> nashok, could you try importing the attached OVA with rhevm-4.0.5?
> Could you later help testing the suggested patch? (as QE did not reproduce
> the issue)

I tested this in rhevm-4.0.4.4-0.1 (latest available through RHSM) and import is working good now. Not sure what has changed. However if you manually export the VM to export domain using v2v and then importing from RHEV-M portal fails with same error.

ok, so this means two things:
- ova file you imported is probably different, than one from original report. No change was merged yet, so same ova file should still equally fail.
- fix is not merged yet, and you was testing version without fix.

Also if manual export using v2v and then importing it back produces this error, it means, that there is probably error in engine somewhere, which reports empty string mac address, which is invalid behavior.  

No DB/engine fixes were done so far, related to engine reporting invalid mac address. I will fix all db code, so that empty string cannot be stored as mac, but it would be really helpful, if you can provide me with details, how to reproduce this failure ~ I'm not familiar with exporting using v2v.

(In reply to nijin ashok from comment #23)
> if you
> manually export the VM to export domain using v2v and then importing from
> RHEV-M portal fails with same error.

Since it is another flow, initiated in another location in code, please include the traceback of that error.

Also, could you double-check that the attached OVA still reproduces the bug with rhevm-4.0.3-0.1.el7ev.noarch (it might be that a wrong OVA has been attached to bugzilla by mistake)

Marina, if possible, we'd love to see another OVA that reproduces the bug.

Hello Dan,

Unfortunately I am not able to reproduce it now while directly importing the ova  even in rhevm-4.0.3-0.1.el7ev.noarch . I am not sure if the OVA file was changed in between . I will check if I can reproduce with some other VMware OVA.

Attaching the traceback of error when importing from export domain after we do v2v to export domain.

Created attachment 1218094 [details]
engine log

thanks for engine.log, but (comment #24): "it would be really helpful, if you can provide me with details, how to reproduce this failure ~ I'm not familiar with exporting using v2v.". I need this kind of information, if possible, in detail.

Hello Martin,

I used the below commands from hypervisor.

export LIBGUESTFS_BACKEND=direct

virt-v2v -i ova /tmp/test.ova -o rhev -of qcow2 -os NFS-Server-IP:/NFS-share-path --network rhevm

Where NFS-Server-IP:/NFS-share-path is your export domain.

Then tried to import the VM from the export domain.

Can you please use the external VM provider to do this import and not via the export domain? Do you see a issue using this?

Nisim, can you reproduce the problem with the steps described in comment 30

Issue reproduced: 
2016-12-01 11:06:40,427 ERROR [org.ovirt.engine.core.bll.exportimport.ImportVmCommand] (org.ovirt.thread.pool-6-thread-13) [6c0f3b55] Exception: java.lang.reflect.UndeclaredThrow
ableException

See attached reproduction_engine.log

Created attachment 1226684 [details]
reproduction_engine.log.xz

BTW, I'm not sure importing ova to export domain using virt-v2v command and then import it from export domain is the preferred way.
Importing it from webadmin --> virtual machines tab --> import --> select VMware virtual appliance (source) is easiest and safer because the user can set OS type, network interface and the import process can install virtio drivers if they are not installed on the ova file.

(In reply to Yaniv Dary from comment #31)
> Can you please use the external VM provider to do this import and not via
> the export domain? Do you see a issue using this?

I don't have any issue by directly importing from external provider. It's working fine.

(In reply to nijin ashok from comment #36)
> 
> I don't have any issue by directly importing from external provider. It's
> working fine.

Is that true for the customer as well? If so, we can lower the urgency of this issue.

Is working with webadmin --> virtual machines tab --> import --> select VMware virtual appliance (source) ?

As CEE see that using the supported flow works. Closing this as won't fix.

just a comment: wontfix may not be accurate resolution. Two problems in two merged patches were real and may actually help someone although we weren't able to reproduce the issue ...

Please note discussion of this issue on the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1506572

OVA files from VMware simply don't contain any MAC Address
and we're not going to fix that any time soon, and we cannot
fix it for OVAs which have already been generated.

oVirt is the only place with enough information to fix a
missing MAC Address.  It should generate one according to
its local policy which only oVirt knows.

(In reply to Richard W.M. Jones from comment #41)
> Please note discussion of this issue on the following bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1506572
> 
> OVA files from VMware simply don't contain any MAC Address
> and we're not going to fix that any time soon, and we cannot
> fix it for OVAs which have already been generated.
> 
> oVirt is the only place with enough information to fix a
> missing MAC Address.  It should generate one according to
> its local policy which only oVirt knows.

Richard, I believe that this is what oVirt already does.
Is there anything to be fixed here these days?

Great if it's already done then let's close this again.  However
don't close it "WONTFIX" this time :-)

Richard, don't be angry with Yaniv Lavi. It's his job, as product manager, to stop us engineers from solving bugs that the product does not care about.

Not angry with anyone :-)  However if the resolution is
"WONTFIX" then it made me think that the bug was still
unresolved.

BTW if there an upstream Gerrit / commit ID / etc for this
fix which I can look at?

*** This bug has been marked as a duplicate of bug 1503947 ***