-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Ensure minimum kernel version for thin_ls #1411
Conversation
FYI @timstclair @vishh - lmk if something like this is ok |
This is breaking nodes in our cluster atm. |
Kernel bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1363776 |
Need to fix that 😄 |
@@ -197,6 +208,80 @@ func startThinPoolWatcher(dockerInfo *dockertypes.Info) (*devicemapper.ThinPoolW | |||
return thinPoolWatcher, nil | |||
} | |||
|
|||
func getKernelVersion() (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function already exists here: https://github.com/google/cadvisor/blob/master/machine/info.go#L142
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool, thanks!
On Thu, Aug 4, 2016 at 1:30 PM Tim St. Clair notifications@github.com
wrote:
In container/docker/factory.go
#1411 (comment):@@ -197,6 +208,80 @@ func startThinPoolWatcher(dockerInfo _dockertypes.Info) (_devicemapper.ThinPoolW
return thinPoolWatcher, nil
}+func getKernelVersion() (string, error) {
This function already exists here:
https://github.com/google/cadvisor/blob/master/machine/info.go#L142—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/google/cadvisor/pull/1411/files/d1e20410961d80cf2bde3a8a68c6249cf7c06c95#r73566520,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYme1PxGA4dN5oKe4pj-zPO7GPq7Eks5qciHAgaJpZM4Jc2Va
.
@timstclair any other comments? |
|
||
if sem.EQ(minRhel7KernelVersion) { | ||
// need to check release | ||
releaseRE := regexp.MustCompile(`^[^-]+-([^.]+)\.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since you're calling Atoi
on it, make the match group digits (([0-9]+)
).
Ensure that kernel >= 4.4.0 or RHEL/Centos 7 kernel >= 3.10.0-366 exists before starting the thin pool watcher. Prior versions have a bug in which reserving and releasing the metadata snapshot can cause thin pool corruption.
Just one nit. All the regex parsing is unfortunate, but I won't block you on it. Fix the nit, squash, and I'll merge. |
1bd0010
to
2b525ff
Compare
Done |
You're fast! Waiting for tests to complete. |
😄 I was in the middle of squashing when you posted the comment |
Thanks for the review + merge! |
Automatic merge from submit-queue godep: cadvisor: bump to v0.23.9 ```release-note Update cadvisor to v0.23.9 to fix a problem where attempting to gather container filesystem usage statistics could result in corrupted devicemapper thin pool storage for Docker. ``` v0.23.9 fixes an important devicemapper issue xref google/cadvisor#1411 @ncdc @vishh @timstclair
Automatic merge from submit-queue godep: cadvisor: bump to v0.23.9 ```release-note Update cadvisor to v0.23.9 to fix a problem where attempting to gather container filesystem usage statistics could result in corrupted devicemapper thin pool storage for Docker. ``` v0.23.9 fixes an important devicemapper issue xref google/cadvisor#1411 @ncdc @vishh @timstclair
Ensure that kernel >= 4.4.0 or RHEL/Centos 7 kernel >= 3.10.0-366 exists before starting the thin
pool watcher. Prior versions have a bug in which reserving and releasing the metadata snapshot can
cause thin pool corruption.
Fixes #1409
@eparis @smarterclayton @derekwaynecarr @pmorie @sjenning