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

devmapper: udev sync is a requirement #11412

Merged
merged 2 commits into from Apr 10, 2015

Conversation

Projects
None yet
10 participants
@vbatts
Contributor

vbatts commented Mar 16, 2015

Rather than disabling devmapper for static builds, this will get more at the root. Udev sync is a prerequisite for devicemapper. That sync is false on static builds as well.

See also #4036

closes #10664

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 16, 2015

Contributor

perhaps this also needs a storage option parameter like dm.use_regardless_of_udev_sync_i_know_what_i_am_asking_for=true
for the folks that are using this on ubuntu and being affected, but we can't help due the version of their udev and libdevmapper...

Contributor

vbatts commented Mar 16, 2015

perhaps this also needs a storage option parameter like dm.use_regardless_of_udev_sync_i_know_what_i_am_asking_for=true
for the folks that are using this on ubuntu and being affected, but we can't help due the version of their udev and libdevmapper...

@vbatts vbatts removed the dco/no label Mar 16, 2015

@jessfraz jessfraz added the dco/yes label Mar 16, 2015

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 16, 2015

Contributor

we run tests on a fedora machine on every push to master we could make sure its tested there

Contributor

jessfraz commented Mar 16, 2015

we run tests on a fedora machine on every push to master we could make sure its tested there

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 16, 2015

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Mar 16, 2015

Contributor

@vbatts I think we need a special flag to allow this to be bypassed so that it can be tested.

Contributor

unclejack commented Mar 16, 2015

@vbatts I think we need a special flag to allow this to be bypassed so that it can be tested.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 16, 2015

Contributor

Will do
On Mar 16, 2015 5:15 PM, "unclejack" notifications@github.com wrote:

@vbatts https://github.com/vbatts I think we need a special flag to
allow this to be bypassed so that it can be tested.


Reply to this email directly or view it on GitHub
#11412 (comment).

Contributor

vbatts commented Mar 16, 2015

Will do
On Mar 16, 2015 5:15 PM, "unclejack" notifications@github.com wrote:

@vbatts https://github.com/vbatts I think we need a special flag to
allow this to be bypassed so that it can be tested.


Reply to this email directly or view it on GitHub
#11412 (comment).

@vbatts

This comment has been minimized.

Show comment
Hide comment
Contributor

vbatts commented Mar 17, 2015

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

@vbatts

Few more suggestions for flag name.

"dm.ignore_udev_sync_check"
"dm.override_udev_sync_check"

Contributor

rhvgoyal commented Mar 17, 2015

@vbatts

Few more suggestions for flag name.

"dm.ignore_udev_sync_check"
"dm.override_udev_sync_check"

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

@vbatts

Can we also modify tests to use above flag by default?

Contributor

rhvgoyal commented Mar 17, 2015

@vbatts

Can we also modify tests to use above flag by default?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 17, 2015

Contributor

perhaps 'override...', because we are still checking it, so we can log in the daemon that it's an issue.

Contributor

vbatts commented Mar 17, 2015

perhaps 'override...', because we are still checking it, so we can log in the daemon that it's an issue.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

@vbatts

"dm.override_udev_sync_check" sounds good.

Contributor

rhvgoyal commented Mar 17, 2015

@vbatts

"dm.override_udev_sync_check" sounds good.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 17, 2015

Contributor

@rhvgoyal updated PTAL

Contributor

vbatts commented Mar 17, 2015

@rhvgoyal updated PTAL

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 17, 2015

Contributor

and added a default for the tests to override the sync requirement

Contributor

vbatts commented Mar 17, 2015

and added a default for the tests to override the sync requirement

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

@vbatts

This patch looks good to me. I have a general question though about tests. Generic tests in docker are run with what graphdriver?

For example, say, I want to run integration tests and one of the test is whether container creation is successful or not. Will I run same test against all graph driver or there is a default graph driver which will always be used or something else?

Contributor

rhvgoyal commented Mar 17, 2015

@vbatts

This patch looks good to me. I have a general question though about tests. Generic tests in docker are run with what graphdriver?

For example, say, I want to run integration tests and one of the test is whether container creation is successful or not. Will I run same test against all graph driver or there is a default graph driver which will always be used or something else?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 17, 2015

Contributor

vfs is the driver the Docker-in-docker daemon is running for tests.

On Tue, Mar 17, 2015 at 1:35 PM, Vivek Goyal notifications@github.com
wrote:

@vbatts https://github.com/vbatts

This patch looks good to me. I have a general question though about tests.
Generic tests in docker are run with what graphdriver?

For example, say, I want to run integration tests and one of the test is
whether container creation is successful or not. Will I run same test
against all graph driver or there is a default graph driver which will
always be used or something else?


Reply to this email directly or view it on GitHub
#11412 (comment).

Contributor

vbatts commented Mar 17, 2015

vfs is the driver the Docker-in-docker daemon is running for tests.

On Tue, Mar 17, 2015 at 1:35 PM, Vivek Goyal notifications@github.com
wrote:

@vbatts https://github.com/vbatts

This patch looks good to me. I have a general question though about tests.
Generic tests in docker are run with what graphdriver?

For example, say, I want to run integration tests and one of the test is
whether container creation is successful or not. Will I run same test
against all graph driver or there is a default graph driver which will
always be used or something else?


Reply to this email directly or view it on GitHub
#11412 (comment).

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

Ok, thanks.

LGTM.

Contributor

rhvgoyal commented Mar 17, 2015

Ok, thanks.

LGTM.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

@vbatts

Thinking more about it. Actually this change is not upgrade proof. Those who are using device mapper and when they upgrade docker, suddenly docker daemon will stop running and that's not good.

Contributor

rhvgoyal commented Mar 17, 2015

@vbatts

Thinking more about it. Actually this change is not upgrade proof. Those who are using device mapper and when they upgrade docker, suddenly docker daemon will stop running and that's not good.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 17, 2015

Contributor

that's why the storage-opt flag is there. It's not supported in the way
they're running it presently.
if with the upgrade, they are then required to add the flag, it will be an
explicit indication that they're running it in an unsupported way, but
still have the ability to do so. Even if for migrating to a supported
setup, or switching to a docker binary that supports udev sync.

On Tue, Mar 17, 2015 at 1:41 PM, Vivek Goyal notifications@github.com
wrote:

@vbatts https://github.com/vbatts

Thinking more about it. Actually this change is not upgrade proof. Those
who are using device mapper and when they upgrade docker, suddenly docker
daemon will stop running and that's not good.


Reply to this email directly or view it on GitHub
#11412 (comment).

Contributor

vbatts commented Mar 17, 2015

that's why the storage-opt flag is there. It's not supported in the way
they're running it presently.
if with the upgrade, they are then required to add the flag, it will be an
explicit indication that they're running it in an unsupported way, but
still have the ability to do so. Even if for migrating to a supported
setup, or switching to a docker binary that supports udev sync.

On Tue, Mar 17, 2015 at 1:41 PM, Vivek Goyal notifications@github.com
wrote:

@vbatts https://github.com/vbatts

Thinking more about it. Actually this change is not upgrade proof. Those
who are using device mapper and when they upgrade docker, suddenly docker
daemon will stop running and that's not good.


Reply to this email directly or view it on GitHub
#11412 (comment).

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 17, 2015

Contributor

Generally I am not a big fan of such approach. Typically first we move users to a different storage driver slowly before switch is made.

Given that so many people are complaining, clearly lot of people are using it. They will be upset when upgraded docker suddenly does not start.

In kernel, we have a deprecated file where we put a feature which we want to deprecate. Send loud warnings if user is using those features in an effort to migrate them to alternate means. And once we are reasonably sure that most of the user's have migrated, we make the switch.

Personally, I would rather continue to give loud warnings and tell users to move to aufs or overlayfs or move to dynamic binary when they complain, instead of breaking working setup. Yes, flag is there, but it is very annoying to figure out that one needs to pass a new flag before one can proceed.

All I am saying that existing users might not like this change.

Contributor

rhvgoyal commented Mar 17, 2015

Generally I am not a big fan of such approach. Typically first we move users to a different storage driver slowly before switch is made.

Given that so many people are complaining, clearly lot of people are using it. They will be upset when upgraded docker suddenly does not start.

In kernel, we have a deprecated file where we put a feature which we want to deprecate. Send loud warnings if user is using those features in an effort to migrate them to alternate means. And once we are reasonably sure that most of the user's have migrated, we make the switch.

