fail with useful error on unmatched interface (bsc#990745) #575

aspiers · 2016-07-27T17:37:54Z

If we fail to find an admin interface matching a pattern such as +10g1 for a given node, don't proceed blindly, but instead bail with a helpful error message. This was experienced in a scenario where the Crowbar nodes were a mix of physical machines and VMs with slow virtio interfaces, but the network.json was incorrectly configured and the admin network conduit failed to match the slow virtio interfaces.

Also add a warning whenever a conduit cannot be matched against available interfaces, to help more quickly identify incorrectly configured network.json files, and de-obfuscate the code with comments and better variable names.

https://bugzilla.suse.com/show_bug.cgi?id=990745

houndci-bot · 2016-07-27T17:38:00Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

-                map_if_ref(if_remap, if_ref)
+                r = map_if_ref(if_remap, if_ref)
+                if r.nil?
+                  raise "Unable to find interface for #{node.fqdn} matching #{if_ref} out of #{if_remap}"


Metrics/LineLength: Line is too long. 105/100

aspiers · 2016-07-27T17:40:03Z

In particular, I need someone to confirm that there aren't any scenarios where it's OK to have an unmatched interface pattern. Because if there are, obviously this will break them.

vuntz · 2016-07-28T08:16:43Z

I might be wrong, but I think it's actually okay to have this be nil in some cases, as not all conduits are used on all nodes. So I think your patch is too aggressive here. You can try setting something crazy on the storage network (which is not used unless you deploy swift or ceph), and see if the exception is triggered.

houndci-bot · 2016-07-28T11:35:54Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+      # intf_to_if_map precalculated by #build_node_map if you are
+      # calling this in a loop and want to avoid it being calculated
+      # on each call.
+      # 


Style/TrailingWhitespace: Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

aspiers · 2016-07-28T11:39:15Z

@vuntz I think you're probably right. I've drastically improved the PR.

aspiers · 2016-08-01T09:46:05Z

Not convinced that gating failure was related to the changes - forced a rebuild.

Itxaka · 2016-08-01T13:38:00Z

lgtm +1

aspiers · 2016-08-11T17:29:21Z

@rhafer Since @vuntz isn't around, your review on this would be appreciated!

rhafer · 2016-08-12T07:42:50Z

@aspiers Yes, I have this on my radar. Just didn't find the time yet to give it the attention it deserves :(

houndci-bot · 2016-08-12T11:17:20Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+          desired.downto(0,&filter)
+        when "?"
+          (desired..speeds.length).each(&filter)
+          desired.downto(0,&filter) unless found


Style/SpaceAfterComma: Space missing after comma. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)

aspiers · 2016-08-12T11:19:30Z

@rhafer OK thanks :) Rebased, slightly improved one of the comments, and fixed some broken indentation.

rhafer · 2016-08-16T09:28:55Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+                r = map_if_ref(if_remap, if_ref)
+                if r.nil?
+                  Chef::Log.warn "Unable to find interface for #{node.fqdn} " \
+                                 "matching #{if_ref} out of #{if_remap}"


As @vuntz already mentioned it's perfectly fine to have this nil here for some of the conduits. As long as that conduit is not used on the node (i.e. not network using that conduit is allocated/enabled on that node). I such a case this will cause a lot of unnecessary warnings (build_node_map is called quite frequently) in the logs. I fear that it will just confuse people.

Wouldn't be better to throw a more useful error message for cases where the conduit is actually used on a node (e.g. here: https://github.com/crowbar/crowbar-core/blob/master/chef/cookbooks/network/recipes/default.rb#L161)

Or even when trying to allocate/enable a network already (i.e. in the rails app)

I such a case this will cause a lot of unnecessary warnings (build_node_map is called quite frequently) in the logs. I fear that it will just confuse people.

Yeah OK, fair point.

Wouldn't be better to throw a more useful error message for cases where the conduit is actually used on a node

Sure, that's why this PR also adds this error.

Or even when trying to allocate/enable a network already (i.e. in the rails app)

You mean, somewhere like in NetworkService#allocate_ip_by_type? That seems to get all its network info from the databag.

@rhafer Would it be good enough to change this Chef::Log.warn into a .debug? Also, I'm still not sure exactly where else you are suggesting that errors should be added if the interface is nil? (See question above.)

Would it be good enough to change this Chef::Log.warn into a .debug?

Yeah, I guess that's fine.

You mean, somewhere like in NetworkService#allocate_ip_by_type?

Yes. And NetworkService#enable_interface. The advantage of doing proper checks there would be that we could actually fail with an error much earlier than during the chef-client run. (Note: I am not say that you should be the one implementing that, it just an idea that I had when reviewing your code)

That seems to get all its network info from the databag.

Yeah, it would need to be enhanced to call into NodeObject#build_node_map. Which reminds me that might want to add the debug message there as well.

OK, it's now .debug.

Based on our follow-on IRC discussion about this, I've created Add network interface validation to NetworkService#build_net_info (Trello).

aspiers · 2016-09-07T08:51:03Z

bsc#990745 - Installing new node; fails to start install

rhafer · 2016-09-08T11:36:05Z

+1

@aspiers I guess you want to remove the "wip" and "do not merge labels"?

aspiers · 2016-09-09T16:03:43Z

Rebased to fix conflict.

It's amazing that this critical part of Crowbar's core code has remained cryptic for so long. Let's make a start at documenting it so that future generations don't suffer the same pain.

... ever!

This isn't technically a bug, because in Ruby: a, b, c = [nil, nil] will result in c being assigned nil. However it's more correct to be consistent in the number of elements the method returns.

If we fail to find an interface matching a pattern such as "+10g1" for a given node, give some useful debugging. We don't make it a warning, because there are cases where we expect this to happen, e.g. with rarely used networks such as 'os_sdn'. This helps more quickly identify incorrectly configured network.json files, and was experienced in a scenario where the Crowbar nodes were a mix of physical machines and VMs with slow virtio interfaces. https://bugzilla.suse.com/show_bug.cgi?id=990745

Catch the case where we failed to find an admin interface for a node, and bail with a more helpful error message. This was experienced in a scenario where the Crowbar nodes were a mix of physical machines and VMs with slow virtio interfaces, but the network.json was incorrectly configured and the admin network conduit failed to match the slow virtio interfaces. https://bugzilla.suse.com/show_bug.cgi?id=990745

"rescue nil" is not only a really bad habit, but it's completely unnecessary when looking up a value in a Hash.

aspiers · 2016-09-21T21:46:53Z

Failed gating build has been deleted, so rebased to trigger another one.

aspiers · 2016-11-06T20:22:41Z

Bah, @vuntz completely rewrote this so rebasing is going to be ugly... One nice thing about gerrit is that it warns you when reviews conflict with each other. #githubsucks

aspiers · 2016-11-06T20:29:13Z

@vuntz I had a quick attempt to rebase and gave up as everything looks different. Let's fix this together this week.

vuntz · 2016-11-18T08:20:09Z

chef/cookbooks/provisioner/recipes/update_nodes.rb

+    if interface.nil?
+      raise "Failed to find admin node interface for node #{node.fqdn}! " \
+            "Check your network barclamp proposal."
+    end


Thinking more about this: no objection about this part, as it's no regression, but I think it's not enough. We do want to avoid a crash in chef-client here, as having a rogue node could result, thanks to this, in a DoS on part of the deployment infrastructure. I'll submit a patch.

aspiers · 2016-12-22T11:55:53Z

@vuntz @rhafer Please can you either rebase this or help me rebase it? I put a lot of effort into adding useful comments to make cryptic code readable, but since you rewrote things completely by moving a lot of stuff to Crowbar::ConduitResolver I would have to almost start from scratch, and I don't know what other interface changes you introduced during this move.

vuntz · 2017-01-06T10:54:32Z

Obsoleted by #803

This is a forward-port of the first commit of crowbar#575 (893bfbb) in an attempt to make this code even clearer. crowbar#575 never got merged because it took too long to review it, and in the meantime all of this code got significantly rewritten during extraction of the conduit resolver code into a library to be shared between Chef and Crowbar. The other (fix) part of crowbar#575 was superseded by crowbar#803.

aspiers · 2017-02-08T19:42:26Z

Looks like I'll have to redo this myself. Resubmitted as #1082.

aspiers added the do not merge yet label Jul 27, 2016

houndci-bot reviewed Jul 27, 2016
View reviewed changes

aspiers assigned rhafer and vuntz Jul 27, 2016

aspiers force-pushed the bail-on-unmatched-intf branch from 1b13bda to 058a7f5 Compare July 27, 2016 17:41

aspiers force-pushed the bail-on-unmatched-intf branch from 058a7f5 to 58c7863 Compare July 28, 2016 11:35

houndci-bot reviewed Jul 28, 2016
View reviewed changes

aspiers force-pushed the bail-on-unmatched-intf branch 2 times, most recently from b5b6dd2 to d0192ac Compare July 28, 2016 11:45

aspiers mentioned this pull request Jul 28, 2016

Avoid duplicating network definitions in multiple locations #510

Merged

