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

Fix for direct memif logic #291

Merged

Conversation

Mixaster995
Copy link
Contributor

@Mixaster995 Mixaster995 commented Jun 30, 2021

Signed-off-by: Mikhail Avramenko avramenkomihail15@gmail.com

Description

Refactored memif chain element and made separate element which supports direct memif logic

Issue link

fixes #287

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@Mixaster995 Mixaster995 force-pushed the fix/direct-memif branch 3 times, most recently from 2a9afdc to 86a3c17 Compare June 30, 2021 10:48
@Mixaster995 Mixaster995 marked this pull request as ready for review June 30, 2021 10:56
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
@Mixaster995
Copy link
Contributor Author

@edwarnicke please, share your thoughts on modifications i made for memif

Comment on lines +60 to +75
if mechanism := memifMech.ToMechanism(conn.GetMechanism()); mechanism != nil {
// Direct memif if applicable
if memifSocketAddDel, ok := load(ctx, true); ok && m.directMemifEnabled {
_, ok := ifindex.Load(ctx, true)
if ok {
if err := del(ctx, conn, m.vppConn, true); err != nil {
_, _ = m.Close(ctx, conn)
return nil, err
}
mechanism.SetSocketFileURL((&url.URL{Scheme: memifMech.SocketFileScheme, Path: memifSocketAddDel.SocketFilename}).String())
delete(ctx, true)
ifindex.Delete(ctx, true)
return conn, nil
}
}
}
Copy link
Member

@denis-tingaikin denis-tingaikin Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be suggesting to not add complexity into memif server element and create a separate chain element for directmemif.

I'm fine with these changes if @edwarnicke has no comments on them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also inclined to a new chain element for directmemif ... but if that is not possible, lets at least default the directMemif=false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed default value

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
@denis-tingaikin denis-tingaikin merged commit b6ed43b into networkservicemesh:main Jul 2, 2021
@Mixaster995 Mixaster995 deleted the fix/direct-memif branch July 2, 2021 09:15
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.

Unable to create firewall with direct memif logic
4 participants