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

feat: add generic nvme support #1252

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
@jippi
Copy link
Contributor

jippi commented Aug 15, 2018

What?

This is my local work, trying to solve same issue as #1233

This PR makes NVMe transparent to rexray, as it translates between the NVMe logical device and the virtual device that is the udev symlink with the device name that rexray request EC2 to mount the EBS volume as

Tested on

  • c4
  • c5
  • c5d
  • m4
  • m5
  • m5d
  • r4
  • r5
  • r5d
  • i3.2xlarge

Requirements

Require a udev rule to alias the NVMe device to the path that rexray expects as mount point.

A similar udev rule is build into the Amazon Linux AMI already, and trivial to add to other linux distributions (see below)

An alternative is to manage the symlinks from the rexray driver (its just a symlink), but seem like feature creep to me, considering the low effort required to make the udev setup work

Example udev rule

# /etc/udev/rules.d/999-aws-ebs-nvme.rules
# ebs nvme devices
KERNEL=="nvme[0-9]*n[0-9]*", ENV{DEVTYPE}=="disk", ATTRS{model}=="Amazon Elastic Block Store", PROGRAM="/usr/local/bin/ebs-nvme-mapping /dev/%k", SYMLINK+="%c"

Helper script for udev rules

#!/bin/bash
#/usr/local/bin/ebs-nvme-mapping
vol=$(/usr/sbin/nvme id-ctrl --raw-binary "$1" | cut -c3073-3104 | tr -s ' ' | sed 's/ $//g')
vol=${vol#/dev/}
if [[ -n "$vol" ]]; then
  echo ${vol/xvd/sd} ${vol/sd/xvd}
fi

Signed-off-by: Christian Winther jippignu@gmail.com

feat: add generic nvme support
Signed-off-by: Christian Winther <jippignu@gmail.com>
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 15, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #1252 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
- Coverage   34.58%   34.54%   -0.05%     
==========================================
  Files          36       36              
  Lines        2362     2362              
==========================================
- Hits          817      816       -1     
- Misses       1441     1442       +1     
  Partials      104      104
Impacted Files Coverage Δ
libstorage/api/types/types_localdevices.go 84.21% <0%> (-1.76%) ⬇️

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 d14c3e3...c01de50. Read the comment docs.

@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Aug 16, 2018

@clintkitson can i get a review on this? its quite simpler than the other NVMe support PR, and is limited strictly to NVMe devices, 0% risk of affecting existing use-cases

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Aug 16, 2018

cc @JustinVenus @Crapworks @codenrhoden

I do like the approach more. What does everyone feel about this path?

@jippi jippi referenced this pull request Aug 19, 2018

Open

[aws/ebs] NVMe support #1104

@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Aug 20, 2018

@clintkitson Can we please get the ball rolling on this? been a issue for many months now, slightly frustrating that its moving so slow :)

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Aug 20, 2018

Thanks for the patience @jippi. Glad to see the community work through updates on this. Can you take a stab at updating the documentation that describe how to make this work in this pr?

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Aug 22, 2018

@JustinVenus Thoughts on this implementation versus the one you submitted?

@JustinVenus

This comment has been minimized.

Copy link

JustinVenus commented Aug 23, 2018

TL;DR looks good, please shipit/merge ---

@clintkitson I haven't tested this PR, but this PR looks very straightforward and easy to maintain. I personally would not hard code the path to the nvme-cli, but I'd worry about that nitpick if it ever became a real issue.

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Aug 23, 2018

Just waiting on some doc updates and we will ship this in a minor release.

@jamesdh

This comment has been minimized.

Copy link

jamesdh commented Aug 30, 2018

Assuming this get's approved and merged in the coming few days, about how long of a wait time would there be for a rexray release containing it?

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Aug 30, 2018

We can turn this around within days, but am still waiting on some doc updates that indicate how to configure the new functionality.

@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Aug 30, 2018

The OP have the two steps. It's literally just putting the two files on disk and reboot / restart udev :)

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Aug 30, 2018

Hi @jippi,

Once upon a time I wrote a blog that outlined how to reproduce REX-Ray locally down to the checksum of the produced binaries. In other words, even without this merged it shouldn't be too difficult for y'all to use it in your environment.

@matthewmrichter

This comment has been minimized.

Copy link

matthewmrichter commented Sep 11, 2018

Bump.

@jwitko

This comment has been minimized.

Copy link

jwitko commented Sep 12, 2018

Would really love to see this merged.

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Sep 12, 2018

Hi @jippi / @matthewmrichter / @jwitko,

I'm going to merge this, but I'm holding one or all of you accountable (aka asking you nicely with a cherry on top) to please go file a PR that takes the instructions in this PR description and incorporates them into the RR docs all nice and pretty like. Once that's done, we can cut a patch release so this is available GA. At least merged you'll be able to pull it officially as the latest unstable bits.

Actually, I'm hedging on my previous comment. I've noticed a few things.

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Sep 12, 2018

Hi @jippi,

