[ https://issues.apache.org/jira/browse/HDFS-10899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15858880#comment-15858880 ]
Andrew Wang commented on HDFS-10899: ------------------------------------ Hi Xiao, I soaked in this patch for a while, had a bunch of review comments. I didn't get through everything, but I think this is worth posting now, and I'll give a more detailed review of the next rev: ReencryptionHandler: * Rather than having get/setProvider, can we set it in the EZManager constructor by getting it from {{dir.getFSNamesystem().getProvider()}} ? Then we don't need the retry loop in {{run}} either. * In FSEditLog, normally we log one edit per call to a {{log}} function. This is broken by {{logReencryptZoneStatus}}, could you change it so ReencryptionHandler does the for loop over the batch calling {{logSetXAttrs}} then a final call to {{logReencryptZoneStatus}} ? * We shouldn't call logSync inside the write lock in reencryptDir, since it's a time consuming operation. In the RPC context, we call this outside the lock, just before responding to the caller. This means there's a little gap where readers could see uncommitted data. For reencrypt, this is actually very safe since the old and new EDEK are functionally equivalent, and we aren't providing any guarantees while the zone is still reencrypting. * We need the write lock held when logging to the FSEditLog. * I'd prefer we gather all the file EDEK information in one shot with just the read lock held, do all the KMS communication, then take the writelock at the end to log the batch. This also makes it easier to multi-thread the KMS communication stage later, and better performance since we're doing more work under the lock. * There are lock/unlocks across functions which makes this harder to understand. For instance, unlock then relock in reencryptINode because it was locked in the caller. Consider refactoring? It'd be great if all locking was handled with simple lock/unlock in try/finally. * restoreReencryptFromProgress, javadoc mentions {{lastFileProcessed}} but abbreviations here and below are {{lpf}}. * Is it possible to rework reencryptEncryptionZoneInt so we only call reencryptDir once after setting up the iteration state? I think this would also make restoreReencryptFromProgress clearer, since it could have fewer arguments. * Would also consider renaming restoreReencryptFromProgress, like {{restoreFromLastFileProcessed}} * Is it possible to encapsulate the file iteration as an iterator? We pass {{subdirs}} around a lot right now. ReencryptionStatus: * Add Precondition check to add, to make sure that it's not added if already present EZManager: * typo: zondId * What does the "Id" in {{getIdRootEncryptionZone}} mean? * getIdRootEncryptionZone checks for read in resolvePath, but aren't reencrypt and cancel write operations? We don't want to be running this on snapshot paths. I think we should push the resolvePath up to the caller of this function, since it also lets us reuse the IIP if necessary (resolvePath is expensive). * In FSNamesystem, looks like we call {{resolvePath}} in reencryptEncryptionZoneInt but not the IIP version. The normal flow is to resolvePath to an IIP in the corresponding FSDir file, then pass it down to avoid resolving again for performance reasons. ReencryptionHandler#checkNNReadyForWrite and catching exceptions * {{checkNNReadyForWrite}} checks that the NN is active. The handler should only be live when the NN is active though, stopActiveServices will interrupt the thread. If we're worried about this, it'd be better to periodically check for the interrupt and throw InterruptedException. * With this, we can change {{checkNNReadyForWrite}} into a simple safemode check. Since exceptions are expensive, I'd prefer a boolean return value and if statement instead. * Note also that safemode checks need to be done with the lock held, so we should do it where we're grabbing the lock anyway. For correctness, I think we only need to check this just before we do a write (i.e. apply the new EDEKs to our in-memory state). We can still check elsewhere to save doing extra work. * Related to the above, in two places in {{run}} we catch InterruptedException and just log and return. This is a code smell; although it's the top-level function, it's still better to re-interrupt the thread before returning. * I like this article I found on InterruptedException: https://www.ibm.com/developerworks/library/j-jtp05236/ * Catching Exception in {{run}} is a code smell. What is the intent? It looks like we already catch the checked exceptions, so this will catch RuntimeExceptions (which are normally unrecoverable). Metrics: * I'd prefer that we track the per zone metrics in ReencryptionStatus, since it's what also tracks the currentZone. This avoids having to reset the metrics on removeZone. * Follow-on idea: it'd be nice for admins to be able to query the status of queued and running reencrypt commands. Progress indicators like submission time, start time, # skipped, # reencrypted, total # (if this is cheap to get) would be helpful. Others: * Looks like we aren't using the op cache in FSEditLog SetXAttrOp / RemoveXAttrOp. I think this is accidental, could you do some research? Particularly since we'll be doing a lot of SetXAttrOps, avoiding all that object allocation would be nice. This could be a separate JIRA. * DFSTestUtil, the new comment says "Pause re-encrypt to allow the cancel command can go through", but IIUC this isn't about authorization but instead pausing reencryption so there's an active zone reencryption to cancel. Could you reword? * fsimage.proto and loader code, since the lastFile belongs to a specific zone, I'd prefer we define the on-disk format like this rather than needing to know about zone ordering: * Rather than making FSNamesystem#getFileInfo public for tests, can we dig it out from the RPC interface? {code} message EncryptionManagerSection { message ReencryptCommand { required uint64 zone = 1; optional string lastFile = 2; } repeated ReencryptCommand reencryptCommands = 1; } {code} With the current on-disk format, we can only reencrypt one zone at a time. Later, we might want to reencrypt in parallel. Follow-on: * This could be difficult with all the lock/unlocks and stages, but I'd prefer a goal-pause-time configuration for the {{run}} loop. This is easier for admins to reason about. We would still use the batch size for determining when to log a batch. > Add functionality to re-encrypt EDEKs. > -------------------------------------- > > Key: HDFS-10899 > URL: https://issues.apache.org/jira/browse/HDFS-10899 > Project: Hadoop HDFS > Issue Type: New Feature > Components: encryption, kms > Reporter: Xiao Chen > Assignee: Xiao Chen > Attachments: editsStored, HDFS-10899.01.patch, HDFS-10899.02.patch, > HDFS-10899.03.patch, HDFS-10899.04.patch, HDFS-10899.05.patch, > HDFS-10899.06.patch, HDFS-10899.07.patch, HDFS-10899.wip.2.patch, > HDFS-10899.wip.patch, Re-encrypt edek design doc.pdf > > > Currently when an encryption zone (EZ) key is rotated, it only takes effect > on new EDEKs. We should provide a way to re-encrypt EDEKs after the EZ key > rotation, for improved security. -- This message was sent by Atlassian JIRA (v6.3.15#6346) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org