Personally, I would rather continue to give loud warnings and tell users to move to aufs or overlayfs or move to dynamic binary when they complain, instead of breaking working setup. Yes, flag is there, but it is very annoying to figure out that one needs to pass a new flag before one can proceed.

All I am saying that existing users might not like this change.

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Mar 18, 2015

Contributor

Perhaps calling it something like unsafe_override_udev_sync_check would send a very clear message that this isn't safe, nor supported. Whatever it's called, we probably want to have something like unsafe or unsupported in the flag itself to make it clear this voids the warranty.

Contributor

unclejack commented Mar 18, 2015

Perhaps calling it something like unsafe_override_udev_sync_check would send a very clear message that this isn't safe, nor supported. Whatever it's called, we probably want to have something like unsafe or unsupported in the flag itself to make it clear this voids the warranty.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 18, 2015

Contributor

@unclejack

I don't think that naming of flag is an issue here. Problem is that docker 1.5 was working in a setup and same setup will suddenly stop working on 1.6. And that's a problem.

We agree that whatever was working in 1.5 is not supportable, but that ship has sailed and people are already using it. I think instead of failing it completely, a better approach is to just give loud warnings that is is not supported and will go away in future, write documentation and point people to documentation.

And once we are reasonably sure that people have migrated to different solutions, then make this switch, IMHO.

Contributor

rhvgoyal commented Mar 18, 2015

@unclejack

I don't think that naming of flag is an issue here. Problem is that docker 1.5 was working in a setup and same setup will suddenly stop working on 1.6. And that's a problem.

We agree that whatever was working in 1.5 is not supportable, but that ship has sailed and people are already using it. I think instead of failing it completely, a better approach is to just give loud warnings that is is not supported and will go away in future, write documentation and point people to documentation.

And once we are reasonably sure that people have migrated to different solutions, then make this switch, IMHO.

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Mar 18, 2015

Contributor

@rhvgoyal You're right, but those users also need to know as soon as possible that they're exposing themselves and any potential clients they may have to data corruption/loss on the Docker daemons where they're running such a setup. These users need to migrate to dynamically linked Docker binaries if they're using devicemapper.

We need to provide apt and yum repositories which are distro specific in order to be able to provide dynamically linked binaires. That's a requirement for getting this merged, IMO.

Contributor

unclejack commented Mar 18, 2015

@rhvgoyal You're right, but those users also need to know as soon as possible that they're exposing themselves and any potential clients they may have to data corruption/loss on the Docker daemons where they're running such a setup. These users need to migrate to dynamically linked Docker binaries if they're using devicemapper.

We need to provide apt and yum repositories which are distro specific in order to be able to provide dynamically linked binaires. That's a requirement for getting this merged, IMO.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 18, 2015

Contributor

should we inverse this flag for now, so that we can make it available to
the pedantic. and once the requirements are met, we can switch this default
to fail support when udev sync is not supported?

On Wed, Mar 18, 2015 at 12:11 PM, unclejack notifications@github.com
wrote:

@rhvgoyal https://github.com/rhvgoyal You're right, but those users
also need to know as soon as possible that they're exposing themselves and
any potential clients they may have to data corruption/loss on the Docker
daemons where they're running such a setup.

We need to provide apt and yum repositories which are distro specific in
order to be able to provide dynamically linked binaires. That's a
requirement for getting this merged, IMO.


Reply to this email directly or view it on GitHub
#11412 (comment).

Contributor

vbatts commented Mar 18, 2015

should we inverse this flag for now, so that we can make it available to
the pedantic. and once the requirements are met, we can switch this default
to fail support when udev sync is not supported?

On Wed, Mar 18, 2015 at 12:11 PM, unclejack notifications@github.com
wrote:

@rhvgoyal https://github.com/rhvgoyal You're right, but those users
also need to know as soon as possible that they're exposing themselves and
any potential clients they may have to data corruption/loss on the Docker
daemons where they're running such a setup.

We need to provide apt and yum repositories which are distro specific in
order to be able to provide dynamically linked binaires. That's a
requirement for getting this merged, IMO.


Reply to this email directly or view it on GitHub
#11412 (comment).

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Mar 18, 2015

Contributor

@vbatts

By inversing the flag you mean provide a flag to fail initialization of devicemapper if udev sync is not supported. And without presence of flag, you will allow devicemapper to initialize?

