Sarah Jelinek wrote: > Hi Ethan, > > My comments inline.. > > >> Hi Sarah, >> >> Additional comments in line, (filling in the gaps ...) >> >> Evan Layton wrote: >>> Some comments inline. >>> (more to come...) >>> >>> Sarah Jelinek wrote: >>>> Hi All, >>>> >>>> I have some review comments on the BE library for Snap Upgrade >>>> Design Specification, Revision 0.5. Apologies for the delay. >>>> >>>> >>> It's not like this reply was quick either. ;-) >>> >>>> -Section 1.2, the BE definition states: >>>> The BE is currently defined by it's name which is the ZFS root pool >>>> it's in and the datasets that belong to it. For example a BE would >>>> have the following datasets... >>>> >>>> I wonder if we can be more precise here about what datasets are >>>> required to meet the BE requirements? And, what are optional or >>>> supported configurations. The reason I ask is that if a user has >>>> separate datasets, or multiple BE's in a root pool, upgrading the >>>> datasets via snap upgrade can mean different things depending on >>>> supported or required configurations. >>>> >>> The only real requirement here is the root dataset. This is the >>> dataset under rpool/ROOT and is the name of the BE. Any other dataset >>> under that is not necessarily required however in may instances a >>> separate /var is required to meet security requirements. There were >>> arguments from some that the BE should just be rpool/ROOT/<be_name> >>> but what we've proposed gives a bit more flexibility. The fact that >>> anything under rpool/ROOT/<be_name> is considered part of the BE >>> means that it really doesn't matter what's there when we do an upgrade. >>>> -Section 2.1, your diagram shows IPS but it doesn't show Transfer. I >>>> don't know if this service will be required or not at the time snap >>>> is available, but I am wondering if you had thought about this? You >>>> note the transfer module in your comments, but it isn't clear to me >>>> how you envision this all to work together. >>>> >>> We mount the BE (and all of it's associated datasets on say /a) and >>> pass that mounted area to the transfer module to install into. >>> (created, mounted and then that mount point is passed to the transfer >>> module). >> And just to clarify, the entity passing that mounted dir to the >> Transfer module is the installer. So what this means is that the >> Transfer module's relationship with the Installer should look like >> IPS's relationship with the Installer; we'll add it to the diagram. >> Currently the installer calls the Transfer module with the target dir >> already created and mounted so there isn't any interaction between >> the Transfer module and TI or the BE library, and I don't see this >> changing. >> > Ok, this makes sense. Thanks for the clarification. > >> If later on we support installing directly from a repo, then the >> Transfer module would likely have a relationship with IPS. >> > Yeah.. that's kind of what I was wondering about. I do think we, the > installer anyway, needs to support installing, upgrading directly from a > repo. The we must install what is on the media is a short term > restriction for expediency. > >>>> -Section 2.2, your diagram shows the libbe invoking the copy but the >>>> text says the 'installer' will do the copy. Just checking to see who >>>> does what. >> The UFS to ZFS migration design is not complete. We'll try to have >> it included with the next rev of the design. But in a nutshell, >> the installer supports the case where there is no additional local >> storage to create a ZFS BE to migrate to. This case requires DSR. >> The installer will be doing the copy of the bits to and fro the >> alternate location. >> >> The other case is when there is additional local storage to create >> a zpool and a ZFS BE to migrate to. For this case though, theres >> no reason not to use the BE cli to accomplish the migration. >> > ok. this makes sense. >>>> -Section 3.1.1, Why would we use a file to hold policy information. >>>> Maybe you don't mean 'file' exactly, but it seems like we could do a >>>> better job than just a flat file. Something like SMF extended profiles. >> We're considering storing this policy information in SMF. >> > Ok. >>>> -Section 3.1.1.1, what are the class definitions for the snapshots? >> These are the policy definitions for snapshots. >> > right, I understand that. What I meant to ask is what are the currently > defined class definitions for policy? Are there any that will be > standard? And, I see you answered this below... >>>> I would think we would want pre-defined types, and perhaps the >>>> ability to allow user defined types. >> That's the idea. We will deliver a default policy class for snapshot >> retention. The default could be something like: >> >> snap_num_max = 12 # maximum num of snapshots to keep >> snap_age_max = 336 # maximum number of hours to keep the snapshot >> snap_storage_free = 80 # % level of pool storage usage before deleting >> # oldest snapshots. >> >> And we'd also deliver a policy class for "infinity". i.e. these >> snapshots would never get deleted unless explicitly called to get >> deleted via be_destroy(). >> >> Our intent was also to allow the caller to define their own >> snapshot policy class, and be able to call into libbe to and create >> snapshots managed via that policy. A caller defined snapshot policy >> class gets defined by delivering a file into a specified area as >> noted in 3.1.1.2-rev 0.5 (or stored in SMF if we decide to do that) >> >> However, we may not end up implementing support for these caller >> defined policy classes. Right now the only consumers of this are IPS, >> the BE cli, and the installer, and they would all likely use either >> the default class or the infinity class. >> >>> But there are issues with user defined types >>>> being understood by the software. Maybe the snapshot policy, instead >>>> of being a file or an attribute on a class we define, could be >>>> something ZFS could add itself as one of the snapshot dataset >>>> attributes? This would be cleaner in my opinion. >> We thought about this some, but since there is going to be multiple >> policy attributes (at least three right now), we didn't want to store >> it all as dataset attributes. >> >> The one thing we hadn't cleanly figured out yet was how to tie a BE >> snapshot to which policy class it belonged to. This perhaps could be >> something we store as a snapshot dataset attribute. (Was that what >> you meant?) >> > Yes, that's what I meant. >>>> -Who manages the retention policies? libbe? The text says that libbe >>>> will provide a way to delete, display, create snapshots, but if a >>>> policy indicates a retention of say 5 hours, who goes back and >>>> deletes this? In >> libbe enforces the retention policy, but its more of a "passive" >> enforcement. i.e. we won't be having a be-daemon running around >> checking statuses of BE snapshots. Instead, on each invocation of >> BE or BE snapshot creation, we run through the snapshots and delete >> the ones that have outstayed their policy. >> > Ah, ok, I see. I assume users can create agents to handle this policy > management more actively if they wanted? >
We might want to provide a cron job or something which can be simply enabled to implement the policy actively. The problem I see with doing this purely passively is that the cleanup can be slightly time-consuming, and as a result I think you'd need to offer options on the BE management to defer it from happening automatically at BE creation/snapshotting when it would be inconvenient for the user. Dave