If you can't attend to the review comments, I'll take over the PR (keeping your commit) and get this merged by the end of the week.

akutz added a commit to akutz/rexray that referenced this pull request Sep 12, 2018

Some minor fixes to the previous commit
This patch just adds some changes to address comments in PR rexray#1252.
@akutz

This comment has been minimized.

Copy link
Member

akutz commented Sep 12, 2018

Hi @jippi,

I just pushed e26cf35 to address my review comments. Please feel free to use it to update your PR.

To grab it, do the following:

$ git remote add akutz https://github.com/akutz/rexray.git
$ git fetch akutz
$ git cherry-pick e26cf35d00a5f237d2384b88329225b4dbc5183c
@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Sep 12, 2018

@akutz feel free to highjack it - I'm on vacation this week - thank you :)

@akutz akutz merged commit c01de50 into rexray:master Sep 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

akutz added a commit that referenced this pull request Sep 12, 2018

Some minor fixes to the previous commit
This patch just adds some changes to address comments in PR #1252:

* Documentation
* Configurable path to `nvme`
* More robust file detection
* Contextual logging
@akutz

This comment has been minimized.

Copy link
Member

akutz commented Sep 12, 2018

Hi @jippi / @matthewmrichter / @jwitko,

This has been merged. Please see 407253f for the addendums I made to the PR. They include:

  • Documentation
  • Configurable path to nvme
  • More robust file detection
  • Contextual logging

cc @clintkitson @codenrhoden

@jippi jippi deleted the seatgeek:generic-nvme-support branch Sep 12, 2018

@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Sep 19, 2018

Any plans for cutting a release? :)

@ccakes

This comment has been minimized.

Copy link

ccakes commented Jan 2, 2019

Any updates on this? Seems like the code is merged and the functionality here is probably worth a release on it's own given that REXray doesn't work out of the box on the latest AWS instance types.

@jippi

This comment has been minimized.

Copy link
Contributor Author

jippi commented Jan 2, 2019

Given its taking full 9 months from initial PR to any release, and 3 months from merge and still no release, I would probably start considering REXray for deprecated and unmaintained. I'm running my own fork internally at @seatgeek now, which is this branch. Got tired of waiting around

@matthewmrichter

This comment has been minimized.

Copy link

matthewmrichter commented Jan 2, 2019

Yes, we are switching to the Cloudstor driver which is quite simple to set up and supports EFS as well. I believe it is maintained by Docker corporation themselves. Unfortunately they are dragging their heels on NVME a bit as well but hopefully that gets fixed soon.

@clintkitson

This comment has been minimized.

Copy link
Member

clintkitson commented Jan 7, 2019

Hello folks, we have been waiting on a community member to run a test of the unstable release. If that happens and we can get a report here we will proceed with a release.

@karlatkinson

This comment has been minimized.

Copy link

karlatkinson commented Jan 15, 2019

A few people have tested this out here #1104
Are you still looking for more feedback before releasing as stable?

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Jan 15, 2019

Hi @karlatkinson / @matthewmrichter / @jippi / @ccakes,

My thanks to the community for testing this PR. Release v0.11.4 is building now. I'll ping you again when the binaries are available. Thank you.

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Jan 15, 2019

Hi @karlatkinson / @matthewmrichter / @jippi / @ccakes,

The complete set of the v0.11.4 binaries are now available. Please note that Bintray appears to be suffering from gremlins, and I've had to spend one-two hours re-running builds until the BT server will accept uploads. This means not all of the Docker images are not yet available, but they are uploading fine and the complete set should be available in the next hour.

wolf31o2 added a commit to wolf31o2/dcos that referenced this pull request Feb 19, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>

wolf31o2 added a commit to wolf31o2/dcos that referenced this pull request Feb 19, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>

@wolf31o2 wolf31o2 referenced this pull request Feb 19, 2019

Merged

DCOS_OSS-4316 REX-Ray: bump to v0.11.4 #4552

5 of 7 tasks complete

wolf31o2 added a commit to wolf31o2/dcos that referenced this pull request Mar 12, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>

gpaul added a commit to gpaul/dcos that referenced this pull request Apr 10, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>
(cherry picked from commit b07fc98)

gpaul added a commit to mesosphere/dcos that referenced this pull request Apr 10, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>
(cherry picked from commit b07fc98)

gpaul added a commit to gpaul/dcos that referenced this pull request Apr 10, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>
(cherry picked from commit b07fc98)

gpaul added a commit to mesosphere/dcos that referenced this pull request Apr 10, 2019

DCOS_OSS-4316 REX-Ray: bump to v0.11.4
Version bump to the latest release of REX-Ray, which includes support for
AWS NVMe storage. Users will still be required to provide `udev` scripts
and provide the `nvme-cli` package on the host. Details can be found in
the original REX-Ray [pull request](rexray/rexray#1252)

Signed-off-by: Chris Gianelloni <cgianelloni@applause.com>
(cherry picked from commit b07fc98)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.