If yes, I am not sure how useful it is. If somebody knows this issue then flag does not help him. And if somebody does not know the issue, then he does not know the flag either and then its of no use to him.

I think right now we seem to have two choices only and we need to pick our poison.

  • Just continue to give loud warnings and write documentation to point to users in right direction.
  • Implement this flag and in error message tell user why daemon has failed and if they want to
    override this behavior how to do it. That way, it will atleast make it easy for users to find this new
    flag. So this will still be annoying but I guess less painful. One will have to still write documentation
    to explain what are the alternatives.
Contributor

rhvgoyal commented Mar 18, 2015

@vbatts

By inversing the flag you mean provide a flag to fail initialization of devicemapper if udev sync is not supported. And without presence of flag, you will allow devicemapper to initialize?

If yes, I am not sure how useful it is. If somebody knows this issue then flag does not help him. And if somebody does not know the issue, then he does not know the flag either and then its of no use to him.

I think right now we seem to have two choices only and we need to pick our poison.

  • Just continue to give loud warnings and write documentation to point to users in right direction.
  • Implement this flag and in error message tell user why daemon has failed and if they want to
    override this behavior how to do it. That way, it will atleast make it easy for users to find this new
    flag. So this will still be annoying but I guess less painful. One will have to still write documentation
    to explain what are the alternatives.
@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 6, 2015

Contributor

We need to move this along.

@rhvgoyal as far as a break in experience, this PR is intended to do that. If the host is running with devcicemapper, and udev sync is not supported, then is should be disallowed. These conditions for running are broken, and ought to be blocked. That is why we added the flag to allow them to run the daemon again in the case of needing to export the images and containers for switching driver, or finding how to get a udev sync supported runtime (likely just a dynamically linked binary).

Contributor

vbatts commented Apr 6, 2015

We need to move this along.

@rhvgoyal as far as a break in experience, this PR is intended to do that. If the host is running with devcicemapper, and udev sync is not supported, then is should be disallowed. These conditions for running are broken, and ought to be blocked. That is why we added the flag to allow them to run the daemon again in the case of needing to export the images and containers for switching driver, or finding how to get a udev sync supported runtime (likely just a dynamically linked binary).

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Apr 6, 2015

Contributor

@vbatts

If you decide to push these patches in, it might be a good idea to also write a small documentation and suggest to affected users what they should do once they face this problem after upgrade.

Contributor

rhvgoyal commented Apr 6, 2015

@vbatts

If you decide to push these patches in, it might be a good idea to also write a small documentation and suggest to affected users what they should do once they face this problem after upgrade.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 6, 2015

Contributor

@rhvgoyal will do

Contributor

vbatts commented Apr 6, 2015

@rhvgoyal will do

devmapper: udev sync is a requirement
closes #10664
closes #4036

Signed-off-by: Vincent Batts <vbatts@redhat.com>
@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Apr 7, 2015

Contributor

LGTM

Contributor

rhvgoyal commented Apr 7, 2015

