[
https://issues.apache.org/jira/browse/HDFS-10899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16136455#comment-16136455
]
Xiao Chen edited comment on HDFS-10899 at 8/22/17 8:04 AM:
-----------------------------------------------------------
Thanks a lot for the reviews [~jojochuang], good comments!
Replying one by one, and attaching a patch in the end. Comments that's not
mentioned are all addressed.
{quote}
reencryptionHandler#reencryptEncryptionZone()
zoneId is obtained when holding FSDirectory read lock, release the lock, and
then acquire FSDirectory read lock again.
This assertion is only correct if there will be only one ReencryptionHandler
running.
{quote}
There is only one {{ReencryptionHandler}}. Added texts to javadoc.
If the zone referred to by inodeid is changed (e.g. deleted/renamed) while the
lock is not held, {{checkZoneReady}} will throw. A similar test case would be
{{TestReencryption#testZoneDeleteDuringReencrypt}}.
bq. ReencryptionStatus#updateZoneStatus() should check that zoneNode is an
encryption zone.
For the 2 callers, {{FSD#addEZ}} is where the zoneId is added, so always true.
{{FSDirXAttrOp#unprotectedSetXAttrs}} is happening within the EZXattr, so also
always true. (There's no 'disable encryption' command, so zone node can only be
deleted/renamed)
bq. Why is currentBatch a TreeMap?
Good question. Initially this was done to keep the element's ordering and using
path as the key. Now that it's changed to inode id based, we can just use a
list. (Sorry didn't rebase the inodeid patch here on 14...)
bq. Does ZoneReencryptionStatus#getLastProcessedFile return the relative path?
or file name only? or absolute path?
Absolute path - so we can restore in case of fail over.
bq. It Allocates a 2000-element map, copy it over, and then clear the map. That
looks suboptimal. Would it be feasible to wrap TreeMap and make a method that
simply assigns the TreeMap reference to another currentBatch?
Agreed, problem is {{currentBatch}} here is passed in from the very outside of
the call stack.
Made it a member variable of {{ReencryptionHandler}} to address this. It's
still safe with the single-threaded handler model, but perhaps harder to read.
Please share your thoughts.
bq. EDEKReencryptCallable ... retry ... if reencryptEdeks() returns numFailures
> 0, call() should not return a new ReencryptionTask object.
Initially talking with [~andrew.wang], we wanted to always retry things, so
admin can just fix the error, and continue (or cancel).
But since KMSCP already has the retry logic added by HADOOP-14521, and to trade
off for maintainability, we do not 'double retry' here, and only let KMSCP's
retry policy to handle failures.
When -listReencryptionStatus, if numOfFailures > 0, a message is printed to ask
admin to examine failure and re-submit.
Implementation-wise, we still depend on the ReencryptionTask object to pass the
failures to the updater, so need that object. Updater handles failed tasks
differently.
was (Author: xiaochen):
Thanks a lot for the reviews [~jojochuang], good comments!
Replying one by one, and attaching a patch in the end. Comments that's not
mentioned are all addressed.
{quote}
reencryptionHandler#reencryptEncryptionZone()
zoneId is obtained when holding FSDirectory read lock, release the lock, and
then acquire FSDirectory read lock again.
This assertion is only correct if there will be only one ReencryptionHandler
running.
{quote}
There is only one {{ReencryptionHandler}}. Added texts to javadoc.
If the zone referred to by inodeid is changed (e.g. deleted/renamed) while the
lock is not held, {{checkZoneReady}} will throw. A similar test case would be
{{TestReencryption#testZoneDeleteDuringReencrypt}}.
bq. ReencryptionStatus#updateZoneStatus() should check that zoneNode is an
encryption zone.
For the 2 callers, {{FSD#addEZ}} is where the zoneId is added, so always true.
{{FSDirXAttrOp#unprotectedSetXAttrs}} is happening within the EZXattr, so also
always true. (There's no 'disable encryption' command, so zone node can only be
deleted/renamed)
bq. Why is currentBatch a TreeMap?
Good question. Initially this was done to keep the element's ordering and using
path as the key. Now that it's changed to inode id based, we can just use a
list. (Sorry didn't rebase the inodeid patch here on 14...)
bq. Does ZoneReencryptionStatus#getLastProcessedFile return the relative path?
or file name only? or absolute path?
Absolute path - so we can restore in case of fail over.
bq. It Allocates a 2000-element map, copy it over, and then clear the map. That
looks suboptimal. Would it be feasible to wrap TreeMap and make a method that
simply assigns the TreeMap reference to another currentBatch?
Agreed, problem is {{currentBatch}} here is passed in from the very outside of
the call stack.
Made it a member variable of {{ReencryptionHandler}} to address this. It's
still safe with the single-threaded handler model, but perhaps harder to read.
Please share your thoughts.
bq. EDEKReencryptCallable ... retry ... if reencryptEdeks() returns numFailures
> 0, call() should not return a new ReencryptionTask object.
Initially talking with [~andrew.wang], we wanted to always retry things, so
admin can just fix the error, and continue (or cancel).
But since KMSCP already has the retry logic added by HADOOP-14521, and to trade
off for maintainability, we do not 'double retry' here, and only let KMSCP's
retry policy to handle failures.
When -listReencryptionStatus, if numOfFailures > 0, a message is printed to ask
admin to examine failure and re-submit.
> 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.08.patch,
> HDFS-10899.09.patch, HDFS-10899.10.patch, HDFS-10899.10.wip.patch,
> HDFS-10899.11.patch, HDFS-10899.12.patch, HDFS-10899.13.patch,
> HDFS-10899.14.patch, HDFS-10899.15.patch, HDFS-10899.wip.2.patch,
> HDFS-10899.wip.patch, Re-encrypt edek design doc.pdf, Re-encrypt edek design
> doc V2.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.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
