Re: [atomic-devel] Running docker-storage-setup from a UI

[Vivek Goyal <vgoyal redhat com>]

On Thu, Apr 14, 2016 at 10:45:03AM +0300, Marius Vollmer wrote:
> Vivek Goyal <vgoyal redhat com> writes:
> 
> > On Wed, Apr 13, 2016 at 10:36:40AM +0300, Marius Vollmer wrote:
> >
> >> [..] So any user-prompting from docker-storage-setup is a bug?  I get
> >> a prompt from pvcreate sometimes, for example when the random data on
> >> the new partition has a filesystem signature at the beginning.  With
> >> what I know now, this is probably unintended.  Should I file an
> >> issue?  Lvm likes to add confirmation prompts like this over
> >> time... :)
> >
> > I think if it runs as service, then no terminal is attached and it
> > assumes a default answer. I guess yes. Give it a try.
> 
> Here what happens with stdin from /dev/null:
> 
>     # docker-storage-setup </dev/null
>     Checking that no-one is using this disk right now ... OK
> 
>     Disk /dev/sda: 2 GiB, 2147483648 bytes, 4194304 sectors
>     Units: sectors of 1 * 512 = 512 bytes
>     Sector size (logical/physical): 512 bytes / 512 bytes
>     I/O size (minimum/optimal): 512 bytes / 512 bytes
> 
>     >>> Script header accepted.
>     >>> Created a new DOS disklabel with disk identifier 0x79a24362.
>     Created a new partition 1 of type 'Linux LVM' and of size 2 GiB.
>     /dev/sda2:
>     New situation:
> 
>     Device     Boot Start     End Sectors Size Id Type
>     /dev/sda1        2048 4194303 4192256   2G 8e Linux LVM
> 
>     The partition table has been altered.
>     Calling ioctl() to re-read partition table.
>     Syncing disks.
>     WARNING: xfs signature detected on /dev/sda1 at offset 0. Wipe it? [y/n]: n
>       Aborted wiping of xfs.
>       1 existing signature left on the device.
>       Aborting pvcreate on /dev/sda1.
> 
> In your opinion, is it a bug or a feature that pvcreate might prompt
> like this?

I think this is default behavior of pvcreate. By default it will not
create lvm signature on disk if it finds other signatures on disk. This
is not a bug and it is trying to be safe.

> 
> >> Why does d-s-s run during boot, actually?  Shouldn't people run it
> >> interactively after changing /e/s/d-s-s?  Or does it only run on
> >> first boot?
> >
> > [ - Get things right automatically during first boot.
> >   - Wait for thin pool to come up
> >   - Automatically grow pvs backing volume group
> > ]
> 
> Thanks for the explanation!
> 
> >> If you think that a --force-wipe option is not appropriate for d-s-s,
> >> Cockpit can wipe the devices itself.  But I think the option is nicer.
> >
> > I don't know, wiping partitions is always risky by default.
> 
> It's an option, it would not be the default behavior.

Sure. Even if you want to introduce option, it might be better to add
a config option in /usr/lib/docker-storage-setup/docker-storage-setup

Docker storage setup reads all the knobs from there and tweaks its
behavior.

> 
> > User might lose important data. If cockpit can do it, that would be
> > great. If there is a strong demand for this, we can add it, but nobody
> > has asked for it yet.
> 
> I have, and I think there was some demand earlier in the thread.  I
> can contribute the code and am happy to look after bugs with it.

If you think this is important to add, open a PR and add an option say
FORCE_WIPE_DISKS=false

> 
> > [d-s-s --status]
> >
> > So all this current config detection can be easily done outside d-s-s
> > as well. I think it probably would be better if you run various lvm
> > commands to figure out volumes, thin-pools, pvs and display whatever
> > information is useful?
> 
> I am not sure about "easy". :-) I was hoping that d-s-s can provide a
> higher-level abstraction that would make it unnecessary to agree on a
> (ever growing) set of low-level conventions.
> 
> I'd say we need to have some code somewhere for inspecting the current
> state of the docker storage pool.  Having it in d-s-s itself would be
> the natural place, IMO, and would make it accessible for others and give
> it a fighting chance to keep working.  Having it in Cockpit would create
> a pretty high risk that things get out of sync, and nobody else would
> benefit from it.

dss is more of a service. So tyring to use it as command line with various
options might not be best fit. But if you have concrete user stories, I 
am open to changes as long as it does not make the existing code messy.

> 
> (The same is true for --force-wipe.)

> 
> >> I think a reliable "other" flag will be interesting for a potential
> >> "docker-storage-setup reset" operation.  This would remove the
> >> current pool and create a new one, so that the user can meaningfully
> >> remove devices from DEVS again.
> >
> > If user removes the thin pool, a new one will be created. "lvremove
> > docker-vg/docker-pool".
> 
> (You need to remove /etc/sysconfig/docker-storage as well before d-s-s
> will make a new pool.  Also stopping docker and rm /var/lib/docker are
> probably good ideas.)

Lot of people have complained about being able to reset the state of
storage. So it is a good idea to provide something there. But I think
we need something outside of docker-storage-setup, say atomic. Reason
being that resetting the state of storage also requires stopping docker
and cleaning up /var/lib/docker and docker-storage-setup should not be
the one touching /var/lib/docker/.

Once Dan had suggested an atomic command say "reset-docker" or
"reinit-docker" or something else. That might be a good place to
cleanup docker state as well as docker-storage-setup state. To make
thins simple I don't mind providing a command in dss which can do
some cleanup for storage state.

So say a dss command "reset" which will remove thin pool and remove
/etc/sysconfig/docker-storage should be reasonable. But one will
have to make sure docker has been stopped and /var/lib/docker/ has
been removed in advance other bad things will happen. And that's where
a command in atomic to deal with it will help.
> 
> > I really don't see a need for a new command or option.
> 
> But maybe others do.  I hope that counts for something. :-)

Sure, if you have convincing story, I am open to it. 

> 
> > Don't want to make a simple bash script too complicated until and
> > unless it is really really needed.
> 
> If we would follow your arguments until the end, there would be no
> reason to have d-s-s at all. :-) Users can 'easily' create the necessary
> thin pools with a few lvm command line calls and write the right
> incantation to /e/s/docker-storage.

Nope, we needed a service which could do it automatically on every
boot and it was hard to do with lvm commands. And it was a concrete
need.

I am against adding too much of functionality and code without lot of
thinking otherwise it becomes maintenance burden soon.

> 
> But we give them a tool since doing all this is in fact non-trivial and
> a common use case.  Let's improve the tool to be more robust and cover
> more use cases.  That is the fate of all useful software, it will grow.
> I would be happy to help.

If we have good concrete use cases and stories, I am open to improve
things incrementally.

> 
> "d-s-s reset" would allow people to switch storage drivers, switch from
> loopback to thinpool, and remove disks from the pool again in a simple
> and restricted, but robust and supported way.

Lets implement "atomic reset-docker" and that in turn can call dss reset.

Thanks
Vivek