continue of previous mail... The same method rethrows an exception which will lead to failure of an metrics exporter. The method should return some numeric value which indicates the failure.
On Wed, Oct 28, 2020 at 3:09 PM Andrey Gura <ag...@apache.org> wrote: > > 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? > > > > > > > > > > > > > > > >