LGTM

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go
@@ -963,9 +965,11 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {
// https://github.com/docker/docker/issues/4036
if supported := devicemapper.UdevSetSyncSupport(true); !supported {
logrus.Warnf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors")
logrus.Errorf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see http://docs.docker.com/reference/commandline/cli/#daemon-storage-driver-option")

This comment has been minimized.

@tiborvass

tiborvass Apr 7, 2015

Collaborator

https

@tiborvass

tiborvass Apr 7, 2015

Collaborator

https

This comment has been minimized.

@vbatts

vbatts Apr 7, 2015

Contributor

... k
Though this should force a redirect if thats the case, because this is
where google sent me ...

On Tue, Apr 7, 2015 at 1:23 PM, Tibor Vass notifications@github.com wrote:

In daemon/graphdriver/devmapper/deviceset.go
#11412 (comment):

@@ -963,9 +965,11 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {

// https://github.com/docker/docker/issues/4036
if supported := devicemapper.UdevSetSyncSupport(true); !supported {
  •   logrus.Warnf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors")
    
  •   logrus.Errorf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see http://docs.docker.com/reference/commandline/cli/#daemon-storage-driver-option")
    

https


Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11412/files#r27899608.

@vbatts

vbatts Apr 7, 2015

Contributor

... k
Though this should force a redirect if thats the case, because this is
where google sent me ...

On Tue, Apr 7, 2015 at 1:23 PM, Tibor Vass notifications@github.com wrote:

In daemon/graphdriver/devmapper/deviceset.go
#11412 (comment):

@@ -963,9 +965,11 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {

// https://github.com/docker/docker/issues/4036
if supported := devicemapper.UdevSetSyncSupport(true); !supported {
  •   logrus.Warnf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors")
    
  •   logrus.Errorf("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see http://docs.docker.com/reference/commandline/cli/#daemon-storage-driver-option")
    

https


Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11412/files#r27899608.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 7, 2015

Contributor

fixed

Contributor

vbatts commented Apr 7, 2015

fixed

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Apr 7, 2015

Collaborator

LGTM

Ping @unclejack

Collaborator

tiborvass commented Apr 7, 2015

LGTM

Ping @unclejack

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Apr 7, 2015

Collaborator

@unclejack once you're good, please move to docs-review, thanks!

Collaborator

tiborvass commented Apr 7, 2015

@unclejack once you're good, please move to docs-review, thanks!

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack Apr 8, 2015

Contributor

LGTM

Contributor

unclejack commented Apr 8, 2015

LGTM

@tiborvass

This comment has been minimized.

Show comment
Hide comment
Collaborator

tiborvass commented Apr 8, 2015

@@ -216,3 +216,26 @@ Here is the list of supported options:
Example use:
``docker -d --storage-opt dm.blkdiscard=false``

This comment has been minimized.

@moxiegirl

moxiegirl Apr 9, 2015

Contributor

There were some awkward constructions that made this hard to parse and obscured the behavior. The Notes and Warning on top of each other indicate structural issues. Hopefully, I cleared this up with some input from @tiborvass (ty!).

Markdown: https://gist.github.com/moxiegirl/ade0634692f7423bf474

---->

  • dm.override_udev_sync_check

Overrides the kernel's udev synchronization checks. udev is the device
manager for the Linux kernel. By default, this value is false and the kernel
does not support override udev synchronization checks.

Under the false setting, race conditions occur between thedevicemapper and udev during create and cleanup. The race conditions result in errors and the
devicemapper fails. (For information on these failures, see
docker#4036)

To allow the docker daemon to function properly with the devicemapper
driver, set dm.override_udev_sync_check to true:

$ docker -d --storage-opt dm.override_udev_sync_check=true

When this value is true, the devicemapper continues and simply warns you the errors are happening

Note: The ideal is to pursue a docker daemon and environment that does
support synchronizing with Udev. For further discussion on this topic, see
docker#4036. Otherwise, set this flag for migrating existing daemons to a daemon
with a supported environment.

@moxiegirl

moxiegirl Apr 9, 2015

Contributor

There were some awkward constructions that made this hard to parse and obscured the behavior. The Notes and Warning on top of each other indicate structural issues. Hopefully, I cleared this up with some input from @tiborvass (ty!).

Markdown: https://gist.github.com/moxiegirl/ade0634692f7423bf474

---->

  • dm.override_udev_sync_check

Overrides the kernel's udev synchronization checks. udev is the device
manager for the Linux kernel. By default, this value is false and the kernel
does not support override udev synchronization checks.

Under the false setting, race conditions occur between thedevicemapper and udev during create and cleanup. The race conditions result in errors and the
devicemapper fails. (For information on these failures, see
docker#4036)

To allow the docker daemon to function properly with the devicemapper
driver, set dm.override_udev_sync_check to true:

$ docker -d --storage-opt dm.override_udev_sync_check=true

When this value is true, the devicemapper continues and simply warns you the errors are happening

Note: The ideal is to pursue a docker daemon and environment that does
support synchronizing with Udev. For further discussion on this topic, see
docker#4036. Otherwise, set this flag for migrating existing daemons to a daemon
with a supported environment.

@@ -370,6 +370,28 @@ Currently supported options are:
$ docker -d --storage-opt dm.blkdiscard=false
* `dm.override_udev_sync_check`

This comment has been minimized.

@moxiegirl

moxiegirl Apr 9, 2015

Contributor

Same as above.

@moxiegirl

moxiegirl Apr 9, 2015

Contributor

Same as above.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 10, 2015

Contributor

@moxiegirl I will update the wording. I see that it is not clear enough. :-)

Contributor

vbatts commented Apr 10, 2015

@moxiegirl I will update the wording. I see that it is not clear enough. :-)

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 10, 2015

Contributor

docs updated PTAL

Contributor

vbatts commented Apr 10, 2015

docs updated PTAL

@moxiegirl

This comment has been minimized.

Show comment
Hide comment
@moxiegirl

moxiegirl Apr 10, 2015

Contributor

@vbatts Thanks for the update. You missed a few ticks around udev and devicemapper line 381 --- but I can clean those up later. LGTM

Contributor

moxiegirl commented Apr 10, 2015

@vbatts Thanks for the update. You missed a few ticks around udev and devicemapper line 381 --- but I can clean those up later. LGTM

@moxiegirl

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
* `dm.override_udev_sync_check`
Overrides the `udev` synchronization checks between devicemapper and udev.

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

devicemapper udev ??? The use/non-use is confusing.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

devicemapper udev ??? The use/non-use is confusing.

Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
the activation and deactivation of devices for containers.
When `udev` sync is `false`, a race conditions occurs between
the`devicemapper` and `udev` during create and cleanup. The race conditions

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

condition

@jamtur01

jamtur01 Apr 10, 2015

Contributor

condition

Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
When `udev` sync is `false`, a race conditions occurs between
the`devicemapper` and `udev` during create and cleanup. The race conditions
result in errors and failures. (For information on these failures, see

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

results

@jamtur01

jamtur01 Apr 10, 2015

Contributor

results

Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
When this value is `true`, the `devicemapper` continues and simply warns
you the errors are happening.
**Note**: The ideal is to pursue a `docker` daemon and environment that

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Is this formatted as a proper note?

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Is this formatted as a proper note?

This comment has been minimized.

@moxiegirl

moxiegirl Apr 10, 2015

Contributor

No it isn't. That is my fault. @vbatts I hate to pepper ya with this formatting. If you have time to update. I've fixed the gist:

https://gist.github.com/moxiegirl/ade0634692f7423bf474

@moxiegirl

moxiegirl Apr 10, 2015

Contributor

No it isn't. That is my fault. @vbatts I hate to pepper ya with this formatting. If you have time to update. I've fixed the gist:

https://gist.github.com/moxiegirl/ade0634692f7423bf474

This comment has been minimized.

@vbatts

vbatts Apr 10, 2015

Contributor

ah. let me get that.

On Fri, Apr 10, 2015 at 3:02 PM, moxiegirl notifications@github.com wrote:

In daemon/graphdriver/devmapper/README.md
#11412 (comment):

  • the activation and deactivation of devices for containers.
  • When udev sync is false, a race conditions occurs between
  • thedevicemapper and udev during create and cleanup. The race conditions
  • result in errors and failures. (For information on these failures, see
  • docker#4036)
  • To allow the docker daemon to start, regardless of udev sync not being
  • supported, set dm.override_udev_sync_check to true:
  •    $ docker -d --storage-opt dm.override_udev_sync_check=true
    
  • When this value is true, the devicemapper continues and simply warns
  • you the errors are happening.
  • Note: The ideal is to pursue a docker daemon and environment that

No it isn't. That is my fault. @vbatts https://github.com/vbatts I hate
to pepper ya with this formatting. If you have time to update. I've fixed
the gist:

https://gist.github.com/moxiegirl/ade0634692f7423bf474


Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11412/files#r28171493.

@vbatts

vbatts Apr 10, 2015

Contributor

ah. let me get that.

On Fri, Apr 10, 2015 at 3:02 PM, moxiegirl notifications@github.com wrote:

In daemon/graphdriver/devmapper/README.md
#11412 (comment):

  • the activation and deactivation of devices for containers.
  • When udev sync is false, a race conditions occurs between
  • thedevicemapper and udev during create and cleanup. The race conditions
  • result in errors and failures. (For information on these failures, see
  • docker#4036)
  • To allow the docker daemon to start, regardless of udev sync not being
  • supported, set dm.override_udev_sync_check to true:
  •    $ docker -d --storage-opt dm.override_udev_sync_check=true
    
  • When this value is true, the devicemapper continues and simply warns
  • you the errors are happening.
  • Note: The ideal is to pursue a docker daemon and environment that

No it isn't. That is my fault. @vbatts https://github.com/vbatts I hate
to pepper ya with this formatting. If you have time to update. I've fixed
the gist:

https://gist.github.com/moxiegirl/ade0634692f7423bf474


Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/11412/files#r28171493.

Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
you the errors are happening.
**Note**: The ideal is to pursue a `docker` daemon and environment that
does support synchronizing with Udev. For further discussion on this topic,

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Consistency of use - udev/Udev.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Consistency of use - udev/Udev.

Show outdated Hide outdated daemon/graphdriver/devmapper/README.md
**Note**: The ideal is to pursue a `docker` daemon and environment that
does support synchronizing with Udev. For further discussion on this topic,
see [docker#4036](https://github.com/docker/docker/issues/4036).
Otherwise, set this flag for migrating existing daemons to a daemon with a

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

migrating existing Docker daemons ...

@jamtur01

jamtur01 Apr 10, 2015

Contributor

migrating existing Docker daemons ...

Show outdated Hide outdated docs/sources/reference/commandline/cli.md
@@ -370,6 +370,41 @@ Currently supported options are:
$ docker -d --storage-opt dm.blkdiscard=false
* `dm.override_udev_sync_check`
Overrides the `udev` synchronization checks between devicemapper and udev.

This comment has been minimized.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Comments as above.

@jamtur01

jamtur01 Apr 10, 2015

Contributor

Comments as above.

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Apr 10, 2015

Contributor

@vbatts Did you push an update? Perhaps wait until I get a chance to finish the review? :) Makes it hard to go back and see what's changed.

Contributor

jamtur01 commented Apr 10, 2015

@vbatts Did you push an update? Perhaps wait until I get a chance to finish the review? :) Makes it hard to go back and see what's changed.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 10, 2015

Contributor

@jamtur01 sorry! i thought i saw them slow down, so i pushed :-\

Contributor

vbatts commented Apr 10, 2015

@jamtur01 sorry! i thought i saw them slow down, so i pushed :-\

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Apr 10, 2015

Contributor

No worries - love the lightning fast fixes! :)

Contributor

jamtur01 commented Apr 10, 2015

No worries - love the lightning fast fixes! :)

Show outdated Hide outdated docs/sources/reference/commandline/cli.md
When `udev` sync is supported, then devicemapper and udev can coordinate
the activation and deactivation of devices for containers.
When `udev` sync is `false`, a race conditions occurs between

This comment has been minimized.

@tiborvass

tiborvass Apr 10, 2015

Collaborator

udev sync is not supported

or

udev sync support is false

Ping @moxiegirl @jamtur01

@tiborvass

tiborvass Apr 10, 2015

Collaborator

udev sync is not supported

or

udev sync support is false

Ping @moxiegirl @jamtur01

This comment has been minimized.

@vbatts

vbatts Apr 10, 2015

Contributor

ah, perhaps "supported" would be good there.

@vbatts

vbatts Apr 10, 2015

Contributor

ah, perhaps "supported" would be good there.

devmapper: storage-opt override for udev sync
This provides an override for forcing the daemon to still attempt
running the devicemapper driver even when udev sync is not supported.

Intended to be a very clear impairment for those choosing to use it. If
udev sync is false, there will still be an error in the daemon logs,
even when the override is in place. The docs have an explicit WARNING.

Including link to the docs for users that encounter this daemon error
during an upgrade.

Signed-off-by: Vincent Batts <vbatts@redhat.com>
@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 10, 2015

Contributor

Notes formatting updated. PTAL

Contributor

vbatts commented Apr 10, 2015

Notes formatting updated. PTAL

@moxiegirl

This comment has been minimized.

Show comment
Hide comment
@moxiegirl

moxiegirl Apr 10, 2015

Contributor

@jamtur01 ping back...let us know if you are satisfied. :D

Contributor

moxiegirl commented Apr 10, 2015

@jamtur01 ping back...let us know if you are satisfied. :D

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Apr 10, 2015

Contributor

Docs LGTM

Contributor

jamtur01 commented Apr 10, 2015

Docs LGTM

tiborvass added a commit that referenced this pull request Apr 10, 2015

@tiborvass tiborvass merged commit 23c12da into moby:master Apr 10, 2015

2 checks passed

janky Jenkins build Docker-PRs 5500 has succeeded
Details
windows Jenkins build Windows-PRs 2471 has succeeded
Details

@vbatts vbatts deleted the vbatts:vbatts-dm_sync_is_required branch Apr 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment