Add commit confirmed option to optional_args

[kderynski]

Can we add commit confirmed option to optional_args ? It may be useful in some cases and it is easy to implement. I think only junos supports real commit confirmed functionality so I assume it would be better to implement it as local optional argument.

Example implementation:

Two optional arguments added to driver's init function:

        self.junos_commit_confirmed = optional_args.get('junos_commit_confirmed', 0)
        self.junos_commit_wait = optional_args.get('junos_commit_wait', 0)

and little change in commit method:

       if self.junos_commit_confirmed:
            self.device.cu.commit(confirm=self.confirm)
            if self.junos_commit_wait:
                sleep(int(self.junos_commit_wait))
        self.device.cu.commit()

Does it make sense? :)

[mirceaulinic]

On the longer term, we want to add this option directly to the commit method, i.e.: you'd be able to do: device.commit(confirmed=30). A change as above would not make much sense to me, as doing so you'll have only commit confirmed during that session. So you'd go into an infinite loop of commit confirmed:

device.open(..., optional_args={'junos_commit_confirmed': True})
device.load_merge_candidate(...)
device.commit()  # yes, here I want to be commit confirmed
device.commit()  # eeerr... how can I make this one unconfirmed now?

Does this make sense to you?

[kderynski]

I was thinking about function scope of this functionality.

I'd like to clarify this. If you define confirmed option in optional_args whole confirmation process would be performed inside commit_config() function.

For example (logic during commit() execution)

    def commit_config(self):
        """Commit configuration."""
        # confirmed optional args were used
        if self.junos_commit_confirmed:
            # executing commit with confirm option
            self.device.cu.commit(confirm=self.confirm) 
            if self.junos_commit_wait:
                # optional wait time
                sleep(int(self.wait))
       # if I can connect to device commit will be confirmed 
       # if not exception is raised and device will perform rollback by itself
        self.device.cu.commit() 
        if not self.config_lock:
            self._unlock()

I have tested this concept and it seems to be working as expected. If optional_arg is not defined code will be executed in the same way as usual.

[mirceaulinic]

I know what you mean.

How would you be able distinguish between a commit confirmed and a straight commit over the same session?

[kderynski]

Ah ok, you are right. In this approach, it would be defined per session not per commit_confg() execution, so there is no way to distinguish it. I was thinking about use case where we have devices without OOB network connectivity and those devices will be in high risk group and every commit against those devices should be confirmed.

[mirceaulinic]

Sure @kderynski - commit confirm is undoubtedly useful and I'm pro introducing it. But it will be via an optional arg per method, not per session, e.g.:

def commit_config(self, **kwargs):
  junos_kwargs = {}
  if 'confirmed' in kwargs:
    junos_kwargs['confirmed'] = kwargs['confirmed']
    self.device.cu.commit(junos_kwargs)

etc.

Same for other features:

• commit with message
• load config using different format preferred by the user, rather than hardcoding to text as of now
• etc.

The current blocker is the documentation. @dbarrosop can you provide some more input on this - when do you expect we can start adding features like that?

[kderynski]

@mirceaulinic definitely, it would be great to have optional arg per method. I've just tried to add functionality without any other changes. Anyway, I'm ready to help with implementation or/and testing when guidelines will be ready :)

[mirceaulinic]

Thanks @kderynski!

[dbarrosop]

I think this one should be added to the commit method itself. It's probably one of those cases where a slight change in the API makes complete sense but we will have to update all drivers.

[dbarrosop]

Btw, this will also require an extra method to confirm the operation, so I guess we will need:

def commit_config(self, revert_in=0) # revert_in is number of seconds until rollback. Don't revert if 0 def commit_confirm(self) # Confirm pending commit

[mirceaulinic]

Yes, that may be true. Although I recall on Junos you only need to do a normal commit after the confirmed one (at least from the CLI). But sure, we should investigate and see also what's the behaviour on the other drivers; also XR has some confirmed feature but I have no idea how it works.

[kderynski]

I can confirm that Junos needs normal commit after the confirmed one (also via netconf), so in this case, only commit_config needs to be extended by one additional argument.

[dbarrosop]

I'd prefer to have two different methods even if the second one calls the first one with revert_in=0, some other vendors might have a more complex logic and it's nicer to have it isolated instead of shitty ifs that are 20 lines long or require to hide the added complexity on hidden methods, cleaner code and easier to test :)

[mirceaulinic]

Yes, I agree with @dbarrosop's arguments. Makes sense to do it that way!

[mirceaulinic]

Tracked in https://github.com/napalm-automation/napalm-junos/issues/128