Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add napalm commit confirmed features #48909

Merged
merged 8 commits into from Aug 3, 2018

Conversation

Projects
None yet
4 participants
@mirceaulinic
Copy link
Member

commented Aug 3, 2018

What does this PR do?

In line with my previous PR #48779, adding now two new features revert_in and revert_at that would schedule a configuration revert, similar to what we have on Junos (where is called "commit confirmed"). I opted to use these names as I find it makes it more obvious that the configuration will be reverted unless instructed otherwise.

The main problem is not the naming nevertheless, but the fact that not all the platforms support this natively (only Junos AFAIK). However, via Salt this would be available on any platform, but with some caveats and the user must be very cautious when using this (not that the feature itself can cause issues, but to have the right expectations when using this).

Usage example:

$ sudo salt veos-lab net.load_config text='ntp server 172.17.17.2' revert_in=2m
veos-lab:
    ----------
    already_configured:
        False
    comment:
        The commit ID is: 20180802145541882500.
        This commit will be reverted at: 2018-08-02T14:57:49, unless confirmed.
        To confirm the commit and avoid reverting, you can execute:

        salt veos-lab net.confirm_commit 20180802145541882500
    diff:
        @@ -5,6 +5,7 @@
         transceiver qsfp default-mode 4x10G
         !
         ntp server 172.17.17.1
        +ntp server 172.17.17.2
         !
         snmp-server location Tampa, FL
         !
    loaded_config:
    result:
        True
$ sudo salt veos-lab net.confirm_commit 20180802145541882500
veos-lab:
    ----------
    comment:
        Commit #20180802145541882500 confirmed.
    result:
        True

With this PR, I am also:

  • Cleaning up the schedule after the job is executed (so we don't endup with tens of scheduled jobs)
  • Store the config schedule to be loaded into a temporary file from where the Minion will read it, as in opposite to storing this in the schedule directly - inefficient especially as the config can be huge.
  • Remove the temporary files after using them.
  • Add the commit_confirmed and commit_cancelled State functions to the netconfig State.
  • The functions would return a commit_id key which can be used using the __slots__ feature to be sent to the state functions mentioned above.

@rallytime rallytime added the Fluorine label Aug 3, 2018

@rallytime rallytime requested review from gtmanfred and rallytime Aug 3, 2018

@cachedout
Copy link
Collaborator

left a comment

CodeClimate has a few ideas about some possible refactoring for DRY and breaking up some of the nested logic but otherwise this looks good!

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2018

Hi @cachedout - I looked through the warnings from codeclimate, but I couldn't identify which one I could fix (some are not relevant to these changes though)...

@rallytime rallytime merged commit 64fa32a into saltstack:develop Aug 3, 2018

7 of 9 checks passed

codeclimate 12 issues to fix
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@mirceaulinic mirceaulinic deleted the mirceaulinic:napalm-commit-confirmed branch Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.