aspiers removed the do not merge yet label Aug 11, 2016

aspiers force-pushed the bail-on-unmatched-intf branch from d0192ac to 65bdf9f Compare August 12, 2016 11:17

houndci-bot reviewed Aug 12, 2016
View reviewed changes

aspiers force-pushed the bail-on-unmatched-intf branch from 65bdf9f to 12da87d Compare August 12, 2016 11:18

rhafer reviewed Aug 16, 2016
View reviewed changes

aspiers force-pushed the bail-on-unmatched-intf branch from 12da87d to 8afe7e2 Compare September 8, 2016 10:17

aspiers removed do not merge yet wip labels Sep 9, 2016

aspiers force-pushed the bail-on-unmatched-intf branch from 8afe7e2 to 72888c1 Compare September 9, 2016 16:03

Adam Spiers added 7 commits September 21, 2016 22:45

add comments to help explain cryptic code

893bfbb

It's amazing that this critical part of Crowbar's core code has remained cryptic for so long. Let's make a start at documenting it so that future generations don't suffer the same pain.

don't use cryptic 1- or 2-byte variable names

498bfb1

... ever!

make #lookup_interface_info always return a 3-element Array

b434a5d

This isn't technically a bug, because in Ruby: a, b, c = [nil, nil] will result in c being assigned nil. However it's more correct to be consistent in the number of elements the method returns.

remove unnecessary rescue nil

8d84239

"rescue nil" is not only a really bad habit, but it's completely unnecessary when looking up a value in a Hash.

fix indentation

15bc4b2

aspiers force-pushed the bail-on-unmatched-intf branch from 72888c1 to 15bc4b2 Compare September 21, 2016 21:46

vuntz approved these changes Nov 18, 2016

View reviewed changes

aplanas approved these changes Nov 18, 2016

View reviewed changes

vuntz reviewed Nov 18, 2016

View reviewed changes

vuntz mentioned this pull request Nov 18, 2016

provisioner: Avoid crashing if node has incomplete admin network mapping #803

Merged

aspiers assigned rhafer Dec 22, 2016

vuntz closed this Jan 6, 2017

aspiers deleted the bail-on-unmatched-intf branch February 8, 2017 19:04

aspiers mentioned this pull request Feb 8, 2017

deobfuscate the network conduit resolver #1082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fail with useful error on unmatched interface (bsc#990745) #575

fail with useful error on unmatched interface (bsc#990745) #575

aspiers commented Jul 27, 2016 •

edited

houndci-bot Jul 27, 2016

aspiers commented Jul 27, 2016

vuntz commented Jul 28, 2016

houndci-bot Jul 28, 2016

aspiers commented Jul 28, 2016

aspiers commented Aug 1, 2016

Itxaka commented Aug 1, 2016

aspiers commented Aug 11, 2016

rhafer commented Aug 12, 2016

houndci-bot Aug 12, 2016

aspiers commented Aug 12, 2016

rhafer Aug 16, 2016 •

edited

aspiers Aug 23, 2016

aspiers Sep 7, 2016

rhafer Sep 7, 2016

aspiers Sep 8, 2016

aspiers commented Sep 7, 2016

rhafer commented Sep 8, 2016

aspiers commented Sep 9, 2016

aspiers commented Sep 21, 2016

aspiers commented Nov 6, 2016

aspiers commented Nov 6, 2016

vuntz Nov 18, 2016

vuntz Nov 18, 2016

aspiers commented Dec 22, 2016

vuntz commented Jan 6, 2017

aspiers commented Feb 8, 2017

fail with useful error on unmatched interface (bsc#990745) #575

fail with useful error on unmatched interface (bsc#990745) #575

Conversation

aspiers commented Jul 27, 2016 • edited

Choose a reason for hiding this comment

aspiers commented Jul 27, 2016

vuntz commented Jul 28, 2016

Choose a reason for hiding this comment

aspiers commented Jul 28, 2016

aspiers commented Aug 1, 2016

Itxaka commented Aug 1, 2016

aspiers commented Aug 11, 2016

rhafer commented Aug 12, 2016

Choose a reason for hiding this comment

aspiers commented Aug 12, 2016

rhafer Aug 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers commented Sep 7, 2016

rhafer commented Sep 8, 2016

aspiers commented Sep 9, 2016

aspiers commented Sep 21, 2016

aspiers commented Nov 6, 2016

aspiers commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers commented Dec 22, 2016

vuntz commented Jan 6, 2017

aspiers commented Feb 8, 2017

aspiers commented Jul 27, 2016 •

edited

rhafer Aug 16, 2016 •

edited