Andrey, thanks for your comment. I will fix this problem shortly.
ср, 28 окт. 2020 г. в 15:10, Andrey Gura <[email protected]>: > > Hi there, > > I accidentally stumbled upon a potential performance problem in this commit. > > CacheGroupMetricImpls.getPagesLeftForReencryption method contains at > least two problems: > > - Relatively major: In order to calculate a value for one metric the > method has O(N) complexity (N is number of partitions). It isn't good. > Better approach is using some precalculated or estimated value during > re-encryption process and just return this value. > - Major: For each partition in this method PageStore.exists() will be > called. This invocation leads to N calls to the file system (may be > cached, may be not, we can't just hope). So with a default affinity > configuration this method will touch the file system 1024 times per > one metrics value calculation. Just increase dramatism and multiply > 1024 on the number of cache groups existing on a node. > > Finally, we have auxiliary functionality (metrics) which could affect > the whole node (and potentially cluster) behavior. > > Please, fix this problem and be more careful in the future. > > On Fri, Oct 23, 2020 at 12:46 PM Pavel Pereslegin <[email protected]> wrote: > > > > Hello folks, > > > > thanks to everyone who joined the review, greatly appreciate your > > helpful comments. > > > > If there is no objection, we will merge this patch [1] shortly. > > > > [1] https://github.com/apache/ignite/pull/7941 > > > > пн, 5 окт. 2020 г. в 15:30, Maksim Stepachev <[email protected]>: > > > > > > Hi, > > > > > > I'm going to do it. > > > > > > сб, 3 окт. 2020 г. в 21:47, Alex Plehanov <[email protected]>: > > > > > > > Hello guys, > > > > > > > > I've finished the review and approved the patch. > > > > Anybody else would like to review it? > > > > > > > > пн, 28 сент. 2020 г. в 11:38, Pavel Pereslegin <[email protected]>: > > > > > > > > > Hello, Maksim! > > > > > > > > > > I am currently working on a review notes from Alexey Plekhanov, will > > > > > let you know when I finish. > > > > > > > > > > пн, 28 сент. 2020 г. в 11:04, Maksim Stepachev < > > > > [email protected] > > > > > >: > > > > > > > > > > > > Hi, Pavel. > > > > > > > > > > > > As I see, the ticket [ > > > > https://issues.apache.org/jira/browse/IGNITE-12843 > > > > > ] > > > > > > is "PATCH AVAILABLE". Is this ticket finished? > > > > > > > > > > > > чт, 13 авг. 2020 г. в 13:49, Pavel Pereslegin <[email protected]>: > > > > > > > > > > > > > Hello all. > > > > > > > > > > > > > > I'm working on TDE cache group key rotation [1] and I have a > > > > > > > couple > > > > of > > > > > > > questions about partition re-encryption. > > > > > > > > > > > > > > As described in the wiki [2], the process of re-encryption at the > > > > > > > moment consists of sequentially marking memory pages as dirty, > > > > > > > this > > > > > > > process looks not resource-intensive. > > > > > > > Do you think it is necessary to do this in a multithreaded mode or > > > > > > > single thread is enough? > > > > > > > (We started testing re-encryption on dedicated servers (Xeon > > > > > > > E5-2680 > > > > > > > 2.4Ghz, SSD Huawei ES3600P 3.2TB, ThrottlingPolicy = > > > > > > > CHECKPOINT_BUFFER_ONLY) with no speed limit and no load, as a > > > > > > > result, > > > > > > > single-threaded encryption loaded disk within 30%. At the same > > > > > > > time, > > > > > > > the total re-encryption speed was around 60 MB/s, which allows one > > > > > > > node to re-encrypt 1 TB of data in about 5 hours, and it seems > > > > > > > that > > > > > > > this performance is enough.) > > > > > > > > > > > > > > The second question is about the approach to storing the > > > > re-encryption > > > > > > > status. > > > > > > > At the moment, the re-encryption status includes two parameters - > > > > > > > the > > > > > > > total number of pages in the partition at the time of the start of > > > > > > > re-encryption (int) and the index of the last re-encrypted page > > > > (int). > > > > > > > These 8 bytes are stored in the metapage on the checkpoint (which > > > > > > > ensures that if the checkpoint does not happen, we will continue > > > > > > > the > > > > > > > process from the last page written to disk). > > > > > > > However, if multithread partition scanning does not make sense, > > > > > > > then > > > > > > > it seems that it is possible to change the implementation and > > > > > > > don't > > > > > > > change the metapage structure. Store only the "pointer" of the > > > > > > > partition (and the cache group) in the metastore and scan in > > > > > > > strict > > > > > > > order. > > > > > > > The approach with storing the status in the metapage of the > > > > > > > partition > > > > > > > seems to me more flexible, stable and has a number of advantages > > > > > > > over > > > > > > > the "pointer" approach: > > > > > > > 1. Since we saving the total number of pages at the re-encryption > > > > > > > startup - we will not scan extra pages that may be added to the > > > > > > > partition later. > > > > > > > 2. We can move partitions between nodes and re-encryption should > > > > > > > continue from a certain point on the new node. > > > > > > > 3. If a partition is (re)created during cache group > > > > > > > re-encryption, it > > > > > > > will not be re-encrypted (since its re-encryption status will be > > > > reset > > > > > > > and all data is encrypted with the latest encryption key after > > > > > > > (re)creation. > > > > > > > > > > > > > > Do you think single-threaded mode is enough? > > > > > > > Is it better to keep the re-encryption status in the metapage or > > > > store > > > > > > > the "pointer" in the metastore? > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12843 > > > > > > > [2] > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652384#TDE.Phase3.Cachekeyrotation.-Backgroundre-encryption > > > > > > > > > > > > > > пт, 31 июл. 2020 г. в 11:09, Pavel Pereslegin <[email protected]>: > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > I'll expand the answer a bit about calculating CRC, the problem > > > > > > > > is > > > > > not > > > > > > > > that it is calculated twice, but that now for encrypted pages, > > > > > > > > EncryptedFileIO checks physical integrity, and FilePageStore > > > > > > > > checks > > > > > > > > the correctness of the encryption key, but from my point of > > > > > > > > view, > > > > it > > > > > > > > should be vice versa - the lower (delegated) FileIO should check > > > > the > > > > > > > > physical integrity and EncryptedFileIO should check the > > > > > > > > correctness > > > > > of > > > > > > > > the encryption key. > > > > > > > > > > > > > > > > пт, 31 июл. 2020 г. в 10:40, Pavel Pereslegin > > > > > > > > <[email protected]>: > > > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > > 10. Question - CRC is read in two places encryptionFileIO > > > > > > > > > > and > > > > > > > > > > filePageStore - what should we do with this? > > > > > > > > > > > > > > > > > > We need to calculate the CRC of encrypted data, because we > > > > > > > > > may be > > > > > > > > > using the wrong encryption key to decrypt data, in which case > > > > > > > > > we > > > > > will > > > > > > > > > not understand if the physical integrity is violated or the > > > > > > > > > wrong > > > > > > > > > encryption key is used. > > > > > > > > > > > > > > > > > > > 9. Question - How do we optimize when we can check that this > > > > > page is > > > > > > > > > > already encrypted by parallel loading? Maybe we should do > > > > > > > > > > this > > > > in > > > > > > > Phase 4? > > > > > > > > > > > > > > > > > > To do this, we need to store the encryption key ID in memory > > > > > > > > > (at > > > > > > > > > least), but this is not easy to do right now without breaking > > > > > binary > > > > > > > > > compatibility. > > > > > > > > > > > > > > > > > > > 7. Question -the current implementation does not use the > > > > > throttling > > > > > > > that > > > > > > > > > > is implemented in PDS. Users should set the throughput such > > > > > > > > > > as > > > > 5 > > > > > MB > > > > > > > per > > > > > > > > > > second, but not the timeout, packet size, or stream size. > > > > > > > > > > > > > > > > > > I've added a simple rate limiter for this. > > > > > > > > > > > > > > > > > > > 8. Question - why we add a lot of system properties? > > > > > > > > > >> Can you, please, list system properties that should be > > > > > > > > > >> moved > > > > to > > > > > the > > > > > > > configuration? > > > > > > > > > > > > > > > > > > It's about the background re-encryption properties, for now, > > > > > > > > > it > > > > is: > > > > > > > > > - re-encryption speed limit (in megabytes per second) > > > > > > > > > - threads count used for re-encryption > > > > > > > > > - count of pages in batch, processed under checkpoint lock > > > > > > > > > - flag to completely disable background re-encryption > > > > > > > > > > > > > > > > > > > 11. We should remember about complicated test scenarios with > > > > > failover > > > > > > > > > > > > > > > > > > PR contains tests for re-encryption (and key rotation) on > > > > unstable > > > > > > > > > topology (with baseline change and without it). I'll expand > > > > > > > > > them > > > > > if I > > > > > > > > > missed some cases. > > > > > > > > > > > > > > > > > > > 13. Will re-encryption continue after the cluster is > > > > > > > > > > completely > > > > > > > stopped? > > > > > > > > > > > > > > > > > > Yes, as I mentioned earlier, we save the re-encryption status > > > > > > > > > in > > > > > the > > > > > > > > > meta page of each re-encrypted partition and trigger > > > > re-encryption > > > > > on > > > > > > > > > node startup if needed (more detailed description on the > > > > > > > > > wiki). > > > > > > > > > > > > > > > > > > Thanks a lot for your comments, I am still working on PR and > > > > > expanding > > > > > > > > > wiki documentation. I'll let you know when it will be ready > > > > > > > > > for > > > > the > > > > > > > > > review. > > > > > > > > > > > > > > > > > > вт, 28 июл. 2020 г. в 19:14, Alexey Goncharuk < > > > > > > > [email protected]>: > > > > > > > > > > > > > > > > > > > > Hello Nikolay, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 10. Question - CRC is read in two places > > > > > > > > > > > > encryptionFileIO > > > > and > > > > > > > > > > > filePageStore - what should we do with this? > > > > > > > > > > > > > > > > > > > > > > filePageStore checks CRC of the encrypted page. This > > > > > > > > > > > required > > > > > to > > > > > > > confirm > > > > > > > > > > > the page not corrupted on the disk. > > > > > > > > > > > encryptionFileIO checks CRC of the decrypted page(CRC > > > > > > > > > > > itself > > > > > > > stored in the > > > > > > > > > > > encrypted data). > > > > > > > > > > > This required to be sure the decrypted page contains > > > > > > > > > > > correct > > > > > data > > > > > > > and not > > > > > > > > > > > replaced with some malicious content. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I still do not see why we need CRC twice, can you please > > > > > elaborate > > > > > > > on this > > > > > > > > > > statement? If an attacker is able to replace the contents > > > > > > > > > > of an > > > > > > > encrypted > > > > > > > > > > page, it means that they have access to the encryption key. > > > > What > > > > > will > > > > > > > > > > prevent them from calculating the CRC of malicious content > > > > > > > > > > and > > > > > then > > > > > > > > > > encrypting it? > > > > > > > > > > > > > > > >
