Skip to content
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.

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

Add empty IP address check to preflight check #18425

Merged
merged 2 commits into from
Feb 15, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 1, 2019

With Linux virtual machines, we have at least two known cases where the IP address for an NIC is empty: 1. ghost config files(ifcfg-*) ; 2. the NIC has more than one IPv4 or one IPv6 address on the NIC. This leads to generating network mappings for conversion with nil addresses, which in turn causes the conversion to fail, as virt-v2v-wrapper can't handle nil addresses.

This PR adds a preflight check on the transformation task to verify that no interface in the mapping has a nil address. The goal is to fail early in the transformation process and not even stop the source VM, preserving the source environment.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1671719

@ghost
Copy link
Author

ghost commented Feb 1, 2019

@miq-bot add-label transformation, enhancement, hammer/yes
@miq-bot add-reviewer jameswnl, jerryk55, djberg96

@miq-bot
Copy link
Member

miq-bot commented Feb 1, 2019

@fdupont-redhat 'jameswnl, jerryk55, djberg96' is an invalid reviewer, ignoring...

@jerryk55
Copy link
Member

jerryk55 commented Feb 7, 2019

@fdupont-redhat do you want a message with the 'raise'? Otherwise looks fine.

@ghost
Copy link
Author

ghost commented Feb 7, 2019

@jerryk55 I don't want a message at the moment. We also works on enhanced error reporting and the message structure is not decided yet. I will add a message in another PR, along with others ;)

@jerryk55
Copy link
Member

jerryk55 commented Feb 7, 2019

@fdupont-redhat sounds good.

@ghost

This comment has been minimized.

@miq-bot

This comment has been minimized.

@miq-bot

This comment has been minimized.

@miq-bot

This comment has been minimized.

@miq-bot

This comment has been minimized.

@djberg96
Copy link
Contributor

👍

@miq-bot
Copy link
Member

miq-bot commented Feb 11, 2019

@fdupont-redhat 'jameswnl, jerryk55, djberg96' is an invalid reviewer, ignoring...

@JPrause
Copy link
Member

JPrause commented Feb 11, 2019

@fdupont-redhat was this PR reviewed and ready for merge. We need this for an upcoming hammer build.

@ghost
Copy link
Author

ghost commented Feb 12, 2019

@jerryk55 @jameswnl @djberg96 could you do a last review and merge if everything's ok ?

@djberg96
Copy link
Contributor

@miq-bot add_reviewer @jerryk55

@miq-bot miq-bot requested a review from jerryk55 February 13, 2019 12:57
@djberg96
Copy link
Contributor

@miq-bot add_reviewer @jameswnl

@miq-bot miq-bot requested a review from jameswnl February 13, 2019 12:58
@djberg96
Copy link
Contributor

@miq-bot add_reviewer @djberg96

@miq-bot miq-bot requested a review from djberg96 February 13, 2019 12:59
@agrare agrare self-assigned this Feb 13, 2019
@@ -54,7 +54,7 @@ def task_active
def preflight_check
destination_cluster
virtv2v_disks
network_mappings
network_mappings.each { |m| raise if m.ip_address.nil? }
Copy link
Member

Choose a reason for hiding this comment

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

@fdupont-redhat it looks like most of the existing validation is done in the calculate_abcd methods, would be more consistent to do this check down here

raise "[#{source.name}] NIC #{nic.device_name} [#{source_network.name}] has no mapping." if destination_network.nil?

@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2019

Checked commits fabiendupont/manageiq@cf83a9b~...3eb1681 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 awesome @fdupont-redhat !

@agrare agrare merged commit 3d26260 into ManageIQ:master Feb 15, 2019
@agrare agrare added this to the Sprint 105 Ending Feb 18, 2019 milestone Feb 15, 2019
@ghost ghost deleted the v2v_preflight_nic_ip_address branch February 15, 2019 18:37
simaishi pushed a commit that referenced this pull request Feb 18, 2019
…dress

Add empty IP address check to preflight check

(cherry picked from commit 3d26260)

https://bugzilla.redhat.com/show_bug.cgi?id=1678376
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit d0ee8411a35d08aba5578eeb34791597e666ba53
Author: Adam Grare <[email protected]>
Date:   Fri Feb 15 11:25:33 2019 -0500

    Merge pull request #18425 from fdupont-redhat/v2v_preflight_nic_ip_address
    
    Add empty IP address check to preflight check
    
    (cherry picked from commit 3d262608700abe96aa0659a506660fcf161204d3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1678376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants