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 <xxt...@gmail.com> 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 <maksim.stepac...@gmail.com>: > > > > Hi, > > > > I'm going to do it. > > > > сб, 3 окт. 2020 г. в 21:47, Alex Plehanov <plehanov.a...@gmail.com>: > > > > > Hello guys, > > > > > > I've finished the review and approved the patch. > > > Anybody else would like to review it? > > > > > > пн, 28 сент. 2020 г. в 11:38, Pavel Pereslegin <xxt...@gmail.com>: > > > > > > > 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 < > > > maksim.stepac...@gmail.com > > > > >: > > > > > > > > > > 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 <xxt...@gmail.com>: > > > > > > > > > > > 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 <xxt...@gmail.com>: > > > > > > > > > > > > > > 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 <xxt...@gmail.com>: > > > > > > > > > > > > > > > > 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 < > > > > > > alexey.goncha...@gmail.com>: > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > >
