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 interface cleanup #275

Closed
wants to merge 1 commit into from
Closed

Conversation

ljkiraly
Copy link
Contributor

Description

The vfConfig structure deleted too early and the inject module can not find the information to delete the interface.
Debug printouts added in cases when interface is added or deleted.

Issue link

Related to: networkservicemesh/deployments-k8s#9778

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 functionality
  • Documentation
  • Refactoring
  • CI

@ljkiraly ljkiraly marked this pull request as draft October 19, 2023 16:26
ljkiraly added a commit to Nordix/nsm-sdk-kernel that referenced this pull request Oct 19, 2023
Move an orphan nsm interface to host namespace and delete it.

Related issue: networkservicemesh/deployments-k8s#9778
Related PR: networkservicemesh/sdk-ovs#275

Signed-off-by: Laszlo Kiraly <[email protected]>
ljkiraly added a commit to Nordix/nsm-sdk-kernel that referenced this pull request Oct 19, 2023
Move an orphan nsm interface to host namespace and delete it.

Related issue: networkservicemesh/deployments-k8s#9778
Related PR: networkservicemesh/sdk-ovs#275

Signed-off-by: Laszlo Kiraly <[email protected]>
ljkiraly added a commit to Nordix/nsm-sdk-kernel that referenced this pull request Oct 20, 2023
Move an orphan nsm interface to host namespace and delete it.

Related issue: networkservicemesh/deployments-k8s#9778
Related PR: networkservicemesh/sdk-ovs#275

Signed-off-by: Laszlo Kiraly <[email protected]>
ljkiraly added a commit to Nordix/nsm-sdk-kernel that referenced this pull request Oct 20, 2023
Move an orphan nsm interface to host namespace and delete it.

Related issue: networkservicemesh/deployments-k8s#9778
Related PR: networkservicemesh/sdk-ovs#275

Signed-off-by: Laszlo Kiraly <[email protected]>
@ljkiraly ljkiraly self-assigned this Oct 24, 2023
@ljkiraly ljkiraly marked this pull request as ready for review October 24, 2023 07:09
@glazychev-art
Copy link
Contributor

@ljkiraly
Nice catch!
Perhaps it would be better if we changed the calls order in inject?
For example, currently it works like:

{
        ...
	rv, err := next.Client(ctx).Close(ctx, conn, opts...)
	injectErr := move(ctx, conn, c.vfRefCountMap, &c.vfRefCountMutex, metadata.IsClient(c), true)
	...
}

But we can change it to

{
        ...
	injectErr := move(ctx, conn, c.vfRefCountMap, &c.vfRefCountMutex, metadata.IsClient(c), true)
        if injectErr!=nil {
        ...
        }
	rv, err := next.Client(ctx).Close(ctx, conn, opts...)
	...
}

(same for server, I think)
Because it would be nice if the same element controlled adding and removing from vfconfig (mechanisms/kernel).

@denis-tingaikin
Copy link
Member

@ljkiraly I think it can be merged. Would you like to resolve comment #275 (comment) before merging?

@ljkiraly
Copy link
Contributor Author

@ljkiraly I think it can be merged. Would you like to resolve comment #275 (comment) before merging?

@denis-tingaikin Yes, please do not merge this yet. The comment is valid and I am to test with the proposed modification.

@denis-tingaikin
Copy link
Member

@ljkiraly Sure, all good

@ljkiraly ljkiraly marked this pull request as draft November 7, 2023 13:09
@ljkiraly
Copy link
Contributor Author

ljkiraly commented Nov 7, 2023

This is also fixed in sdk-kernel by correction: networkservicemesh/sdk-kernel#618

@ljkiraly ljkiraly closed this Nov 7, 2023
@ljkiraly ljkiraly deleted the ovs-cleanup branch November 10, 2023 15:08
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.

3 participants