Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fallback to /dev/mapper device if metadata device is not set in docker info #1343

Merged
merged 2 commits into from
Jun 22, 2016

Conversation

sjenning
Copy link
Contributor

@sjenning sjenning commented Jun 21, 2016

@derekwaynecarr @aveshagarwal @pmorie @ncdc

In systems that have an LVM managed docker thin pool, docker info does not contain information for the data or metadata devices:

$ docker info
..
Storage Driver: devicemapper
 Pool Name: fedora-docker--pool
 Pool Blocksize: 524.3 kB
 Base Device Size: 107.4 GB
 Backing Filesystem: xfs
 Data file: 
 Metadata file: 
 ...

LVM puts the metadata device at /dev/mapper/_tmeta. If we fail to get the metadata device from the docker info, we should fall back to the LVM path.

Fixes #1338

@k8s-bot
Copy link
Collaborator

k8s-bot commented Jun 21, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@ncdc
Copy link
Collaborator

ncdc commented Jun 21, 2016

FYI @vishh @dchen1107 @timstclair the initial "thin_ls" PR that we added only works for devicemapper-managed thin pools. This PR should allow support for LVM-managed pools as well. AFAIK this PR helps us pass some node E2E tests that are currently failing on the Red Hat stack.

cc @rhvgoyal @ingvagabund

@ncdc
Copy link
Collaborator

ncdc commented Jun 21, 2016

If this isn't approved for Kube 1.3, we'd love to see it ASAP after.

@sjenning
Copy link
Contributor Author

Just added some new changes

  • os.Stat to check to metadata file existence when using assumed LVM path
  • only fail to gather filesystem stats if metadata device can't be found (other stats still work)


metadataDevice = fmt.Sprintf("/dev/mapper/%s_tmeta", poolName)

_, err = os.Stat(metadataDevice)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if _, err := os.Stat(metadataDevice); err != nil {

@vishh
Copy link
Contributor

vishh commented Jun 21, 2016

LGTM. Once the existing comments are addressed, this is good to go. @ncdc is this supposed to be integrated into kube v1.3?

@ncdc
Copy link
Collaborator

ncdc commented Jun 21, 2016

@vishh if this can make 1.3, that would be great

@vishh
Copy link
Contributor

vishh commented Jun 21, 2016

That would require a cherrypick against the release-0.23 branch. A new
tag needs to be cut against v0.23 and then vendor that tag into kube.

On Tue, Jun 21, 2016 at 1:17 PM, Andy Goldstein notifications@github.com
wrote:

@vishh https://github.com/vishh if this can make 1.3, that would be
great


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1343 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGvIKDUW_IS_t9rVFZGz7Taj32mr5Hkdks5qOEbWgaJpZM4I7F0V
.

@ncdc
Copy link
Collaborator

ncdc commented Jun 21, 2016

@vishh assuming this gets merged, we can do the PR against release-0.23 and then update the kube godep once you guys cut a new tag

@derekwaynecarr
Copy link
Collaborator

I think this needs to make 1.3 as without it pod stats are not reported at all.

@ncdc
Copy link
Collaborator

ncdc commented Jun 22, 2016

I think this looks good. @vishh can you kick the e2e?

@ncdc
Copy link
Collaborator

ncdc commented Jun 22, 2016

Not sure if I have admin, but let's see:

@k8s-bot ok to test

@timstclair
Copy link
Contributor

ok to test

@k8s-bot
Copy link
Collaborator

k8s-bot commented Jun 22, 2016

Jenkins GCE e2e

Build/test failed for commit eafff74.

@vishh
Copy link
Contributor

vishh commented Jun 22, 2016

@k8s-bot test this

@k8s-bot
Copy link
Collaborator

k8s-bot commented Jun 22, 2016

Jenkins GCE e2e

Build/test failed for commit eafff74.

@timstclair
Copy link
Contributor

CoreOS issue: #1344 - looks like this has started affecting the build job as well.

@timstclair
Copy link
Contributor

Disabled coreos-beta.

@k8s-bot test this

@k8s-bot
Copy link
Collaborator

k8s-bot commented Jun 22, 2016

Jenkins GCE e2e

Build/test passed for commit eafff74.

@ncdc
Copy link
Collaborator

ncdc commented Jun 22, 2016

@vishh @derekwaynecarr any other comments?

@derekwaynecarr
Copy link
Collaborator

LGTM

@ncdc
Copy link
Collaborator

ncdc commented Jun 22, 2016

I'd like to get this merged ASAP once it passes reviews so we can prep the follow-on tasks (release-0.23 cherry-pick, kube godep bump)... any other comments?

@vishh
Copy link
Contributor

vishh commented Jun 22, 2016

LGTM

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants