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

Ensure minimum kernel version for thin_ls #1411

Merged
merged 1 commit into from
Aug 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions container/docker/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@ import (
"fmt"
"path"
"regexp"
"strconv"
"strings"
"sync"

"github.com/blang/semver"
dockertypes "github.com/docker/engine-api/types"
"github.com/google/cadvisor/container"
"github.com/google/cadvisor/container/libcontainer"
"github.com/google/cadvisor/devicemapper"
"github.com/google/cadvisor/fs"
info "github.com/google/cadvisor/info/v1"
"github.com/google/cadvisor/machine"
"github.com/google/cadvisor/manager/watcher"
dockerutil "github.com/google/cadvisor/utils/docker"

Expand Down Expand Up @@ -178,6 +181,10 @@ func startThinPoolWatcher(dockerInfo *dockertypes.Info) (*devicemapper.ThinPoolW
return nil, err
}

if err := ensureThinLsKernelVersion(machine.KernelVersion()); err != nil {
return nil, err
}

dockerThinPoolName, err := dockerutil.DockerThinPoolName(*dockerInfo)
if err != nil {
return nil, err
Expand All @@ -197,6 +204,66 @@ func startThinPoolWatcher(dockerInfo *dockertypes.Info) (*devicemapper.ThinPoolW
return thinPoolWatcher, nil
}

func ensureThinLsKernelVersion(kernelVersion string) error {
// kernel 4.4.0 has the proper bug fixes to allow thin_ls to work without corrupting the thin pool
minKernelVersion := semver.MustParse("4.4.0")
// RHEL 7 kernel 3.10.0 release >= 366 has the proper bug fixes backported from 4.4.0 to allow
// thin_ls to work without corrupting the thin pool
minRhel7KernelVersion := semver.MustParse("3.10.0")

matches := version_re.FindStringSubmatch(kernelVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just to check that kernelVersion has sane formatting? I think blang/semver already does that with semver.Make

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make blows up on x86_64. Maybe there's another function that doesn't?
On Thu, Aug 4, 2016 at 1:35 PM Tim St. Clair [email protected]
wrote:

In container/docker/factory.go
#1411 (comment):

  • b := make([]byte, len(unameOutput.Release))
  • for i, v := range unameOutput.Release {
  •   b[i] = byte(v)
    
  • }
  • return string(b), nil
    +}

+func ensureThinLsKernelVersion(

kernelVersion string) error {

  • // kernel 4.4.0 has the proper bug fixes to allow thin_ls to work without corrupting the thin pool
  • version_4_4_0 := semver.MustParse("4.4.0")
  • // RHEL kernel 3.10.0 release >= 366 has the proper bug fixes backported from 4.4.0 to allow
  • // thin_ls to work without corrupting the thin pool
  • version_3_10_0 := semver.MustParse("3.10.0")
  • matches := version_re.FindStringSubmatch(kernelVersion)

Is this just to check that kernelVersion has sane formatting? I think
blang/semver already does that with semver.Make


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#r73567406,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYtvojP7NzLKBOb5XYLhNiCKb-MLvks5qciL6gaJpZM4Jc2Va
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested search/replace _ with -...

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 just tried Eric's suggestion, and because 4.4.0-abc has a pre-release component (abc), it is considered less than 4.4.0 proper. So that won't work.

if len(matches) < 4 {
return fmt.Errorf("error parsing kernel version: %q is not a semver", kernelVersion)
}

sem, err := semver.Make(matches[0])
if err != nil {
return err
}

if sem.GTE(minKernelVersion) {
// kernel 4.4+ - good
return nil
}

// Certain RHEL/Centos 7.x kernels have a backport to fix the corruption bug
if !strings.Contains(kernelVersion, ".el7") {
// not a RHEL 7.x kernel - won't work
return fmt.Errorf("kernel version 4.4.0 or later is required to use thin_ls - you have %q", kernelVersion)
}

// RHEL/Centos 7.x from here on
if sem.Major != 3 {
// only 3.x kernels *may* work correctly
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because 4.0.0 <= version < 4.4.0 kernels won't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing he was worried about 2.x kernels?

Copy link
Contributor

@eparis eparis Aug 4, 2016

Choose a reason for hiding this comment

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

How about we turn all of these into:

if !strings.Contains(kernelVersion, ".el7.") || sem.EQ(version_3_10_0) {
 return ERROR
}
[check more RHEL version madness]

(admittedly that might make it harder to extend for others, but how many people do the long term support like RHEL kernels and are likely to find/fix this?)

Copy link
Contributor

Choose a reason for hiding this comment

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

2.x kernels will get picked up by the following checks.

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 can't speak to if any other vendors backported the bug fixes to something earlier than 4.4.0. (@eparis where did you find that 4.4.0 was the min version with the fix?) My logic flow was this:

  1. Regardless of distro, if >= 4.4.0, good
  2. Otherwise < 4.4.0
    1. If kernel doesn't contain ".el7.", bad
    2. Otherwise it does have ".el7", so:
      1. RHEL only backported it to 3.x, so if Major != 3, bad
      2. Major is 3, so check 3.10.0-366 or newer -> good

Copy link
Contributor

Choose a reason for hiding this comment

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

git tag -l --contains ed8b45a3679eb49069b094c0711b30833f27c734 v\*

return fmt.Errorf("RHEL/Centos 7.x kernel version 3.10.0-366 or later is required to use thin_ls - you have %q", kernelVersion)
}

if sem.GT(minRhel7KernelVersion) {
// 3.10.1+ - good
return nil
}

if sem.EQ(minRhel7KernelVersion) {
// need to check release
releaseRE := regexp.MustCompile(`^[^-]+-([0-9]+)\.`)
releaseMatches := releaseRE.FindStringSubmatch(kernelVersion)
if len(releaseMatches) != 2 {
return fmt.Errorf("unable to determine RHEL/Centos 7.x kernel release from %q", kernelVersion)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're looking for semver.Version.Pre

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, except for the aforementioned fact that having x86_64 in the version string won't parse as a semver. so my sem literally only has the major, minor, and patch values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ick. Oh well...


release, err := strconv.Atoi(releaseMatches[1])
if err != nil {
return fmt.Errorf("error parsing release %q: %v", releaseMatches[1], err)
}

if release >= 366 {
return nil
}
}

return fmt.Errorf("RHEL/Centos 7.x kernel version 3.10.0-366 or later is required to use thin_ls - you have %q", kernelVersion)
}

// Register root container before running this function!
func Register(factory info.MachineInfoFactory, fsInfo fs.FsInfo, ignoreMetrics container.MetricSet) error {
client, err := Client()
Expand Down
51 changes: 51 additions & 0 deletions container/docker/factory_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2016 Google Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package docker

import "testing"

func TestEnsureThinLsKernelVersion(t *testing.T) {
tests := []struct {
version string
expectedError string
}{
{"4.4.0-31-generic", ""},
{"4.4.1", ""},
{"4.6.4-301.fc24.x86_64", ""},
{"3.10.0-327.22.2.el7.x86_64", `RHEL/Centos 7.x kernel version 3.10.0-366 or later is required to use thin_ls - you have "3.10.0-327.22.2.el7.x86_64"`},
{"3.10.0-366.el7.x86_64", ""},
{"3.10.0-366.el7_3.x86_64", ""},
{"3.10.0.el7.abc", `unable to determine RHEL/Centos 7.x kernel release from "3.10.0.el7.abc"`},
{"3.10.0-abc.el7.blarg", `unable to determine RHEL/Centos 7.x kernel release from "3.10.0-abc.el7.blarg"`},
{"3.10.0-367.el7.x86_64", ""},
{"3.10.0-366.x86_64", `kernel version 4.4.0 or later is required to use thin_ls - you have "3.10.0-366.x86_64"`},
{"3.10.1-1.el7.x86_64", ""},
{"2.0.36", `kernel version 4.4.0 or later is required to use thin_ls - you have "2.0.36"`},
{"2.1", `error parsing kernel version: "2.1" is not a semver`},
}

for _, test := range tests {
err := ensureThinLsKernelVersion(test.version)
if err != nil {
if len(test.expectedError) == 0 {
t.Errorf("%s: expected no error, got %v", test.version, err)
} else if err.Error() != test.expectedError {
t.Errorf("%s: expected error %v, got %v", test.version, test.expectedError, err)
}
} else if err == nil && len(test.expectedError) > 0 {
t.Errorf("%s: expected error %v", test.version, test.expectedError)
}
}
}