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

changes in direct memif implementation #377

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

Mixaster995
Copy link
Contributor

Signed-off-by: Mikhail Avramenko [email protected]

Related issues

networkservicemesh/deployments-k8s#2381
#373

Description

To fix problem with nse-composition i made and tested rework for direct memif.

@Mixaster995 Mixaster995 force-pushed the fix/direct-memif branch 5 times, most recently from 667742a to 2d1d797 Compare September 15, 2021 10:30
@edwarnicke
Copy link
Member

@Mixaster995 I'm a little curious why you are fixing this in mechanism/memif/client.go instead of in mechanism/memif/memifproxy ? It looks like you are simply replicating logic the memif proxy has for not recreating proxies for existing connections, only in memif/ rather than memif/proxy.

Signed-off-by: Mikhail Avramenko <[email protected]>
@Mixaster995
Copy link
Contributor Author

Mixaster995 commented Sep 16, 2021

Hello, @edwarnicke.
It is not related to memifproxy chain element.

Current direct memif logic working like this:

  1. Create memif on nse
  2. Create memif on client side of forwarder
  3. On server side of forwarder check if client-side memif created delete it and not create server-side memif.
  4. create memif on nsc
    every step happens on way back

With this fixes current logic is:

  1. On way forward on server side of forwarder if left mechanism is memif, then add info to metadata
  2. On way back on client side of forwarder check left(from metadata) and right mechanism to be memif (which is direct memif scenario)
  3. In direct memif case not create memif on client side of forwarder and just save socket url to metada
  4. On server side of forwarder not create memif and set mechanism socketurl from metada

Thus, we removing excessive memif creation on client side of forwarder, because it is not actually needed in direct memif scenario. And it is also eliminates leak reported in networkservicemesh/deployments-k8s#2381

@edwarnicke
Copy link
Member

@Mixaster995 After much poking at it, this looks good. Could you flip to ready to review so we can merge it?

@Mixaster995 Mixaster995 marked this pull request as ready for review September 20, 2021 03:25
@edwarnicke edwarnicke merged commit db4ea46 into networkservicemesh:main Sep 20, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsc-vpp that referenced this pull request Sep 20, 2021
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#377

Commit: db4ea46
Author: Авраменко Михаил
Date: 2021-09-20 19:25:15 +0700
Message:
  - changes in direct memif implementation (#377)
* changes in direct memif implementation

Signed-off-by: Mikhail Avramenko <[email protected]>

* review fixes

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/cmd-forwarder-vpp that referenced this pull request Sep 20, 2021
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#377

Commit: db4ea46
Author: Авраменко Михаил
Date: 2021-09-20 19:25:15 +0700
Message:
  - changes in direct memif implementation (#377)
* changes in direct memif implementation

Signed-off-by: Mikhail Avramenko <[email protected]>

* review fixes

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/cmd-nse-firewall-vpp that referenced this pull request Sep 20, 2021
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#377

Commit: db4ea46
Author: Авраменко Михаил
Date: 2021-09-20 19:25:15 +0700
Message:
  - changes in direct memif implementation (#377)
* changes in direct memif implementation

Signed-off-by: Mikhail Avramenko <[email protected]>

* review fixes

Signed-off-by: NSMBot <[email protected]>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder-vpp that referenced this pull request Sep 20, 2021
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#377

Commit: db4ea46
Author: Авраменко Михаил
Date: 2021-09-20 19:25:15 +0700
Message:
  - changes in direct memif implementation (#377)
* changes in direct memif implementation

Signed-off-by: Mikhail Avramenko <[email protected]>

* review fixes

Signed-off-by: NSMBot <[email protected]>
@Mixaster995 Mixaster995 deleted the fix/direct-memif branch October 13, 2021 11:35
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.

5 participants