HTTP/2 Framing layer implementation (#309). #1233

aleksostapenko · 2019-03-28T21:06:32Z

Working HTTP/2 functionality in PR

PING and SETTINGS frames exchange: receiving, parsing and response with ACK; tested via h2i utility (https://github.com/golang/net/tree/master/http2/h2i);
Establishing HTTP/2 connection; tested via nghttp utility - the part of nghttp2/nghttp2-client packages (https://nghttp2.org):
1. Client's connection preface (starting magic sequence and SETTINGS frame) receiving and parsing;
2. Response with server side preface (initial SETTINGS frame);
3. Send SETTINGS ACK frame (in response to client's preface SETTINGS frame);
4. Receiving and parsing client's SETTINGS ACK frame (in response to server side preface SETTINGS fram
5. Receiving and parsing client's PRIORITY frames;
6. Receiving and parsing client's HEADERS frame;
7. Applying client's HEADERS as initial frame for new stream creation.

Command:

nghttp -v https://<name_of_host_with_started_tempesta_fw>

Note: to test HTTP/2 connection establishing via nghttp (described above) - it is necessary to replace the implementation of APP_FRAME() macro with simple false value, since upper level for processing of application frames (HEADERS, DATA and CONTINUATION) is not implemented yet.

HTTP/2 Stream Scheduler

In context of #309 only base implementation of priority/dependency scheduler is planned (the rest must be implemented in #1196). So this PR contains only initial form of scheduler: at stage of #309 all streams are assumed to have equal weights and depends on the one root logical stream (with ID equal 0); thus, streams are processed in order of their frames/request/response come from TCP connection, and in this case we need just a simple streams' storage with fast search by ID. In current PR red-black tree based storage is used; this structure has been chosen - compared to hash table - due to lack of per-connection memory overhead, since for hash table some amount of memory (maybe large enough) must be allocated initially - at the connection's creation moment, and besides, this could be a ddos vector.

Considering future priority/dependency scheduler implementation in context of #1196 - an appropriate scheme for the scheduler, in my opinion, would be a tree-like structure (let's call it a dependency tree), built with the help of, "parent" and "child" pointers (embedded into Stream structure) for vertical links and "prev" and "next" pointers - for the horizontal links between neighbor streams of the same level. A logical stream with identifier 0 acts as a root of this tree. In addition to the specified links, each stream has a pointer to a special priority queue (based on the binary heap), in which only its child single-level streams with jobs to be processed (frames/requests/responses) are placed.

Thus, the red-black tree, mentioned above, in proposed scheme acts as a basic storage structure for a quick search of streams; on top of it - the dependency tree acts as a structure to track current dependencies between streams and to calculate effective priorities for each stream in the tree — based on their weights (in case of re-prioritization etc.); also dependency tree provides up/down traversals between dependency levels; and the priority queues, in turn, track the next stream to be processed for each level.

tempesta_fw/http_frame.c

i-rinat

Looks OK.

i-rinat · 2019-04-12T22:22:23Z

tempesta_fw/http_frame.c

+
+#define VERIFY_FRAME_SIZE(ctx)						\
+do {									\
+	if ((ctx)->hdr.length <= 0)					\


Scanned through the http/2 description, but found no evidence of zero-length frames are forbidden. In fact, 6.9.1 mentions them explicitly: "Frames with zero length with the END_STREAM flag set (that is, an empty DATA frame) MAY be sent if there is no available space in either flow-control window."
Some client may send such zero-length frames. I think we should allow that.

Added handling for zero length payload.

i-rinat · 2019-04-12T22:44:21Z

tempesta_fw/http_frame.c

+	}
+
+	T_FSM_STATE(HTTP2_RECV_FRAME_SETTINGS) {
+		BUG_ON(ctx->to_read > FRAME_HEADER_SIZE);


I propose to move this BUG_ON() into FRAME_FSM_READ_SRVC(), and change FRAME_HEADER_SIZE to sizeof(ctx->rbuf).

i-rinat · 2019-04-12T23:10:41Z

tempesta_fw/http_frame.c

+	 * certain service data should be separated from it (placed at the
+	 * frame's beginning): frame header, optional pad length and optional
+	 * priority data (the latter is for HEADERS frames only). Besides,
+	 * DATA and HEADERS frames can containing some padding in the frame's


Suggested change

* DATA and HEADERS frames can containing some padding in the frame's

* DATA and HEADERS frames can contain some padding in the frame's

i-rinat · 2019-04-12T23:12:29Z

tempesta_fw/http_frame.c

+	 * DATA and HEADERS frames can containing some padding in the frame's
+	 * tail, but we don't need to worry about that here since such padding
+	 * is processed as service data, separately from app frame, and it
+	 * will be just splitted into separate skb (above).


Suggested change

* will be just splitted into separate skb (above).

* will be just split into separate skb (above).

i-rinat · 2019-04-13T17:50:55Z

tempesta_fw/http_frame.c

+}
+
+/*
+ * Initialization of HTTP/2 framing context. Due to passing frames to


It's more like re-initialization, as there is a period where context is used, but this function wasn't called yet. As far as I can see, context is initialized in tfw_tls_check_alloc_h2_context().

i-rinat · 2019-04-13T18:09:51Z

tempesta_fw/http_frame.c

+
+#define FRAME_CLI_MAGIC		"PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"
+#define FRAME_CLI_MAGIC_LEN	24
+#define FRAME_SRVC1_SIZE	4


I'd propose to use separate constants for WINDOW_UPDATE and RST_STREAM. Hiding both behind SRVC1 doesn't save anything, but makes code harder to read. Same for SRVC2.

i-rinat · 2019-04-13T18:11:29Z

tempesta_fw/http_frame.c

+#define FRAME_CLI_MAGIC_LEN	24
+#define FRAME_SRVC1_SIZE	4
+#define FRAME_PRIORITY_SIZE	5
+#define FRAME_STNGS_ENTRY_SIZE	6


I can't help but read STNGS as "stings". Is it worth to save letters like that?

i-rinat · 2019-04-13T18:55:14Z

tempesta_fw/http_frame.c

+		break;
+
+	default:
+		/* Only DATA and HEADERS frames cab be padded. */


Suggested change

/* Only DATA and HEADERS frames cab be padded. */

/* Only DATA and HEADERS frames can be padded. */

i-rinat · 2019-04-13T19:19:20Z

tempesta_fw/tls.c

+	protos[order].id = TTLS_ALPN_ID_HTTP1;
+	protos[!order].name = TTLS_ALPN_HTTP2;
+	protos[!order].len = sizeof(TTLS_ALPN_HTTP2) - 1;
+	protos[!order].id = TTLS_ALPN_ID_HTTP2;


Looks like HTTP/2 is unconditionally enabled even if listen directive has https:http1 and doesn't mention http2 at all. Is this intended behavior?

Yes, currently http1 is just a shortcut for http1,http2 and http2 is a shortcut for http2,http1. Thus, we always enable http2 handling, and since our protocols order takes precedence, we can define what protocol will be used when client sends us in any order both http/1.1 and h2 in ALPN.
But in any case, this point (as well as the names for HTTP/1.1 and HTTP/2 protocols in configuration) remains the subject TBD.

I think it's not a good idea to enable HTTP/2 by default before we have the stable implementation - this may lead to bad user experience. I also vote for RFC compliant name like h2.

OK. Corrected.

i-rinat · 2019-04-13T19:33:25Z

tls/tls_srv.c

-	/* TODO #309 #834 confugure on Tempesta side: "h2, http/1.1" */
-	if (!tls->conf->alpn_list)
-		return 0;
+	/* If TLS processig is enabled, ALPN must be configured. */


Suggested change

/* If TLS processig is enabled, ALPN must be configured. */

/* If TLS processing is enabled, ALPN must be configured. */

vankoven · 2019-04-19T12:39:58Z

Makefile

@@ -35,8 +35,10 @@ DBG_HTTP_PARSER ?= 0
 DBG_SS ?= 0
 DBG_TLS ?= 0
 DBG_APM ?= 0
+DBG_HTTP_FRAME ?= 0


I bet there will be families of debug functions for HTTP2: for frames, HPACK, and what ever else. The whole group will have the same meaning to HTTP2 as DBG_HTTP_PARSER for HTTP1.1. Should we rename it as DBG_HTTP2_FRAME?

For now I cannot do such separation: these sets do not intersect yet; also both HTTP/1.1 and HTTP/2 protocols belong to the same HTTP domain, so there is no contradiction here. But in the nearest future (e.g. during HTTP parser implementation changing in context of HTTP/2 processing) mentioned intersections may appear, and in this case the separation for HTTP1_* and HTTP2_* may be really needed and will be made for preprocessor variables (and possibly for filenames too, e.g. http_frame.c/h, http_stream.c/h).

vankoven · 2019-04-19T13:01:04Z

tempesta_fw/connection.h

@@ -209,9 +210,11 @@ enum {
 typedef struct {
 	TfwCliConn	cli_conn;
 	TlsCtx		tls;
+	TfwHttp2Ctx	*h2;


The structure name becomes misleading, now it's more than TLS connection, it also stores the HTTP2 context. And a new issue is introduced: HTTP2 context is not allocated for non-TLS HTTP2 connections (which are allowed by standard).
In the same time HTTP2 connections will have two contexts in the same time: HTTP2 and HTTP1. HTTP1 context is stored as TfwConn->TfwHttpParser. Probably we should introduce a union in TfwConn class to store 'protocol context' whatever http2 or http1. Seems like we never want both. Even is the HTTP1 connection is upgraded to HTTP2 we can easily re-init the field.

HTTP2 context is not allocated for non-TLS HTTP2 connections (which are allowed by standard)

In result of discussion in #309 and in chat - it has been decided not to support non-TLS HTTP/2 in TempestaFW.

Probably we should introduce a union in TfwConn class to store 'protocol context' whatever http2 or http1.

Yes, agree with that. The similar approach is proposed in #309 (comment) (in Stream Priority/Dependencies). These changes is part of future PRs in context of #309 ("Request HTTP/2 -> HTTP/1.1" or more soon "Disable @seq_queue for HTTP/2"). During implementation of Framing, Scheduler and Stream States - I temporary left that part as is - for testing/debugging purposes.

vankoven · 2019-04-19T13:05:11Z

tempesta_fw/http.c

-static int
-__tfw_http_msg_process(void *conn, TfwFsmData *data)
+int
+tfw_http_msg_process_generic(void *conn, TfwFsmData *data)


Bad naming. It's not generic. It's HTTP1 processing.

In the nearest future PRs (in context of #309) this function is planned for usage for HTTP/2 processing too, so I have run a little forward here. In my opinion, most part of tfw_http_msg_process_generic() is general processing, whatever HTTP/1.1 or HTTP/2 protocol (except the parsing stage only), so it is worth to try adapt it for both protocols. In any case if this idea will fail, and the separate procedures will be needed - the name will be reverted accordingly.

vankoven · 2019-04-19T13:08:12Z

tempesta_fw/http.c

@@ -3930,7 +3931,9 @@ tfw_http_msg_process(void *conn, TfwFsmData *data)
 		if (likely(r == T_OK || r == T_POSTPONE)) {
 			data->skb->next = data->skb->prev = NULL;
 			data->trail = !next ? trail : 0;
-			r = __tfw_http_msg_process(conn, data);
+			r = TFW_CONN_TLS((TfwConn *)conn) && tfw_http2_context(conn)


Seems like TFW_CONN_TLS((TfwConn *)conn) is extra here. HTTP2 recommends using HTTP2 over TLS (h2) but it still allows HTTP2 over TCP (h2c). Personally I don't understand why the unencrypted connections are allowed by standard, but we have to follow them.

It has been decided not to support non-TLS HTTP/2 in TempestaFW: standard allows HTTP/2 over plain TCP, but does not force to use it.

vankoven · 2019-04-19T13:09:21Z

tempesta_fw/http_frame.c

+ *		Tempesta FW
+ *
+ * Copyright (C) 2014 NatSys Lab. ([email protected]).
+ * Copyright (C) 2015-2019 Tempesta Technologies, Inc.


Bad copy-paste. This file wasn't existed before, so only 2019 here.

vankoven · 2019-04-19T13:21:13Z

tempesta_fw/http_frame.c

+ * and GOAWAY frames to report the reasons of the stream or
+ * connection error.
+ */
+#define FRAME_ECODE_PROTO	0x1


Section 7 contains 14 error codes. I understand that only two are needed at this moment, but it seems that we will definitely will need all of them, let's introduce enum like TfwFrameType to store all the possible errors.

vankoven · 2019-04-19T13:22:27Z

tempesta_fw/http_frame.h

+ *		Tempesta FW
+ *
+ * Copyright (C) 2014 NatSys Lab. ([email protected]).
+ * Copyright (C) 2015-2019 Tempesta Technologies, Inc.


Same here. Bad copy-paste.

vankoven · 2019-04-19T13:23:49Z

tempesta_fw/http_frame.h

+ * this program; if not, write to the Free Software Foundation, Inc., 59
+ * Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+#ifndef __HTTP_FRAME__


__HTTP2_FRAME__?

The file has name http_frame.h, so this will be irrelevant (also see #1233 (comment)).

vankoven · 2019-04-19T13:26:13Z

tempesta_fw/http_frame.h

+	HTTP2_RECV_HEADER = __HTTP2_RECV_FRAME_APP,
+	HTTP2_RECV_CONT,
+	HTTP2_RECV_DATA
+} TfwFrameState;


Do we really need to expose this states to other modules? Why it's not in http2_frame.c?

Currently we need that due to necessity of correct state initialization in tfw_tls_check_alloc_h2_context() in tls.c.

vankoven · 2019-04-19T13:28:18Z

tempesta_fw/http_frame.h

+ * @flags	- frame's flags;
+ */
+typedef struct {
+	int		length;


Never going to be below 0, thus should be unsigned.

Never going to be below 0

Currently, code in tfw_http2_recv_priority() may temporarily make length negative. If, for example, client sends a wrong length in PRIORITY frame.

krizhanovsky

Good to merge, but please make the several cleanups/improvements before. It's also good to discuss (think once more) the parser design.

krizhanovsky · 2019-05-11T19:39:34Z

tempesta_fw/http_frame.c

+				return tfw_http2_stream_terminate(ctx,
+								  hdr->stream_id,
+								  NULL,
+								  HTTP2_ECODE_REFUSED);


The line is too wide. I'd propose to use tfw_h2_ prefix instead of tfw_http2_. There is too deep nesting, probably the case code should be moved to separate function.

krizhanovsky · 2019-05-11T19:52:01Z

tempesta_fw/tls.c

+		return 0;
+	}
+
+	h2 = kzalloc(sizeof(TfwHttp2Ctx), GFP_ATOMIC);


Please use SLAB for fixed-size allocations. Next, h2 is first class citizen for us while HTTP/1 is legacy. We also don't have any business with plain TLS, so it's likely that any TLS connection is essentially a h2 connection. I think TfwHttp2Ctx should be inherited from TfwTlsConn instead of using the pointer, so let's just allocated large piece of memory to fit TLS and h2 context.

BTW @i-rinat probably h2 context is a good place to store second checksum. In HTTP/3 all the layers must be aware about each other after all, so this can be an option (I don't like it though, but if we're out of memory on TLS handshakes, then this makes sense to use the extra memory and there is guarantee that we move to either h2 or h3 after TLS stuff, even for QUIC).

krizhanovsky · 2019-05-12T12:33:47Z

tempesta_fw/http.c

-			r = __tfw_http_msg_process(conn, data);
+			r = TFW_CONN_TLS((TfwConn *)conn) && tfw_http2_context(conn)
+				? tfw_http2_frame_process(conn, data)
+				: tfw_http_msg_process_generic(conn, data);


In #309 we discussed that we should split HTTP parser in at least 2 pieces: headers and body and both the parsers will be called separately from H2 layer. How do you see the split in current code? Check H2 state in tfw_http_req_process()?

Frankly, now we have single-jump cost in our parser to enter any state and I believe we're able to make GCC, probably with some assembly help, to generate proper FSM code layout to make the large parser code faster (after all, even after the parser split, we'll have complex FSM due to many checks), so probably we'll be fine with current code if it simpler than possible split parser.

Check H2 state in tfw_http_req_process()?

Yeah - this must be a frame type and, maybe, a stream state (since we have to track request's start/end, and this exactly matches with stream's states, as mentioned in penultimate paragraph in https://tools.ietf.org/html/rfc7540#section-8.1).

Сurrently I'm not very optimistic about parser division into separate parts: indeed, we have rather effective implementation with single-jump cost to enter any state; and with divided parser we will get costs of additional function calls (if not inlined) and necessity for additional branching to call separate parser parts (with associated costs). But in general, I am still thinking about this point.

krizhanovsky · 2019-05-12T12:59:33Z

tempesta_fw/tls.c

+
+#define CONF_HTTPS	"https"
+#define CONF_HTTP1	"http1"
+#define CONF_HTTP2	"http2"


Please update etc/tempesta_fw.conf and appropriate Wiki with the HTTP/2 support. Please say (in bold) in the WIki that HTTP/2 implementation is incomplete.

krizhanovsky · 2019-05-12T13:04:47Z

tempesta_fw/tls.c

+	protos[order].id = TTLS_ALPN_ID_HTTP1;
+	protos[!order].name = TTLS_ALPN_HTTP2;
+	protos[!order].len = sizeof(TTLS_ALPN_HTTP2) - 1;
+	protos[!order].id = TTLS_ALPN_ID_HTTP2;


I think it's not a good idea to enable HTTP/2 by default before we have the stable implementation - this may lead to bad user experience. I also vote for RFC compliant name like h2.

krizhanovsky · 2019-05-12T14:27:19Z

tempesta_fw/http_stream.c

+		}
+	}
+
+	new_stream = kzalloc(sizeof(TfwStream), GFP_ATOMIC);


The same allocation problem: basically we usually don't use pattern kzalloc(sizeof(....), ...) in any more or less performance crucial code - if we have fixed size allocations, then we use some kind of SLAB allocator.

Yeah - missed that. Thanks!

krizhanovsky · 2019-05-12T14:40:46Z

tempesta_fw/http_stream.c

+	tfw_http2_init_stream(new_stream, id, weight);
+
+	rb_link_node(&new_stream->node, parent, new);
+	rb_insert_color(&new_stream->node, &sched->streams);


We'll need priority index plus to current index for stream ID (as you said in the PR description), so we need two indexes. However, binary heap is technically an array (continuous memory location which usually gives us good spactial locality in run time - this is why I wanted binary heap in #309), and, since you already use dynamic memory allocator, all your TfwStream instances reside somewhere in memory and it's not wise to use additional array just for heap index. It's better to introduce additional node entries in TfwStream to build the second index.

If you fix the kzalloc() above, then SLAB/SLOB/SLUB already cares about spacial locality. The question is: do we expect to see more frequently frames from the same TCP connection streams or from streams as they first appeared (to fit the internal SLAB allocation pattern)? I think the first one, but I won't bet that there is so strong spacial locality in access pattern to streams from the same heap/rb-tree. Moreover, if we use binary heap, as I proposed, on top of an array, we'll have to manager erased items, i.e. move stored items and this is basically slower than SLAB's bitmasks.

All it all, I think RB tree is a good enough decision here since there is no confidence in access pattern. I write the comment for #1196 and will leave link there to the comment.

OK. I'll think yet further about building a dependency/priority tree.

krizhanovsky · 2019-05-12T14:40:57Z

tempesta_fw/http_stream.c

+	tfw_http2_init_stream(new_stream, id, weight);
+
+	rb_link_node(&new_stream->node, parent, new);
+	rb_insert_color(&new_stream->node, &sched->streams);


We'll need priority index plus to current index for stream ID (as you said in the PR description), so we need two indexes. However, binary heap is technically an array (continuous memory location which usually gives us good spactial locality in run time - this is why I wanted binary heap in #309), and, since you already use dynamic memory allocator, all your TfwStream instances reside somewhere in memory and it's not wise to use additional array just for heap index. It's better to introduce additional node entries in TfwStream to build the second index.

If you fix the kzalloc() above, then SLAB/SLOB/SLUB already cares about spacial locality. The question is: do we expect to see more frequently frames from the same TCP connection streams or from streams as they first appeared (to fit the internal SLAB allocation pattern)? I think the first one, but I won't bet that there is so strong spacial locality in access pattern to streams from the same heap/rb-tree. Moreover, if we use binary heap, as I proposed, on top of an array, we'll have to manager erased items, i.e. move stored items and this is basically slower than SLAB's bitmasks.

All it all, I think RB tree is a good enough decision here since there is no confidence in access pattern. I write the comment for #1196 and will leave link there to the comment.

1. "http2 -> h2" renaming for functions and structures; 2. SLAB usage for allocations.

1. Make 'TfwH2Conn' (with HTTP/2 context) inherited from 'TfwTlsConn'. 2. Avoid default enabling of HTTP/2 protocol.

aleksostapenko added 2 commits March 29, 2019 03:52

HTTP/2 Framing layer implementation (#309).

3d77573

Minor layout correction (#309).

10be35a

aleksostapenko added the ready for review label Mar 28, 2019

aleksostapenko added 2 commits March 29, 2019 16:17

Correction of HTTP/2 switching (#309).

7139f53

Minor correction in unit tests (#309).

26cc780

krizhanovsky requested review from krizhanovsky and i-rinat April 8, 2019 14:10

krizhanovsky assigned i-rinat and aleksostapenko Apr 8, 2019

i-rinat reviewed Apr 9, 2019

View reviewed changes

tempesta_fw/http_frame.c Show resolved Hide resolved

tempesta_fw/http_frame.c Outdated Show resolved Hide resolved

tempesta_fw/http_frame.c Outdated Show resolved Hide resolved

tempesta_fw/http_frame.c Outdated Show resolved Hide resolved

aleksostapenko added 2 commits April 10, 2019 23:51

HTTP/2 Starting/Switching implementation (#309).

fe57412

Remove forgotten debug code (#309).

ed79830

i-rinat approved these changes Apr 13, 2019

View reviewed changes

krizhanovsky requested a review from vankoven April 18, 2019 14:45

krizhanovsky assigned vankoven Apr 18, 2019

vankoven reviewed Apr 19, 2019

View reviewed changes

aleksostapenko added 6 commits April 25, 2019 19:05

Implementation of base functionality for HTTP/2 Stream Scheduler (#309).

f6e8edc

Corrections according review comments (#309).

79fcd17

Add processing for zero length of application frames payload (#309).

05bd194

Corrections according review comments (#309).

23d53db

Implementation of HTTP/2 Stream FSM (#309).

5b93ee8

Split header and payload processing for service frames (#309).

1e8be28

krizhanovsky approved these changes May 12, 2019

View reviewed changes

krizhanovsky mentioned this pull request May 12, 2019

HTTP/2 streams prioritization #1196

Closed

4 tasks

aleksostapenko added 5 commits May 13, 2019 20:34

Add support for maximum window of Flow Control (#309).

1d962dd

Corrections according review comments (#309).

b22c1f7

1. "http2 -> h2" renaming for functions and structures; 2. SLAB usage for allocations.

Corrections according review comments (#309).

5515e82

1. Make 'TfwH2Conn' (with HTTP/2 context) inherited from 'TfwTlsConn'. 2. Avoid default enabling of HTTP/2 protocol.

Add description in Tempesta FW configuration for HTTP/2 (#309).

ad183a6

Make HTTP/1.1 protocol default (#309).

dc1f523

aleksostapenko merged commit aee18fe into master May 16, 2019

aleksostapenko deleted the ao-309-framing branch May 16, 2019 20:23

	* DATA and HEADERS frames can containing some padding in the frame's
	* DATA and HEADERS frames can contain some padding in the frame's

	* will be just splitted into separate skb (above).
	* will be just split into separate skb (above).

	/* Only DATA and HEADERS frames cab be padded. */
	/* Only DATA and HEADERS frames can be padded. */

	/* If TLS processig is enabled, ALPN must be configured. */
	/* If TLS processing is enabled, ALPN must be configured. */

HTTP/2 Framing layer implementation (#309). #1233

HTTP/2 Framing layer implementation (#309). #1233

Conversation

aleksostapenko commented Mar 28, 2019 • edited Loading

Working HTTP/2 functionality in PR

HTTP/2 Stream Scheduler

i-rinat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Apr 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko commented Mar 28, 2019 •

edited

Loading

aleksostapenko Apr 29, 2019 •

edited

Loading

aleksostapenko Apr 29, 2019 •

edited

Loading

aleksostapenko Apr 28, 2019 •

edited

Loading

aleksostapenko May 16, 2019 •

edited

Loading