So, part of this is on danw/slavemonitor, but I think a complete fix depends on bug 724041 / bug 697370 (better live tracking of connections/devices, aka the whiteboard in Brno).


Currently (on master), when NM first sees virbr0, it has no IP address associated with it. So NM creates a connection for it with ip4.method=disabled, ip6.method=ignore and assumes that connection. (Later, when libvirt assigns an IP address to that device, NM will observe the change, but it only updates the device's NMIP4Config, not the connection's NMSettingIP4Config, so the device's alleged assumed connection is actually wrong.) Meanwhile, since we never notice that virbr0-nic has become a slave, virbr0 never manages to move out of STATE_IP_CONFIG.

With danw/slavemonitor, NM observes virbr0-nic becoming a slave of virbr0, and so virbr0 gets a chance to try stage3 again. But this time, nm_device_activate_stage3_ip4_start() will set ip4_state to IP_FAIL (because ip4.method=disabled) and nm_device_activate_stage3_ip6_start() will set ip6_state to IP_FAIL (because ip6.method=ignore), and so nm_device_activate_stage3_ip_config_start() will transition to STATE_FAILED with REASON_IP_CONFIG_UNAVAILABLE.


With both master and danw/slavemonitor, virbr0-nic never gets assigned a connection, because it also has no IP address associated with it when NM first sees it, but since it is not a master and not (at that point) a slave, nm_device_generate_connection() will throw out the generated connection and return NULL. Since nm_device_generate_connection() is only called when a device is first created, that means the device is permanently connection-less, and thus unmanaged.


We could come up with various hacks to try to address these issues, but it seems like it would be better to fix things completely, the right way. So maybe move this bug from nm-0-9-10 to nm-1-0?

That said, the first four patches on danw/slavemonitor are still good cleanups and could be committed:

    cf058ef devices: flip the ordering of priv->slaves
    5b8b81c devices: fix "slaves" property notification in release_slaves
    2b88e18 devices: improve slave release log messages
    21c8ab3 devices: refactor the link-changed code

So the original intent of pavel's 'runtime' branch was that the IP config changes *would* potentially update the IPv4/IPv6 'method' and the rest of the connection too.  eg, whenever the platform detected a change in IP configuration or even in L2 configuration, it would call NMDeviceClass->update_connection() to update the L2 parameters (bond/bridge/vlan/ethernet/whatever) and then call nm_ip4/6_config_update_setting() to update the IP configuration of the setting.

Obviously that might require mucking with some internal state if the IP method changes, but at this point, we could limit the changes to "ignore/disabled -> manual|auto".  And, I think, if something external adds IP address/routes to an auto-generated connection, we should probably just set METHOD_MANUAL until we add the RT_PROT stuff to be able to figure out if its supposed to be auto or not.

An additional way to limit the risk of regressions here would be to do this *only* for assumed connections for now?  The intent was always to apply this to all connections, but that's a bit more work to validate that.

So those commits look good, but I have a small concern with "21c8ab3 devices: refactor the link-changed code".

That commit assumes that priv->ip_iface will be !NULL and runs a strcmp() on it.  The previous code protected that strcmp() with a !NULL check.  I know that we should always have a !NULL priv->ip_iface if priv->ip_ifindex > 0.  But I'd feel more comfortable with a g_strcmp0() there.

Other than that, I think those bottom-four commits are ready to be merged.

For the record,

4 commits were merged as:

950d9fff263da19d4d9652ab5807d4e2984d63a3 devices: refactor the link-changed code
01b7bef6b4174ba89576cd6c476b3646f9d5be94 devices: improve slave release log messages
c48ba1ab10035759054da2ec54eb7b613415d8ab devices: fix "slaves" property notification in release_slaves
1f22c8859a2753d1430818104c019f9fdd7c364d devices: flip the ordering of priv->slaves

OK, repushed.

The new state of the world is that:

  - For devices with generated connections, we update their
    NMSettingIP4Config and NMSettingIP6Config when their NMIP4Config and
    NMIP6Config change. [In the libvirtd case, this is needed so that we
    don't permanently snapshot a slightly-too-early configuration of the
    device.]

  - If an inactive device's IP or slave configuration changes, NMManager
    will re-attempt connection assumption on it. [In the libvirtd case,
    this is needed in case we first notice the device before libvirt
    has added its IP config; an initial attempt to assume it would fail,
    but a second attempt after the IP address is added will succeed.]

  - We no longer allow generating a connection with no IP config on
    master devices. [This is not strictly necessary, but it seems to be
    correct, and makes things nicer; without this commit, NM will try to
    assume virbr0 before it's ready, fail, and then try again.]    

  - We track master/slave changes on all devices, even if they don't have
    a connection.

The last commit is a bit ugly, so maybe it's not the best way to do it... if we didn't care about the accuracy of the "master" property on unmanaged devices, we could probably simplify things... (Or, if we could rewrite everything to do a better job of splitting up device-state-modification and device-state-observation/reaction.)

> core: tweak NMSettingIP[46]Config generation

I'd do:

        s_ip4 = NM_SETTING_IP4_CONFIG (nm_setting_ip4_config_new ());
        ...
        return NM_SETTING (s_ip4);

 but it's up to you.


> devices: update generated connections when the underlying IP config changes

Seems a bit more complicated; what about a new 'priv->generated' for NMSettingsConnection instead, with an appropriate accessor?  That would get cleared whenever it was saved to disk or updated externally, and obviously would default to FALSE so that settings plugin originated connections wouldn't hit it.


The rest looks great, though I haven't tested it yet.  Will do that very soon.

re-pushed with those changes

+gboolean
+nm_settings_connection_get_nm_generated (NMSettingsConnection *connection)
+{
+	return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->nm_generated;
+}
+
+void
+nm_settings_connection_set_nm_generated (NMSettingsConnection *connection,
+                                         gboolean nm_generated)
+{
+	NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (connection);
+
+	priv->nm_generated = nm_generated;
+}

maybe NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->nm_generated = nm_generated?

> settings: add 'nm_generated' flag on NMSettingsConnection

I would set the 'nm_generated' flag inside nm_device_generate_connection(), i.e. the earliest possible place to mark the connection as generated.

Also, I would change nm_settings_connection_set_nm_generated() not to accept a parameter, instead always set nm_generated=TRUE.
You cannot reset the flag, but that happens implicitly inside of NMSetting (whenever the setting gets modified).

And I would have the priv->nm_generate field a tri-state internally, with TRUE, FALSE, UNSET. So that you can assert that nm_settings_connection_set_nm_generated() is only set if UNSET. And a call to nm_settings_connection_get_nm_generated() actively modifies UNSET->FALSE.


and I would move the following line immediately at the start of the {block} (because recheck_visibility might already raise notififications).

          nm_settings_connection_recheck_visibility (self);

+         priv->nm_generated = FALSE;
+
          /* Manually emit changed signal since we disconnected the handler, but
           * only update Unsaved if the caller wanted us to.



>> devices: update generated connections when the underlying IP config changes


-nm_device_uses_generated_connection (NMDevice *self)
+nm_device_get_connection_nm_generated (NMDevice *self)

(or something better, so that the name indicates, that this is just a shorthand for accessing a similarly named property inside NMSettings)




+         if (nm_device_uses_generated_connection (self)) {

maybe move the replacing of the setting up, before raising other signals? (not sure about this).



>> core: re-attempt connection assumption when the device state changes

 
+static gboolean nm_device_uses_generated_connection (NMDevice *self);
+

How about move the whole function up in the previous commit, so that this forward declaration is not necessary.




You could split this commit, one that just adds the RECHECK_ASSUME signal, one that uses it.



trailing space in src/nm-manager.c :)





>> devices: observe externally-caused master/slave changes (rh #1066706)


-    g_object_notify (G_OBJECT (device), "slaves");
+    g_object_notify (G_OBJECT (device), NM_DEVICE_BOND_SLAVES);



+    } else if (priv->enslaved && !info->master) {
-    } else if (priv->enslaved && !info->master)
+         nm_device_release_one_slave (priv->master, device, FALSE);






All in all, this looks solid to me

(In reply to comment #9)
> > settings: add 'nm_generated' flag on NMSettingsConnection
> 
> I would set the 'nm_generated' flag inside nm_device_generate_connection(),
> i.e. the earliest possible place to mark the connection as generated.

the connection inside nm_device_generate_connection() is a (plain) NMConnection, whose settings later get copied to an NMSettingsConnection created by the active settings plugin. So, we can either (a) add an nm_generated argument to nm_settings_add_connection() and pass it all the way down into the plugins [which I considered, but gave up on], or (b) do what I ended up doing.

I agree with the rest of your comments on set_nm_generated() though.

(In reply to comment #10)
> (In reply to comment #9)
> > > settings: add 'nm_generated' flag on NMSettingsConnection
> > 
> > I would set the 'nm_generated' flag inside nm_device_generate_connection(),
> > i.e. the earliest possible place to mark the connection as generated.
> 
> the connection inside nm_device_generate_connection() is a (plain)
> NMConnection, whose settings later get copied to an NMSettingsConnection
> created by the active settings plugin. So, we can either (a) add an
> nm_generated argument to nm_settings_add_connection() and pass it all the way
> down into the plugins [which I considered, but gave up on], or (b) do what I
> ended up doing.

Ah, I missed that point. Then I agree with how you did it :)

(In reply to comment #9)

fixed everything except:

> And I would have the priv->nm_generate field a tri-state internally

Didn't do that. Didn't seem necessary.

> -nm_device_uses_generated_connection (NMDevice *self)
> +nm_device_get_connection_nm_generated (NMDevice *self)
> 
> (or something better, so that the name indicates, that this is just a shorthand
> for accessing a similarly named property inside NMSettings)

Well, it checks both (a) the device has an active connection, and (b) that connection is nm-generated. So it's not *just* an accessor for that property.

> +         if (nm_device_uses_generated_connection (self)) {
> 
> maybe move the replacing of the setting up, before raising other signals? (not
> sure about this).

Moving it up would only affect what other NM-internal bits of code see, and nothing NM internal cares about this yet, so it doesn't matter.

> You could split this commit, one that just adds the RECHECK_ASSUME signal, one
> that uses it.

i feel like in cases like this, where the new API exists only to be used in exactly one place, that it doesn't make sense to split it up.

> core: re-attempt connection assumption when the device state changes

	if (!connection) {
		nm_log_info (LOGD_DEVICE, "can't assume; no connection");
		return;
	}

Should this really be 'info' level since it might well be common occurance?  I think it should also be prefixed with the interface name.

---
Next up, we've got a problem with ordering because when virbr0-nic starts activating, we fail to find the master active-connection because virbr0 hasn't generated a connection yet.  That causes the bits in nm_device_activate_stage3_ip_config_start() that check whether the ActiveConnection has a master (which this doesn't because virbr0 hasn't been found yet) to return FALSE, so it continues on expecting IP config:

<info> (virbr0-nic): carrier is OFF
<info> (virbr0-nic): new Tun device (driver: 'unknown' ifindex: 5)
<info> (virbr0-nic): exported as /org/freedesktop/NetworkManager/Devices/4
<info> can't assume; no connection
<info> (virbr0): bridge port virbr0-nic was attached
<info> (virbr0-nic): enslaved to virbr0
<info> (virbr0-nic): device state change: unmanaged -> unavailable (reason 'connection-assumed') [10 20 41]
<info> (virbr0-nic): link connected
<info> (virbr0): link connected
<info> (virbr0-nic): device state change: unavailable -> disconnected (reason 'connection-assumed') [20 30 4
<info> Activation (virbr0-nic) starting connection 'virbr0-nic'
...
<info> Activation (virbr0-nic) Stage 3 of 5 (IP Configure Start) started...
<info> (virbr0-nic): device state change: config -> ip-config (reason 'none') [50 70 0]
<info> (virbr0-nic): device state change: ip-config -> failed (reason 'ip-config-unavailable') [70 120 5]
<warn> Activation (virbr0-nic) failed for connection 'virbr0-nic'
<info> Activation (virbr0-nic) Stage 3 of 5 (IP Configure Start) complete.
<info> (virbr0-nic): device state change: failed -> disconnected (reason 'none') [120 30 0]
<info> (virbr0-nic): deactivating device (reason 'none') [0]
<info> (virbr0-nic): device state change: disconnected -> unmanaged (reason 'none') [30 10 0]
<info> (virbr0-nic): enslaved to virbr0
<info> (virbr0-nic): link disconnected
<info> (virbr0): link disconnected
<info>     read connection 'virbr0'

ideally we'd find the master before the slave on cold start, but I guess that's a bit hard to do.  But I fear with this behavior that we might be tearing down some slave configuration because we're failing the connection?

One approach to this would be that if we can't find the master connection yet in NMSettings, that connection assumption is postponed.  Then when we get a connection-added signal from NMSettings we try to re-assume any devices that aren't yet assumed?  I'd rather not assume them, and leave them in UNMANAGED if we don't have a master, than try to start them and just fail.  Thoughts?

> core: re-attempt connection assumption when the device state changes

	unmanaged_specs = nm_settings_get_unmanaged_specs (priv->settings);
	if (nm_device_spec_match_list (device, unmanaged_specs))
		return;

could be replaced with:

    if (nm_device_get_unmanaged_flag (device, NM_UNMANAGED_USER))
        return;

---

RE the comment above, the Manager already listens to the connection-added signal, so possible this could work?

diff --git a/src/nm-manager.c b/src/nm-manager.c
index bf8be64..6f5e84f 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -1121,22 +1121,30 @@ system_create_virtual_devices (NMManager *self)
 
 static void
 connection_added (NMSettings *settings,
                   NMSettingsConnection *settings_connection,
                   NMManager *manager)
 {
        NMConnection *connection = NM_CONNECTION (settings_connection);
+       const GSList *iter;
 
        if (connection_needs_virtual_device (connection)) {
                NMSettingConnection *s_con = nm_connection_get_setting_connection (connection);
 
                g_assert (s_con);
                if (nm_setting_connection_get_autoconnect (s_con))
                        system_create_virtual_device (manager, connection);
        }
+
+       for (iter = priv->devices; iter; iter = iter->next) {
+               NMDevice *device = NM_DEVICE (iter->data);
+
+               if (nm_device_get_state (device) <= NM_DEVICE_STATE_DISCONNECTED)
+                       recheck_assume_connection (device, manager);
+       }
 }
 
 static void
 connection_changed (NMSettings *settings,
                     NMSettingsConnection *connection,
                     NMManager *manager)
 {

Created attachment 277854 [details] [review]
assume-race.patch

Turns out you can't use connection_added() to trigger assumption recheck, because assumption also creates a connection, which re-enters connection_added() and causes a double-activation.

Instead, you could use manager_device_state_changed() because once the master enters the PREPARE state, obviously it's got a master we can use.

This is a proof-of-concept patch that works for me for the startup race; I'm not ecstatic about how it turned out but it could be an approach to look at?

nm-manager.c:assume_connection()
  g_return_val_if_fail (nm_device_get_state (device) >= NM_DEVICE_STATE_DISCONNECTED, FALSE);

> devices: observe externally-caused master/slave changes (rh #1066706)
adds a stray new line to device_ip_link_changed()

+	if (!assume_connection (self, device, connection)) {
+		if (was_unmanaged) {
+			nm_device_state_changed (device,
+			                         NM_DEVICE_STATE_UNAVAILABLE,
+			                         NM_DEVICE_STATE_REASON_CONFIG_FAILED);
+			nm_device_state_changed (device,
+			                         NM_DEVICE_STATE_UNMANAGED,
+			                         NM_DEVICE_STATE_REASON_CONFIG_FAILED);
+		}
+	}
What's the reason of changing to UNAVAILABLE first?

+ nm_log_info (LOGD_DEVICE, "can't assume; no connection");

nm_log_info (LOGD_DEVICE, "(%s): can't assume; no connection", nm_device_get_iface (device));

(In reply to comment #16)
> +    if (!assume_connection (self, device, connection)) {
> +        if (was_unmanaged) {
> +            nm_device_state_changed (device,
> +                                     NM_DEVICE_STATE_UNAVAILABLE,

> What's the reason of changing to UNAVAILABLE first?

In general, you're not allowed to skip NMDevice state transitions; some setup/cleanup steps only happen at particular states, so if we skipped right from DISCONNECTED to UNMANAGED, we might leave things broken.

(In reply to comment #13)
> ideally we'd find the master before the slave on cold start, but I guess that's
> a bit hard to do.

Actually, at boot time, we *do* find the master before the slave, it's just that virbr0 is not yet assumable at the point when virbr0-nic is enslaved (because it doesn't have an IP address yet).

> But I fear with this behavior that we might be tearing down
> some slave configuration because we're failing the connection?

It is supposed to not do any active teardown if nm_device_uses_generated_connection().

> Thoughts?

Pushed a new commit "devices: don't allow assuming a slave before its master" that makes two small tweaks: (a) bail out of generate_connection() if the device is a slave but it doesn't have a master_ac; (b) at the end of stage2, if the master is using a generated connection, and has slaves that are disconnected, retry assuming them.

This seems to work for me, although I think it was working for me before too, so I may not be seeing the races the same way you do...

Doing some testing still..

But could you add this bug # to the commit message for this branch (or the merge commit) to make sure we don't loose it?

> devices: observe externally-caused master/slave changes (rh #1066706)

Missing g_object_notify (NM_DEVICE_MASTER) in device_link_changed()?  I don't think anything in NM actually listens to the property, but might as well be consistent.

In any case, this works for me on bootup with virbr0 and virbr0-nic.  I think you've nailed the activation side of things perfectly.  But...

----

$ nmcli dev
DEVICE      TYPE      STATE         CONNECTION       
virbr0      bridge    connected     virbr0           
virbr0-nic  tap       connected     virbr0-nic       

$ sudo brctl delif virbr0 virbr0-nic

$ nmcli dev
DEVICE      TYPE      STATE         CONNECTION       
virbr0      bridge    connected     virbr0           
virbr0-nic  tap       connected     virbr0-nic       

which seems wrong, especially since the logs show:

NetworkManager[420]: <info> (virbr0): bridge port virbr0-nic was detached
NetworkManager[420]: <info> (virbr0-nic): released from master virbr0

When a slave is released should we re-generate a connection and somehow apply it?  Technically that would mean we have to go to DISCONNECTED (skip DEACTIVATING since it's forced) but possibly we could suppress the deconfiguration stuff when doing this?

I should point out that obviously not recognizing slave-release is not a regression, since that's always been the case.  And the branch makes the situation better than before, so I'm open to merging this now and fixing the slave-removal thing later too.  Thoughts?

> core: re-attempt connection assumption when the device state changes
in assume_connection()
- g_return_if_fail (nm_device_get_state (device) >= NM_DEVICE_STATE_DISCONNECTED);
+ g_return_val_if_fail (nm_device_get_state (device) >= NM_DEVICE_STATE_DISCONNECTED, FALSE);

(In reply to comment #22)
> I should point out that obviously not recognizing slave-release is not a
> regression, since that's always been the case.  And the branch makes the
> situation better than before, so I'm open to merging this now and fixing the
> slave-removal thing later too.  Thoughts?

Yeah, I'm guessing if we try to fix this, there'll be another edge case, and it will take another day to figure that out, etc. We can fix it for 0.9.10.2

(In reply to comment #21)
> > devices: observe externally-caused master/slave changes (rh #1066706)
> 
> Missing g_object_notify (NM_DEVICE_MASTER) in device_link_changed()?

In the "abnormal" cases (master is unknown, or is not a type that NM thinks can be a master), we don't set priv->master. In the working case, we call nm_device_enslave_slave(), which calls nm_device_slave_notify_enslave(), which does the notification. So we're good.

(In reply to comment #24)
> (In reply to comment #22)
> > I should point out that obviously not recognizing slave-release is not a
> > regression, since that's always been the case.  And the branch makes the
> > situation better than before, so I'm open to merging this now and fixing the
> > slave-removal thing later too.  Thoughts?
> 
> Yeah, I'm guessing if we try to fix this, there'll be another edge case, and it
> will take another day to figure that out, etc. We can fix it for 0.9.10.2

Agreed.

(In reply to comment #25)
> (In reply to comment #21)
> > > devices: observe externally-caused master/slave changes (rh #1066706)
> > 
> > Missing g_object_notify (NM_DEVICE_MASTER) in device_link_changed()?
> 
> In the "abnormal" cases (master is unknown, or is not a type that NM thinks can
> be a master), we don't set priv->master. In the working case, we call
> nm_device_enslave_slave(), which calls nm_device_slave_notify_enslave(), which
> does the notification. So we're good.

Great.

Re-opening for NM exiting on an assert. It happens when you have an established ethernet connection and you add the eth0 device into a bridge. Basically:

# ip link add name BR type bridge
# brctl addif BR eth0
... NM hits assertion ...

A fix is on branch jk/external-enslave-fix.

(In reply to comment #27)
> Re-opening for NM exiting on an assert.

I think it's been closed long enough that it would make more sense to open a new bug. But doesn't really matter.

Patch looks good, alhtough:

>-	if (priv->master)
>-		g_assert_cmpstr (method, ==, NM_SETTING_IP4_CONFIG_METHOD_DISABLED);

Alternatively we could do "if (priv->master && !nm_active_connection_is_assumed(..." ?

(In reply to comment #28)
> (In reply to comment #27)
> > Re-opening for NM exiting on an assert.
> 
> I think it's been closed long enough that it would make more sense to open a
> new bug. But doesn't really matter.
> 
I was basically lazy to open a new bug :)

> Patch looks good, alhtough:
> 
> >-	if (priv->master)
> >-		g_assert_cmpstr (method, ==, NM_SETTING_IP4_CONFIG_METHOD_DISABLED);
> 
> Alternatively we could do "if (priv->master &&
> !nm_active_connection_is_assumed(..." ?
Hmm, I don't think that the connection is assumed.

Pushed to master:
c9b9229 core: do not assert when a device is enslaved externally