Operating System: All
Platform: All

Strange indeed... but it looks like a clash between current.config (yes, its should override the configs) and feature-config-pusher.

This failure occurs every time you try to start a reconfigured controller ? Or was just a one time thing ?

I tried it a couple times - happened each time. You should be able to reproduce it.

The feature config pusher and persistent config pusher are completely disconnected and do their own thing on startup.

It looks like the feature config pusher needs to drive it and merge the current config in some way. When a config from a feature is obtained, look for a corresponding config in the current config and replace it to preserve the current behavior. That seems fairly straightforward. The one tricky part I see is how to handle a netconf connector that was pushed via restconf and not via a feature.

The bigger issue is with upgrades. The current config system implementation is not upgradeable. For Li, the core mdsal and clustering wiring has changed and is incompatible with He. So if the netconf-connector was installed and a connector config was pushed via restconf in He, upgrade to Li will break.

The main problem is that it puts every module config in the current xml file even though 1 was added/changed. It would be nice if a restconf push only wrote the config that was added/changed but the way it's currently implemented generically using netconf and JMX, that looks problematic. Netconf is designed to get the entire config, merge the changes and commit.

A solution that comes to mind, at least for the short term, is that if you change wiring that makes it incompatible with previous versions, then you have to create a new config XML file (and bump the yang version) and ship that in the feature XML. As long as the feature config pusher drives the whole thing then the old file in initial would be ignored as well as the old config in the the current XML file. So, eg, 05-clustering.xml would become 06-clustering.xml with a new yang version. Assuming this works, I think that would get us over the upgrade hump from He to Li.

Pushed preliminary draft https://git.opendaylight.org/gerrit/#/c/19477 to to remove the current config push by the config persister and expose the Persister instance from the ConfigPusher interface so the feature config pusher can access it.

Expanding on the idea of creating a new config yang with a new version when incompatible changes are made, we could introduce the concept of an obsoleted capability (or config yang model). At a high level, if a config depends on an obsoleted capability then drop it. The feature config pusher could push the persisted configs first before pushing any feature configs. If a config from a feature was already pushed via persistence, then ignore it so it isn't pushed twice.

Hi Tom,

Could you grant me access to the draft you are mentioning in your comment ?

Thanks
Maros

The underlying cause for the CDS restart failure is outlined in https://bugs.opendaylight.org/show_bug.cgi?id=3156. However this bug is still valid - having both pushers doing the same thing on startup results in each module restarting on startup which can lead to sporadic system instability (also increases startup time).

Hi Tom,

Done some digging:

• The concurrent push from current and feature pusher should be harmless in case where user only added new netconf connecter. Heres why: They both configure the same modules with the same configuration and the default behavior of modules/config-subsystem is that if the configuration did not change, the module is not recreated, the previous instance is kept. This means that if feature-pusher configured CDS first, the current pusher configuration for CDS was ignored. Basically the only thing from current that would be used is the configuration for new netconf connector. If this mechanism was not present, we might see this issue sooner for different modules. So it looks like the check whether the configuration in CDS is the same was always returning false. This was already fixed by Moiz (https://git.opendaylight.org/gerrit/#/c/19957/) and with this fix the clash between current and feature pusher should be harmless. (I was unable to reproduce the issue with his fix)

• Moiz had to override the canReuse most likely because the canReuse does not work properly for DTOs .. if there is a DTO for config attributes, the instances of the DTO are not compared for equality properly. So instead returning always true (as Moiz did) we should fix the equals for DTOs because right now it would be impossible to reconfigure CDS.

• Back to the clash of current vs feature pusher. The clash is potentially harmless only when users change the configuration of modules spawned by default. And thats because it might trigger instantiation and sudden reconfiguration of modules. This again should not be harmless for most modules but could cause issues with modules like CDS where the initialization takes quite some time and sudden reconfiguration might be problematic. So we should address the clash ... with the time for Lithium left I can see 2 options for fixing the clash:
  + In case both feature and current are present, current wins over feature with warnings from feature pusher
  + Current config will be changed so that it stores the current feature list as well and the feature pusher will ignore all features being installed that are mentioned in the current config snapshot. The other features will be scanned for config files and pushed as usual.

What do you say ?

Excellent analysis.

Yes - we changed CDS to always return true from canReuseInstance so it works now. You are correct about the config properties DTO's causing a false inequality in isSame in the generated code. That would be good to address but probably not in time for Li release. Hopefully SR1. Is there a bug for that already?

To preserve current functionality, current needs to win over configs that are also pushed via features.

As you mentioned, we should prevent 2 pushes when a config feature differs from current or it has config DTO's like CDS that cause false inequality. Doing 2 pushes would increase startup time and may cause startup instability as some app modules may not close or restart gracefully (app devs may not test dynamic restarts and I don't think it's a requirement for all apps to be "restartable").

Your proposal looks reasonable - that's essentially what I was thinking.

Making the mechanism automatically upgradable is another issue and not something we can address in time for Li unfortunately. That will take a lot more thought.

Regarding the missing hashCode/equals, I have already proposed a patch:
https://git.opendaylight.org/gerrit/#/q/message:BUG-2976

I pushed it as part of this bug, I didnt open another one.

https://git.opendaylight.org/gerrit/#/c/20656/1

Current config persister now stores the list of features from karaf with the config snapshot. Feature config pusher then checks that list whenever a new feature is installed.