JunOS discard config if loading fails, fixes #1445

[indy-independence]

Discard config if there was an error. This avoids stale config sessios lingering on the device after config load fails. Discussed in issue #1445

[mirceaulinic]

Thanks for this patch @indy-independence. As discussed, would you be able to apply the same changes for the other drivers to ensure consistency?

[indy-independence]

Sorry I'm not familiar with the other device drivers and I don't have lab access to those devices, but from I know about the classic IOS one it doesn't support candidate configs in the device per se so I suppose it will not run in to this issue? Might be an issue on NX OS or IOS-XR though

[indy-independence]

The ios-xr netconf driver uses something called error_option="rollback-on-error" in the load_merge_candidate() function which sounds like it would fix this kind of thing, but I haven't tested it

[indy-independence]

@mirceaulinic is there something more I should do?

[mirceaulinic]

Not sure if you or someone else, but like I said, this needs to be implemented consistently across all the drivers @indy-independence. :)

[indy-independence]

@mirceaulinic is it a requirement that all behavior should aligned in the same PR? I mean if Arista and XR does it one way, could we change Juniper to do it the same way now or should we wait (a possibly very long time) until someone who has other devices comes and find this issue and verifies and/or changes behavior for remaining platforms in this same PR?

[mirceaulinic]

Not the same PR, but I want to ensure we don't introduce (more) inconsistent behaviour into the develop branch. If we'd do so, it'll block future releases, as you said, for a possibly very long time until someone fixes the other platforms. That said, this PR will stay open till we have the code for the other platforms (in this PR or others). Alternatively we could do what we did in the past for other similar issues: we created a separate branch where we merged the fixes for each individual platform as they've been submitted; once we had the code for all the platforms, we merged that branch into develop then eventually released. Would that work for you?

[indy-independence]

I'm not familiar with working on this project so thank you for taking the time to explain! Yes merging in to a separate branch sounds like a good plan to keep all the changes together and synchronized.
This PR and related issue specifically mentions just JunOS, should I create separate issues to investigate the behavior for other platforms?

[indy-independence]

Seems like people don't run in to this issue on other platforms, so maybe it's not an issue anywhere else other than junos-driver? Is there something in the source code that suggests other platforms do it the junos-driver way compared to the arista/iosxr way?

[mirceaulinic]

Ok, I'm fine to go ahead with this, but the problem may continue to exist on the other platforms, so I suggest to keep the issue open.

Seems like people don't run in to this issue on other platforms, so maybe it's not an issue anywhere else other than junos-driver?

@indy-independence this assumption doesn't really make a lot of sense, as until you reported it, we weren't aware of Junos either, so that's not something to rely on.