Uploaded image for project: 'Mesos'
  1. Mesos
  2. MESOS-2986

Docker version output is not compatible with Mesos

    Details

    • Sprint:
      Mesosphere Sprint 13
    • Story Points:
      1

      Description

      We currently use docker version to get Docker version, in Docker master branch and soon in Docker 1.8 [1] the output for this command changes. The solution for now will be to use the unchanged docker --version output, in the long term we should consider stop using the cli and use the API instead.

      [1] https://github.com/docker/docker/pull/14047

        Issue Links

          Activity

          Hide
          bernd-mesos Bernd Mathiske added a comment -

          commit 1830a28bfbe2c53136896b467b749c34910ef5b4
          Author: haosdent huang <haosdent@gmail.com>
          Date: Fri Sep 4 11:24:45 2015 +0200

          Made docker version parsing resilient to trailing version components.

          Review: https://reviews.apache.org/r/37669

          Show
          bernd-mesos Bernd Mathiske added a comment - commit 1830a28bfbe2c53136896b467b749c34910ef5b4 Author: haosdent huang <haosdent@gmail.com> Date: Fri Sep 4 11:24:45 2015 +0200 Made docker version parsing resilient to trailing version components. Review: https://reviews.apache.org/r/37669
          Hide
          bernd-mesos Bernd Mathiske added a comment -

          Let's be defensive in our programming anyhow and land your patch!

          Show
          bernd-mesos Bernd Mathiske added a comment - Let's be defensive in our programming anyhow and land your patch!
          Hide
          haosdent@gmail.com haosdent added a comment -

          Thank you Isabel Jimenez. Dokcer also say they don't change version in rpm. So I open a bug in fedora fedora-1259427

          Show
          haosdent@gmail.com haosdent added a comment - Thank you Isabel Jimenez . Dokcer also say they don't change version in rpm. So I open a bug in fedora fedora-1259427
          Hide
          bernd-mesos Bernd Mathiske added a comment -
          Show
          bernd-mesos Bernd Mathiske added a comment - This is the patch: https://reviews.apache.org/r/37669/
          Hide
          ijimenez Isabel Jimenez added a comment -

          Thanks haosdent ! I'm not sure opening an issue in docker will really follow up with the guys that introduce this new versioning in docker for Fedora, it might be worth following up with the Fedora guys directly since they distribute their own 'patched' version of docker on Fedora and possibly RedHat.

          Show
          ijimenez Isabel Jimenez added a comment - Thanks haosdent ! I'm not sure opening an issue in docker will really follow up with the guys that introduce this new versioning in docker for Fedora, it might be worth following up with the Fedora guys directly since they distribute their own 'patched' version of docker on Fedora and possibly RedHat.
          Hide
          tnachen Timothy Chen added a comment -

          I don't think x.y.z.extra is common, at least it's not conforming to the spec as Ben pointed out. Thanks haosdent for the change and flexible of changing the patch again!

          Show
          tnachen Timothy Chen added a comment - I don't think x.y.z.extra is common, at least it's not conforming to the spec as Ben pointed out. Thanks haosdent for the change and flexible of changing the patch again!
          Hide
          haosdent@gmail.com haosdent added a comment -

          Sorry for misunderstand that. So let me update the patch convert "X.Y.Z.EXTRA -> X.Y.Z"

          Show
          haosdent@gmail.com haosdent added a comment - Sorry for misunderstand that. So let me update the patch convert "X.Y.Z.EXTRA -> X.Y.Z"
          Hide
          bmahler Benjamin Mahler added a comment -

          I was just suggesting that if we want to fix this, we avoid pushing it into stout's Version class. Rather, we could have the docker code in mesos go from X.Y.Z.EXTRA -> X.Y.Z, for example, with a big comment saying why we do this for docker, and a pointer to the docker issue you opened about not following semver. Alternatively, if it turns out that x.y.z.extra.versions is a common practice, perhaps it is worth storing an arbitrary number of components in Version, rather than just semver's 3 (would be great to avoid this though!).

          I didn't want to suggest we do nothing if this is a real problem for users

          Show
          bmahler Benjamin Mahler added a comment - I was just suggesting that if we want to fix this, we avoid pushing it into stout's Version class. Rather, we could have the docker code in mesos go from X.Y.Z.EXTRA -> X.Y.Z, for example, with a big comment saying why we do this for docker, and a pointer to the docker issue you opened about not following semver. Alternatively, if it turns out that x.y.z.extra.versions is a common practice, perhaps it is worth storing an arbitrary number of components in Version, rather than just semver's 3 (would be great to avoid this though!). I didn't want to suggest we do nothing if this is a real problem for users
          Hide
          haosdent@gmail.com haosdent added a comment -

          Benjamin Mahler Timothy Chen So for this issue, it is not mesos problem, right? I open the issue in docker.

          docker --version
          Docker version 1.7.0.fc22, build 74e7a7a/1.7.0
          
          Show
          haosdent@gmail.com haosdent added a comment - Benjamin Mahler Timothy Chen So for this issue, it is not mesos problem, right? I open the issue in docker. docker --version Docker version 1.7.0.fc22, build 74e7a7a/1.7.0
          Hide
          haosdent@gmail.com haosdent added a comment -

          For

          docker --version
          Docker version 1.7.0.fc22, build 74e7a7a/1.7.0
          

          I have a simple patch to ignore overflow version components, but I not sure whether it is a correct way or not. https://reviews.apache.org/r/37669/

          Show
          haosdent@gmail.com haosdent added a comment - For docker --version Docker version 1.7.0.fc22, build 74e7a7a/1.7.0 I have a simple patch to ignore overflow version components, but I not sure whether it is a correct way or not. https://reviews.apache.org/r/37669/
          Hide
          haosdent@gmail.com haosdent added a comment -

          Steve Hoffman I rebase Isabel Jimenez patch to 0.22.1 (in attachment). You could use it through

          patch -p1 < MESOS-2986_0_22_1.patch
          

          and rebuild the packages.

          But I only test it under Ubuntu 14.04 with docker 1.8.1, and make sure it pass

          sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="DockerContainerizerTest.*" --verbose
          sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="DockerTest.*" --verbose
          

          If you want to use it in production environment, please test it in test environment first. Because I not sure my backport don't have problem.

          Show
          haosdent@gmail.com haosdent added a comment - Steve Hoffman I rebase Isabel Jimenez patch to 0.22.1 (in attachment). You could use it through patch -p1 < MESOS-2986_0_22_1.patch and rebuild the packages. But I only test it under Ubuntu 14.04 with docker 1.8.1, and make sure it pass sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter= "DockerContainerizerTest.*" --verbose sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter= "DockerTest.*" --verbose If you want to use it in production environment, please test it in test environment first. Because I not sure my backport don't have problem.
          Hide
          tnachen Timothy Chen added a comment -

          I think we fixed it but as David Young reported we don't handle Fedora version string yet.
          This is still open IMO.

          Show
          tnachen Timothy Chen added a comment - I think we fixed it but as David Young reported we don't handle Fedora version string yet. This is still open IMO.
          Hide
          hoffman60613 Steve Hoffman added a comment -

          I'm confused. The resolution on this says "fixed" but the state is "in progress". Can we get that clarified?
          Would really love to get this fix in 1.8 (https://github.com/docker/docker/pull/11784) in my cluster.
          Any word on this a release date and/or a backport to 0.22.X?

          Show
          hoffman60613 Steve Hoffman added a comment - I'm confused. The resolution on this says "fixed" but the state is "in progress". Can we get that clarified? Would really love to get this fix in 1.8 ( https://github.com/docker/docker/pull/11784 ) in my cluster. Any word on this a release date and/or a backport to 0.22.X?
          Hide
          hoffman60613 Steve Hoffman added a comment - - edited

          Yea, the 0.22.1 version of this code while ugly, just checked the major version number rather than creating a Version class which assumes there are just 3 number digits – which clearly everybody doesn't follow (as in the FC case)

            foreach (string line, strings::split(output.get(), "\n")) {
              line = strings::trim(line);
              if (strings::startsWith(line, "Client version: ")) {
                line = line.substr(strlen("Client version: "));
                vector<string> version = strings::split(line, ".");
                if (version.size() < 1) {
                  return Error("Failed to parse Docker version '" + line + "'");
                }
                Try<int> major = numify<int>(version[0]);
                if (major.isError()) {
                  return Error("Failed to parse Docker major version '" +
                               version[0] + "'");
                } else if (major.get() < 1) {
                  break;
                }
                return new Docker(path);
              }
            }
          

          At this point in time do we still need a check here? Would anybody be using pre 1.0 docker with mesos? You could just dump the check outright...

          Also,when this is fixed, can we get a patch to the 0.22.1 RPM?

          Show
          hoffman60613 Steve Hoffman added a comment - - edited Yea, the 0.22.1 version of this code while ugly, just checked the major version number rather than creating a Version class which assumes there are just 3 number digits – which clearly everybody doesn't follow (as in the FC case) foreach (string line, strings::split(output.get(), "\n" )) { line = strings::trim(line); if (strings::startsWith(line, "Client version: " )) { line = line.substr(strlen( "Client version: " )); vector<string> version = strings::split(line, "." ); if (version.size() < 1) { return Error( "Failed to parse Docker version '" + line + "'" ); } Try< int > major = numify< int >(version[0]); if (major.isError()) { return Error( "Failed to parse Docker major version '" + version[0] + "'" ); } else if (major.get() < 1) { break ; } return new Docker(path); } } At this point in time do we still need a check here? Would anybody be using pre 1.0 docker with mesos? You could just dump the check outright... Also,when this is fixed, can we get a patch to the 0.22.1 RPM?
          Hide
          dove-young David Young added a comment -

          In Fedora 22, docker 1.7 will report version as 1.7.0.fc22, this will break mesos slave from start up.

          docker --version
          Docker version 1.7.0.fc22, build 74e7a7a/1.7.0
          
          Failed to create a containerizer: Could not create DockerContainerizer: Failed to create docker: Failed to get docker version: Failed to parse docker version: Version string has 4 components; maximum 3 components allowed
          
          Show
          dove-young David Young added a comment - In Fedora 22, docker 1.7 will report version as 1.7.0.fc22 , this will break mesos slave from start up. docker --version Docker version 1.7.0.fc22, build 74e7a7a/1.7.0 Failed to create a containerizer: Could not create DockerContainerizer: Failed to create docker: Failed to get docker version: Failed to parse docker version: Version string has 4 components; maximum 3 components allowed
          Hide
          marco-mesos Marco Massenzio added a comment -

          Re-opening so we can add the Resolution field (due to bug caused to workflow change).

          Show
          marco-mesos Marco Massenzio added a comment - Re-opening so we can add the Resolution field (due to bug caused to workflow change).
          Hide
          tnachen Timothy Chen added a comment -

          commit 6760601cbe0b737047b75a8b058cba3fb892d128
          Author: Isabel Jimenez <contact@isabeljimenez.com>
          Date: Thu Jul 2 11:00:33 2015 -0700

          Change docker version parsing.

          Review: https://reviews.apache.org/r/36128

          Show
          tnachen Timothy Chen added a comment - commit 6760601cbe0b737047b75a8b058cba3fb892d128 Author: Isabel Jimenez <contact@isabeljimenez.com> Date: Thu Jul 2 11:00:33 2015 -0700 Change docker version parsing. Review: https://reviews.apache.org/r/36128
          Show
          ijimenez Isabel Jimenez added a comment - https://reviews.apache.org/r/36128/

            People

            • Assignee:
              haosdent@gmail.com haosdent
              Reporter:
              ijimenez Isabel Jimenez
              Shepherd:
              Benjamin Hindman
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile