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

[WIP] Rdma device rename in container #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adrianchiris
Copy link
Collaborator

This PR adds the functionality of providing incrementing RDMA device name in container.
allowing for consistent RDMA device names in containers e.g mlx5_0 for the first RDMA enabled network, mlx5_1 for the second RDMA enabled network etc...

Missing:

@adrianchiris adrianchiris requested a review from moshe010 April 13, 2020 13:26
@coveralls
Copy link

coveralls commented Apr 13, 2020

Pull Request Test Coverage Report for Build 87

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+9.6%) to 63.014%

Totals Coverage Status
Change from base Build 47: 9.6%
Covered Lines: 46
Relevant Lines: 73

💛 - Coveralls


"github.com/vishvananda/netlink"
)

// Max netdevice length is 15 chars. see if.h linux uapi header IFNAMSIZ
const localIFNAMSIZ=16
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use the const https://golang.org/pkg/syscall/ it in hex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I originally thought that its not needed to link against it but thinking on this further, i already use netlink which imports syscall

cmd/rdma/main.go Outdated
}

// Expected format is <driver_name>_<idx>
s := strings.Split(sRdmaDev, "_")
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can create on method that extract the driver and the index and use it here and getRdmaDevIndexFromName

}

func GetRandomRdmaDevName() string {
return fmt.Sprintf("rdmadev_%d", rand.Int31())[:(localIFNAMSIZ-1)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't each rdma device has it own index like net device?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it has, do you prefer to use rdmadev_<rdma-dev-idx> ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it ok, just remove the random

- method to generate a random name
- method to generate the "next" name
Extend RDMA manager API to facilitate RDMA device
renmae when moving it to/form container network namespace

- Add method to get available RDMA devices in the system
- Add method to rename RDMA device
This commit implements the main flow for RDMA device rename
when moving the device to/from container

On CmdAdd: rename device to the next available rdma device name
           according to the naming scheme defined in previous commit.

On CmdDel: Restore the name to its orignal name as was saved in cache
}

// Get the next RDMA device name for a given RDMA device prefix
func GetNextRdmaDeviceName(prefix string, currDevs []string) (string, error) {

Choose a reason for hiding this comment

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

@adrianchiris
I think it is better to use PCI device based naming scheme from
https://github.com/linux-rdma/rdma-core/blob/master/kernel-boot/rdma-persistent-naming.rules

And let rdma/hca operator to configure system for rdma device naming.

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.

4 participants