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

decoupled connections and task #35

Closed
wants to merge 3 commits into from
Closed

decoupled connections and task #35

wants to merge 3 commits into from

Conversation

Ali-aqrabawi
Copy link

@Ali-aqrabawi Ali-aqrabawi commented Jun 22, 2019

created a new pkg connection which is similar to nornir connections,
this is related to #3

@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   35.66%   35.66%           
=======================================
  Files           4        4           
  Lines         143      143           
=======================================
  Hits           51       51           
  Misses         92       92

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a27105f...4249b32. Read the comment docs.

@Ali-aqrabawi Ali-aqrabawi mentioned this pull request Jun 22, 2019
@dbarrosop
Copy link
Contributor

Thanks for the PR. However, this is not what I had in mind. A few general comments:

  1. The interface defined is good for command based interactions like ssh and telnet but it's not going to work for rest/restconf/netconf/grpc. In any case, the interface probably just needs the open/close methods and the rest will be up to type casting.
  2. The idea with handling connections is to be able to reuse the sessions across different tasks, for instance, you could connect via ssh, upload a file via sftp/scp, send a command and close the connection.

Give some time to work on a proposal for this and then we can add the gomiko connection plugin and reference tasks.

@Ali-aqrabawi
Copy link
Author

Ali-aqrabawi commented Jun 23, 2019

the Send method takes an interface which can be string, xml or json, so it should be same for all protocols, and the casting/typeAssertion done inside the implementation,

the SSHConn has *ssh.Session property which maintain the session for multiple Run Commands as well as in gomiko.

does that make sense?

@dbarrosop
Copy link
Contributor

and the casting/typeAssertion done inside the implementation

That means that Send might do different things on the same plugin depending on the arguments, which is confusing and hard to test. It also means the connection plugin is hardwired so it's harder to extend and becomes more complex to develop and maintain. It's easier/cleaner if you keep the connection plugin lean and delegate the responsibility of what to do with the session to the task.

which maintain the session for multiple Run Commands

With your implementation that's true for a single grouped task.

Let me mock something very quickly and we can take the discussion from there.

@Ali-aqrabawi
Copy link
Author

Ali-aqrabawi commented Jun 23, 2019

That means that Send might do different things

not really, its the same thing but different implementation one send a command string, other send a command xml,
same apply for open and close, one does ssh auth and other does https OAuth or something else,
so basically it's same job different implementation/protocol.
But yeah there always a better way.

With your implementation that's true for a single grouped task.

not sure about this, the implementation is exactly the same as in nornir,is multiple grouped tasks supported by nornir?

Let me mock something very quickly and we can take the discussion from there.

Cool!. Ok

@dbarrosop
Copy link
Contributor

one send a command string, other send a command xml

There is more you may want to do over a transport protocol, sending files, subscribe to events, etc. And it doesn't work well (or at all) with grpc.

not sure about this, the implementation is exactly the same as in nornir,is multiple grouped tasks supported by nornir?

The implementation doesn't look much like nornir, for instance, nornir doesn't implement the Send method precisely for the reasons stated above.

is multiple grouped tasks supported by nornir

Yes

@Ali-aqrabawi
Copy link
Author

The implementation doesn't look much like nornir, for instance, nornir doesn't implement the Send method precisely for the reasons stated above.

i mean for sending multiple commands on same connection, nornir have a connection plugging for paramiko and netmiko, the open connect to the device and maintain the connection to be used later, which is exactly the same in the pr, i open a session and store it in *ssh.Session attribute of the SSHConn,
send method just uses the connection that was created by the open,

@dbarrosop
Copy link
Contributor

Ok, we are probably talking about different things. My main concern is being able to reuse the connection. I already opened #37 which should be enough to illustrate what I mean.

@Ali-aqrabawi
Copy link
Author

an pr will be made after #37 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants