load config with optional config db locking

[lampwins]

Description of Issue/Question

When using one of the load config methods, the user should be able to control if the config db should be locked or not prior to the load. I propose an extension to use of the existing config_lock optional param. Currently the docs state this param is used to determine if the config is locked at connection open. I propose it is also looked at in the load methods. If config_lock is false, the config db will not be locked before a config load, if it is true, it will be checked before the load, like it is in the current implementation.

Did you follow the steps from https://github.com/napalm-automation/napalm#faq

[x] Yes [ ] No

Setup

napalm-junos version

(Paste verbatim output from pip freeze | grep napalm-junos between quotes below)

napalm-junos==0.6.6

JunOS version

(Paste verbatim output from show version and haiku between quotes below)

Hostname: lab-4300
Model: ex4300-48p
Junos: 14.1X53-D42.3
JUNOS EX  Software Suite [14.1X53-D42.3]
JUNOS FIPS mode utilities [14.1X53-D42.3]
JUNOS Online Documentation [14.1X53-D42.3]
JUNOS EX 4300 Software Suite [14.1X53-D42.3]
JUNOS Web Management Platform Package [14.1X53-D42.3]
JUNOS py-base-powerpc [14.1X53-D42.3]

        Blessed are the meek:
        They shall inherit the earth.
        Can I have the moon?

Steps to Reproduce the Issue

No matter the value of config_lock, the config always be locked when using a load config method.

[mirceaulinic]

Thanks for suggestion @lampwins.

I would propose to introduce an additional optional argument, say never_lock_config (bool). Would you be happy to submit a PR to address this?

[lampwins]

@mirceaulinic I would be more than happy to contribute this.

On second though, I agree it should be a second param. I suggest config_load_lock which will default to true, to preserve the current functionality.

If we are to do this, I also think config_lock should be renamed to open_config_lock (or similar), but that would obviously be a breaking change and outside of this PR.

[mirceaulinic]

No, renaming should not take place.

[dbarrosop]

If we are to do this, I also think config_lock should be renamed to open_config_lock (or similar), but that would obviously be a breaking change and outside of this PR.

I actually suggest "aliasing". For example, in the code we would certainly change the variable name, so we would have session_lock (current one) and config_lock (the one proposed here). I think current name is confusing and makes code hard to read. Then on init we could do:

self.session_lock = optional_args.get("session_lock", None) or optional_args.get("config_lock", None)

This would allow us to have a more consistent naming without breaking backwards compatibility.

What do you think?

[mirceaulinic]

To be honest, I'm not sure what are the benefits. That optional arg is documented http://napalm.readthedocs.io/en/develop/support/index.html#list-of-supported-optional-arguments

The explanation is obvious enough for me:

Lock the config during open() (default: False).

If we feel that is not, feel free to put there a wall of text to explain, we don't aim to have them self-explanatory (if you look at that list, all of them require reading except port: none is self-explanatory). I consider this overkill without any real value - we should have probably assigned a better name when introducing it.

But if you feel like this is one of the priorities, feel free to submit a PR here and under napalm-iosxr (the other driver using this arg).

[mirceaulinic]

Personally without DB lock on config changes is not a good idea in production anyway (even on the CLI "configure exclusive" is one of the best practices recommended): you can discard or overwrite and eventually commit someone else's changes or a different process, which often leads to unpleasant surprises. But as long as this optional arg defaults to True, I'm fine.

[dbarrosop]

The explanation is obvious enough for me:

The explanation fails to indicate that otherwise the configuration is locked during the load_*_configuration phase until the configuration is committed or discarded. Maybe that's worth mentioning.

Personally without DB lock on config changes is not a good idea in production anyway

I agree. Might be convenient though to not lock it if you just want to grab a diff.

@lampwins out of curiosity, what's the use case?

[lampwins]

Totally agree that locking the config in production is best practice. Nevertheless, I need the option just you do when do things manually on the cli.

I do have a couple of use cases for this, mainly surrounding provisioning of devices. In some cases I generate a large portion of the config but an operator needs to add some specific routing information. If the automated config were committed prematurely access to the device would be lost. So the operator needs to merge his config with what now resides on the device as the candidate config. In another use case, I have two or more automated systems that place candidate config on the device and it gets committed at a specific point in the future.

We can get into tangents on architecture of these systems (believe me I have :) ), but it remains that I need the same flexibility that a human would have in deciding whether or not the lock the db.

[dbarrosop]

I see, I'd recommend alternatives to your workflow but anyway, I understand we should be flexible enough to support you. So I am fine with reworking a tiny bit the code as suggested in https://github.com/napalm-automation/napalm-junos/issues/163#issuecomment-307028811

@mirceaulinic thoughts? @lampwins, if @mirceaulinic agrees to it, is this something you could send a PR for?

[mirceaulinic]

Yes, @lampwins please go ahead and submit this. My comment was more like a suggestion, but as David pointed out, we should be flexible enough.

[mirceaulinic]

Hi @lampwins - I am afraid i need to close this, as we no longer accept PRs and issues to this repository. After the reunification (see https://napalm-automation.net/reunification/) you can submit a PR or discuss this further under the main repo (https://github.com/napalm-automation/napalm). Thanks for understanding!