<html> <head> <meta content="text/html; charset=koi8-r" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> 07-Oct-16 20:09, Olga Krishtal пишет: <blockquote cite="mid:e109c6ea-ea00-757d-2bab-d7ff7c85ca34@virtuozzo.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=koi8-r"> <div class="moz-cite-prefix">On 24/09/16 00:12, John Ferlan wrote: </div> <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap=""> On 09/23/2016 11:56 AM, Olga Krishtal wrote: </pre> <blockquote type="cite"> <pre wrap="">On 21/09/16 19:17, Maxim Nestratov wrote: </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="">20 сент. 2016 г., в 23:52, John Ferlan <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:jferlan@redhat.com"><jferlan@redhat.com></a> написал(а): </pre> <blockquote type="cite"> <pre wrap="">On 09/15/2016 03:32 AM, Olga Krishtal wrote: Hi everyone, we would like to propose the first implementation of fspool with directory backend. Filesystem pools is a facility to manage filesystems resources similar to how storage pools manages volume resources. Furthermore new API follows storage API closely where it makes sense. Uploading/downloading operations are not defined yet as it is not obvious how to make it properly. I guess we can use some kind of tar to make a stream from a filesystem. Please share you thoughts on this particular issue. </pre> </blockquote> <pre wrap="">So how do you differentiate between with the existing <pool type="fs"> </pre> </blockquote> <pre wrap="">Pool type=fs still provides volumes, i. e. block devices rather than filesystem, though this storage pool can mount file systems resided on a source block device. </pre> <blockquote type="cite"> <pre wrap=""><a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://libvirt.org/storage.html#StorageBackendFS">http://libvirt.org/storage.html#StorageBackendFS</a> Sure the existing fs pool requires/uses a source block device as the source path and this new variant doesn't require that source but seems to use some item in order to dictate how to "define" the source on the fly. Currently only a "DIR" is created - so how does that differ from a "dir" pool. </pre> </blockquote> <pre wrap="">Same here, storage "dir" provides files, which are in fact block devices for guests. While filesystem pool "dir" provides guests with file systems. </pre> <blockquote type="cite"> <pre wrap="">I think it'll be confusing to have and differentiate fspool and pool commands. </pre> </blockquote> </blockquote> <pre wrap="">Some time ago, we wrote the proposal description and asked for everyone's advice and opinion. The aim of fspool is to provide filesystems, not volumes. The simplest type of fspool is directory pool and it do has a lot in common with storage_backend_fs. However, in the proposal description we said that the plan is to use other backends: eg, storage volumes from storage pool as the source of fs, zfs, etc. The final api for fspool will be significantly different, because of the other backends needs. </pre> </blockquote> <pre wrap="">Can you please try to create an extra line after the paragraph you're responding to and the start of your paragraph and then one after. </pre> </blockquote> Thanks for noticing. It looks better. <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap="">Anyway, as I pointed out - that description wasn't in my (short term) memory. Keeping a trail of pointers to previous stuff helps those that want to refresh their memory on the history. </pre> </blockquote> I will hold this links through the next versions. <a moz-do-not-send="true" href="https://www.redhat.com/archives/libvir-list/2016-April/msg01941.html">https://www.redhat.com/archives/libvir-list/2016-April/msg01941.html </a> <a moz-do-not-send="true" href="https://www.redhat.com/archives/libvir-list/2016-May/msg00208.html">https://www.redhat.com/archives/libvir-list/2016-May/msg00208.html</a> <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap="">If you're going to "reuse" things, then using the 'src/util/*' is the way to go rather than trying to drag in storage_{driver|backend*} APIs. Crossing driver boundaries is something IIRC we try to avoid. </pre> </blockquote> As I have written before at the moment we have only one backend for fspool - directory. It is the simplest backend and only the starting point. I think that it is too early to decide which parts should be moved to src/util/*. Moreover, as fspool items and storage pool volumes are pretty different, it could be possible that they have very little in common. That said, I would leave things as they are, but if you insist I can try. </blockquote> If you meant the resulting code will have very little in common, then I would agree here. More backends implementation will show us where common parts are and we will have more basis for splitting out common parts. <blockquote cite="mid:e109c6ea-ea00-757d-2bab-d7ff7c85ca34@virtuozzo.com" type="cite"> <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="">I didn't dig through all the patches, but from the few I did look at it seems as though all that's done is to rip out the guts of stuff not desired from the storage pool driver and replace it with this new code attributing all the work to the new author/copyright. IOW: Lots of places where StoragePool appears to be exactly the same as the FSPool.</pre> </blockquote> </blockquote> </blockquote> </blockquote> I have written this lines as a part of GPLv2+ boilerplate: <a moz-do-not-send="true" href="https://www.redhat.com/archives/libvir-list/2016-August/msg01160.html">https://www.redhat.com/archives/libvir-list/2016-August/msg01160.html</a>, which I took from other libvirt parts. And I guess it was naturally to change name and company, don't you? And again, if you insist I can leave out the author/copyright as it wasn't the aim of this series.</blockquote> Indeed, storage pool is very similar to FS pool but their items are not - volumes (block devices) versus filesystems (directory trees). And intention here was to introduce a *new API*, which is also very different from storage pool one, effectivly introducing a new driver. As driver boundaries crossing isn't favored, the code was simply borrowed, following earlier practice used by libvirt to get new drivers implemented. John, keeping all said above in mind, do you think it's worth trying to reuse common code while introducing a new API? It won't allow us to leave existing code untouched and it will increase the series even more. <blockquote cite="mid:e109c6ea-ea00-757d-2bab-d7ff7c85ca34@virtuozzo.com" type="cite"> <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre wrap=""> I think you need to find a different means to do what you want. It's not 100% what the end goal is.I did download/git am the patches and scan a few patches... * In patch 2 you've totally missed how to modify libvirt_public.syms * In patch 3, the build breaks in "conf/fs_conf" since the "if { if {} }" aren't done properly in virFSPoolDefFormatBuf. * In patch 5 the remote_protocol_structs fails check/syntax-check... I stopped there in my build each patch test. </pre> </blockquote> </blockquote> <pre wrap="">According to the guide I have to do: |make check| and |make syntax-check for every patch| </pre> </blockquote> <pre wrap="">Always a good plan! </pre> <blockquote type="cite"> <pre wrap="">And it was done. </pre> </blockquote> <pre wrap="">And yet as we find out *all the time* some compilers complain more than others. Watch the list - we have a CI environment in which we find all sorts of oddities. In any case, the code in question is: + if (def->target.perms.mode != (mode_t) -1 || + def->target.perms.uid != (uid_t) -1 || + def->target.perms.gid != (gid_t) -1 || + def->target.perms.label) { + virBufferAddLit(buf, "<permissions>\n"); + virBufferAdjustIndent(buf, 2); + if (def->target.perms.mode != (mode_t) -1) + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); + if (def->target.perms.uid != (uid_t) -1) + virBufferAsprintf(buf, "<owner>%d</owner>\n", + (int) def->target.perms.uid); + if (def->target.perms.gid != (gid_t) -1) + virBufferAsprintf(buf, "<group>%d</group>\n", + (int) def->target.perms.gid); + virBufferEscapeString(buf, "<label>%s</label>\n", + def->target.perms.label); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</permissions>\n"); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</target>\n"); So do you "see" the problem? The first if has an open {, but the second one doesn't, although the code is indented and would seemingly want to have one. The second one has a close }, which gives the impression something is missing. </pre> </blockquote> Thanks for pointing this out. I will be more attentive. <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">At libvirt_public.syms I will look one more time. </pre> </blockquote> <pre wrap="">You added all those API's into LIBVIRT_2.0.0 { above the "LIBVIRT_1.3.3", but you'll notice there's a LIBVIRT_2.2.0 { afterwards which all those API's should have gone "at least for now". IOW: When adding new API's you have to add to the version specific stanza. As of now you'd be adding API's to 2.3.0, but I doubt we'll make that cut-off.</pre> </blockquote> Thanks. Will do. <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap=""> Just because you added them in 2.0.0 internally, once you go to upstream them - they need to be in the latest. So this brings me back to my other recent response. I think if we can figure out what the driver will need, then work through the external API portion. There's just so much going on - trying to get a sense of it all at once is well overwhelming. </pre> </blockquote> Actually, I thought we have already figured it out and it was decided to have completely separate API to manage filesystems but similar to storage pool's, and only this approach let containers get the greatest benefit from it. </blockquote> Let me share my thoughts here. The minimum that this new API needs is ability to: - define (create a persistent FS pool), - create (create a transisional FS pool), - build (for directory/remotefs pools it is simply directory creation, for block device backend, for instanse, it will be a mkfs call ) - start/stop (activate/deactivate), - create/delete items (subdirectories), - undefine, - maybe ability to use storage pool volumes as sources for FS pools. Create and build flags will control whether we should overwrite existing directory content or leave it untouched. But currently they are not used in code and it is really difficult to guess what is their purpose. This certainly should be fixed in the next revision of the series. Maxim <blockquote cite="mid:e109c6ea-ea00-757d-2bab-d7ff7c85ca34@virtuozzo.com" type="cite"> <blockquote cite="mid:df0a2eef-b049-2dde-6750-c61b8c0d7bf0@redhat.com" type="cite"> <pre wrap="">John [...] </pre> </blockquote> <pre class="moz-signature" cols="72">-- Best regards, Olga</pre> </blockquote> </body> </html>