Set pod labels to be optional fields in the network flow records #2739

yanjunz97 · 2021-09-09T20:37:30Z

The network flow record contains two fields related to pod labels, sourcePodLabel and destinationPodLabel. Sometimes long pod labels may cause problems. This PR sets the pod labels to be optional fields by adding a parameter includePodLabels in the flow aggregator configmap. By default, the pod labels will not be included in the flow records. If the user would like to include them, they can modify the flow aggregator configmap.

Signed-off-by: Yanjun Zhou zhouya@vmware.com

codecov-commenter · 2021-09-09T21:03:08Z

Codecov Report

Merging #2739 (b9d95d8) into main (0946ca6) will decrease coverage by 19.42%.
The diff coverage is 72.72%.

❗ Current head b9d95d8 differs from pull request most recent head 7f1ec92. Consider uploading reports for the commit 7f1ec92 to get more accurate results

@@             Coverage Diff             @@
##             main    #2739       +/-   ##
===========================================
- Coverage   60.31%   40.88%   -19.43%     
===========================================
  Files         284      158      -126     
  Lines       23470    19542     -3928     
===========================================
- Hits        14155     7990     -6165     
- Misses       7787    10802     +3015     
+ Partials     1528      750      -778

Flag	Coverage Δ
kind-e2e-tests	`?`
unit-tests	`40.88% <72.72%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/flowaggregator/flowaggregator.go	`21.20% <72.72%> (-48.09%)`	⬇️
pkg/ovs/openflow/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/util/runtime/runtime.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/logs.go	`9.52% <0.00%> (-90.48%)`	⬇️
pkg/apis/controlplane/register.go	`0.00% <0.00%> (-90.00%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`0.00% <0.00%> (-83.88%)`	⬇️
pkg/agent/agent_linux.go	`0.00% <0.00%> (-80.00%)`	⬇️
pkg/agent/client.go	`0.00% <0.00%> (-77.42%)`	⬇️
pkg/ovs/ovsconfig/ovs_client_linux.go	`0.00% <0.00%> (-76.93%)`	⬇️
... and 231 more

dreamtalen

Thanks Yanjun working on this.

dreamtalen · 2021-09-10T17:11:14Z

pkg/flowaggregator/flowaggregator.go

+			fa.fillPodLabels(key, record.Record)
+			fa.aggregationProcess.SetExternalFieldsFilled(record)
+		}
+	} else {
 		fa.aggregationProcess.SetExternalFieldsFilled(record)


This line should be inside the if statement.

Thanks for the review! It seems that in the if statement, we have really ensured the external fields to be filled. I added line 444 for the case that pod labels are not included. I tried to mark the external fields as filled to keep consistence with the case pod labels are provided. Should I just remove the line? Correct me if my understanding is wrong!

Got it, missed you have this line inside the if statement, you could remove this else statement.

dreamtalen · 2021-09-10T17:14:59Z

test/e2e/framework.go

@@ -679,6 +680,7 @@ func (data *TestData) mutateFlowAggregatorConfigMap(ipfixCollector string, faClu
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#externalFlowCollectorAddr: \"\"", fmt.Sprintf("externalFlowCollectorAddr: \"%s\"", ipfixCollector), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#activeFlowRecordTimeout: 60s", fmt.Sprintf("activeFlowRecordTimeout: %v", aggregatorActiveFlowRecordTimeout), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#inactiveFlowRecordTimeout: 90s", fmt.Sprintf("inactiveFlowRecordTimeout: %v", aggregatorInactiveFlowRecordTimeout), 1)
+	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#includePodLabels: false", fmt.Sprintf("includePodLabels: %v", includePodLabels), 1)


For e2e test, currently we are changing the config to include pod labels during e2e test and didn't add new test for not including pod labels situation to avoid making test longer, we would like to hear more options from @srikartati, do you think we need to add the test for the not including pod labels situation?

That is fine but I see that the "false" case was skipped for unit tests. Maybe it can be added there to get coverage for it.
@zyiou plans to add flow aggregator restart test in e2e tests. This scenario of not including Pod labels could be covered then. Please take note of that.

srikartati · 2021-09-10T17:47:18Z

cmd/flow-aggregator/options.go

@@ -57,6 +57,8 @@ type Options struct {
 	flowAggregatorAddress string
 	// Format for record sent to the configured flow collector
 	format string
+	// Whether sourcePodLabels and destinationPodLabels will be included


"includePodLabels indicates whether source and destination Pod labels are included or not"

srikartati · 2021-09-10T17:52:34Z

test/e2e/framework.go

@@ -679,6 +680,7 @@ func (data *TestData) mutateFlowAggregatorConfigMap(ipfixCollector string, faClu
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#externalFlowCollectorAddr: \"\"", fmt.Sprintf("externalFlowCollectorAddr: \"%s\"", ipfixCollector), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#activeFlowRecordTimeout: 60s", fmt.Sprintf("activeFlowRecordTimeout: %v", aggregatorActiveFlowRecordTimeout), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#inactiveFlowRecordTimeout: 90s", fmt.Sprintf("inactiveFlowRecordTimeout: %v", aggregatorInactiveFlowRecordTimeout), 1)
+	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#includePodLabels: false", fmt.Sprintf("includePodLabels: %v", includePodLabels), 1)


That is fine but I see that the "false" case was skipped for unit tests. Maybe it can be added there to get coverage for it.
@zyiou plans to add flow aggregator restart test in e2e tests. This scenario of not including Pod labels could be covered then. Please take note of that.

dreamtalen · 2021-09-10T18:12:26Z

Thanks @srikartati, Yanjun please make corresponding changes. Also, please modify the flow visibility doc since we have this new config.

yanjunz97 · 2021-09-10T19:13:26Z

Thanks @dreamtalen and @srikartati . Starting to work on them.

yanjunz97 · 2021-09-13T16:39:07Z

/test-e2e

dreamtalen · 2021-09-13T21:09:28Z

pkg/flowaggregator/flowaggregator_test.go

 		k8sClient:                   nil,
 		observationDomainID:         testObservationDomainID,
 	}

-	for _, isIPv6 := range []bool{false, true} {
+	faWithoutPodLabels := &flowAggregator{


Same above.

dreamtalen · 2021-09-13T21:10:07Z

pkg/flowaggregator/flowaggregator_test.go

 		observationDomainID:         testObservationDomainID,
 		podInformer:                 informerFactory.Core().V1().Pods(),
 	}
+
+	faWithoutPodLabels := &flowAggregator{


Could we have a for loop here with true or false includePodLabels when creating flowAggregator object instead of copying these lines?

Sounds good. Done.

yanjunz97 · 2021-09-13T22:09:10Z

/test-all

srikartati

LGTM.
In general, I feel changing this option runtime is more useful to the user rather than enforcing a restart of the Flow Aggregator. We should support this through antctl in the future.

Any thoughts on how this can be extended to other K8s metadata fields if required?

srikartati · 2021-09-13T23:24:34Z

/test-ipv6-e2e /test-ipv6-only-e2e

yanjunz97 · 2021-09-14T18:03:31Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

yanjunz97 · 2021-09-14T18:59:57Z

LGTM.
In general, I feel changing this option runtime is more useful to the user rather than enforcing a restart of the Flow Aggregator. We should support this through antctl in the future.

Any thoughts on how this can be extended to other K8s metadata fields if required?

I have not dug deep into the implementation of antctl. But it seems for the metadata fields in flow-aggregator configmap, all of them are parsed and saved in a variable of type flowAggregator, which is modifiable in runtime.

yanjunz97 · 2021-09-14T20:10:20Z

/test-windows-networkpolicy
/test-networkpolicy
/test-all-features-conformance

yanjunz97 · 2021-09-14T22:28:04Z

/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy

srikartati

LGTM

antoninbas

I have a concern that this will make integration with other tools harder, as these tools (e.g. NetworkPolicy recommendation) may depend on these labels to function properly. @srikartati what's the plan in this case? It seems that we do not need a better solution for Pods with "long labels".

antoninbas · 2021-09-21T19:57:49Z

build/yamls/flow-aggregator/base/conf/flow-aggregator.conf

@@ -34,3 +34,6 @@
 # Provide format for records sent to the configured flow collector.
 # Supported formats are IPFIX and JSON.
 #recordFormat: "IPFIX"
+
+# Provide whether source and destination Pod labels will be included in the flow record.
+#includePodLabels: false


I'd rather define a more generic configuration parameter that can be extended later:

recordContents: podLabels: false

or recordInformation if it is preferred to recordContents.

This is the direction we are taking for other Antrea components as well. @srikartati what do you think?

Yes. It is a good idea to make this extensible to multiple fields. However, this would only help with a small subset of fields that are added at the Flow Aggregator. We cannot truly disable the fields, especially the rest of Kubernetes metadata, which are added at the Flow Exporter--for this, we may need config map parameter in Antrea agent.
The rest of the fields that are added at the Aggregator are source and destination statistics, which I feel is not so significant to have this toggle option.

Maybe we could continue to have this option at the Aggregator and add this map-based data type in the Antrea Agent config map to disable at the Flow Exporter in a different PR.

I would say symmetry between the 2 configs is a good thing to have. There is also no telling whether we will include more information in the FlowAggregator in the future.

It is a valid point that we may add new fields like pod labels in the future to the Flow Aggregator. Changing to map-based generic config parameter sounds good.

@yanjunz97 could you address this ^

Sure. Start to work on it

srikartati · 2021-09-21T23:57:56Z

I have a concern that this will make integration with other tools harder, as these tools (e.g. NetworkPolicy recommendation) may depend on these labels to function properly. @srikartati what's the plan in this case? It seems that we do not need a better solution for Pods with "long labels".

Yes, Network Policy Recommendation engine would need this. However, we have two options to deploy the reco engine--either inside or outside the cluster. If it is inside, netpol reco engine could fetch the labels from API server directly rather than getting from the flow record. Do you agree @dreamtalen?

And the other visibility application that would need labels is Monitoring UI, which can be customizable by the user.

dreamtalen · 2021-09-22T00:05:16Z

I have a concern that this will make integration with other tools harder, as these tools (e.g. NetworkPolicy recommendation) may depend on these labels to function properly. @srikartati what's the plan in this case? It seems that we do not need a better solution for Pods with "long labels".

Yes, Network Policy Recommendation engine would need this. However, we have two options to deploy the reco engine--either inside or outside the cluster. If it is inside, netpol reco engine could fetch the labels from API server directly rather than getting from the flow record. Do you agree @dreamtalen?

And the other visibility application that would need labels is Monitoring UI, which can be customizable by the user.

Yes, I agree. I have another option in my head on long term architecture design is that we could fill these pod labels info when doing the flow processing before store flow records in the database, so that np recommendation engine, UI, and performance analysis module could have these info when fetching flow records from db.

antoninbas · 2021-09-22T22:45:11Z

Yes, Network Policy Recommendation engine would need this. However, we have two options to deploy the reco engine--either inside or outside the cluster. If it is inside, netpol reco engine could fetch the labels from API server directly rather than getting from the flow record.

But since we do want to support out-of-cluster, what is the plan then? How will the label information be exported from the cluster?

srikartati · 2021-09-22T23:29:25Z

Yes, Network Policy Recommendation engine would need this. However, we have two options to deploy the reco engine--either inside or outside the cluster. If it is inside, netpol reco engine could fetch the labels from API server directly rather than getting from the flow record.

But since we do want to support out-of-cluster, what is the plan then? How will the label information be exported from the cluster?

Label information will be present along with fields in the flow record. It will either be added by the Flow Aggregator or a visibility application. The design for exporting the flow record out of the cluster is still WIP. Currently, we are focusing on supporting single cluster.

antoninbas · 2021-09-23T20:19:51Z

build/yamls/flow-aggregator.yml

@@ -169,12 +169,15 @@ data:
    # Provide format for records sent to the configured flow collector.
    # Supported formats are IPFIX and JSON.
    #recordFormat: "IPFIX"
+
+    # Provide whether source and destination Pod labels will be included in the flow record.


Suggested change

# Provide whether source and destination Pod labels will be included in the flow record.

# Determine whether source and destination Pod labels will be included in the flow records.

antoninbas · 2021-09-23T20:20:16Z

build/yamls/flow-aggregator/base/conf/flow-aggregator.conf

@@ -34,3 +34,6 @@
 # Provide format for records sent to the configured flow collector.
 # Supported formats are IPFIX and JSON.
 #recordFormat: "IPFIX"
+
+# Provide whether source and destination Pod labels will be included in the flow record.
+#includePodLabels: false


@yanjunz97 could you address this ^

antoninbas · 2021-09-23T20:20:56Z

pkg/flowaggregator/flowaggregator.go

+		for _, ie := range antreaLabelsElementList {
+			element, err := fa.registry.GetInfoElement(ie, ipfixregistry.AntreaEnterpriseID)
+			if err != nil {
+				return 0, fmt.Errorf("%s not present. returned error: %v", ie, err)


Suggested change

return 0, fmt.Errorf("%s not present. returned error: %v", ie, err)

return 0, fmt.Errorf("error when getting InformationElement %s from registry: %v", ie, err)

antoninbas · 2021-09-23T20:22:10Z

pkg/flowaggregator/flowaggregator_test.go

+		{
+			false,
+			true,
+		},
+		{
+			true,
+			true,
+		},
+		{
+			false,
+			false,
+		},
+		{
+			true,
+			false,
+		},


nit: maybe use a less verbose syntax here

{ false, true }, { true, true }, ...

antoninbas · 2021-09-23T20:23:31Z

test/e2e/framework.go

@@ -679,6 +680,7 @@ func (data *TestData) mutateFlowAggregatorConfigMap(ipfixCollector string, faClu
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#externalFlowCollectorAddr: \"\"", fmt.Sprintf("externalFlowCollectorAddr: \"%s\"", ipfixCollector), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#activeFlowRecordTimeout: 60s", fmt.Sprintf("activeFlowRecordTimeout: %v", aggregatorActiveFlowRecordTimeout), 1)
 	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#inactiveFlowRecordTimeout: 90s", fmt.Sprintf("inactiveFlowRecordTimeout: %v", aggregatorInactiveFlowRecordTimeout), 1)
+	flowAggregatorConf = strings.Replace(flowAggregatorConf, "#includePodLabels: false", fmt.Sprintf("includePodLabels: %v", includePodLabels), 1)


I am not sure of the usefulness of defining an includePodLabels constant

Just to keep the consistence with other variables. I'm ok to use true here if you think it is better.

antoninbas · 2021-09-23T20:24:35Z

Sorry, I closed this by mistake...

yanjunz97 · 2021-09-24T17:05:27Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

srikartati · 2021-09-24T17:55:35Z

cmd/flow-aggregator/config.go

@@ -51,4 +51,12 @@ type FlowAggregatorConfig struct {
 	// Provide format for records sent to the configured flow collector. Supported formats are IPFIX and JSON.
 	// Defaults to "IPFIX"
 	RecordFormat string `yaml:"recordFormat,omitempty"`
+	// RecordContents contains flow records related configuration options.
+	RecordContents RecordContentsConfig `yaml:"recordContents,omitempty"`


Can we use simple type definition like featureGates in Antrea Agent? We can avoid a separate struct.
https://github.com/antrea-io/antrea/blob/main/cmd/antrea-agent/config.go#L23

Thanks Srikar. Sounds good as we may only need boolean value in the RecordContents. Updated.

yanjunz97 · 2021-09-24T23:06:03Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

srikartati · 2021-09-29T00:21:48Z

build/yamls/flow-aggregator/base/conf/flow-aggregator.conf

@@ -34,3 +34,8 @@
 # Provide format for records sent to the configured flow collector.
 # Supported formats are IPFIX and JSON.
 #recordFormat: "IPFIX"
+
+# recordContents contains flow records related configuration options.


"recordContents enables configuring some fields in the flow records, A field can be either included (true) or excluded (false) by providing a bool value."

srikartati · 2021-09-29T00:22:02Z

cmd/flow-aggregator/config.go

@@ -51,4 +51,6 @@ type FlowAggregatorConfig struct {
 	// Provide format for records sent to the configured flow collector. Supported formats are IPFIX and JSON.
 	// Defaults to "IPFIX"
 	RecordFormat string `yaml:"recordFormat,omitempty"`
+	// RecordContents contains flow records related configuration options.


Same as above

srikartati

LGTM

antoninbas

LGTM, thanks for making the changes

antoninbas · 2021-09-29T16:32:12Z

/test-all

yanjunz97 · 2021-09-29T19:25:46Z

/test-conformance

antoninbas · 2021-10-01T21:12:03Z

@yanjunz97 can you resolve the conflicts?

yanjunz97 · 2021-10-01T21:55:45Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

antoninbas · 2021-10-02T00:05:56Z

pkg/flowaggregator/flowaggregator_test.go

-		flowAggregatorAddress:       "",
-		observationDomainID:         testObservationDomainID,
-		podInformer:                 informerFactory.Core().V1().Pods(),
+	fas := make(map[bool]*flowAggregator)


why are we using a map for this?

newFlowAggregator := func(bool includePodLabels) * flowAggregator { return &flowAggregator{...} } flowAggregatorWithPodLabels := newFlowAggregator(true) flowAggregatorWithoutPodLabels := newFlowAggregator(false)

Good idea! Updated

antoninbas · 2021-10-02T00:06:16Z

pkg/flowaggregator/flowaggregator.go

+	if fa.includePodLabels {
+		if !fa.aggregationProcess.AreExternalFieldsFilled(*record) {


maybe unify the ifs?

Signed-off-by: Yanjun Zhou <zhouya@vmware.com>

antoninbas

LGTM

yanjunz97 · 2021-10-02T01:17:43Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

yanjunz97 · 2021-10-18T16:49:00Z

/test-all-features-conformance
/test-hw-offload

yanjunz97 marked this pull request as draft September 9, 2021 20:47

yanjunz97 marked this pull request as ready for review September 9, 2021 21:19

dreamtalen reviewed Sep 10, 2021

View reviewed changes

srikartati reviewed Sep 10, 2021

View reviewed changes

yanjunz97 force-pushed the optional-pod-labels branch from 4cb5ae2 to c84bb06 Compare September 10, 2021 23:47

yanjunz97 requested a review from dreamtalen September 13, 2021 18:16

dreamtalen reviewed Sep 13, 2021

View reviewed changes

yanjunz97 force-pushed the optional-pod-labels branch from c84bb06 to e7329e2 Compare September 13, 2021 21:50

yanjunz97 requested a review from srikartati September 13, 2021 22:20

srikartati reviewed Sep 13, 2021

View reviewed changes

srikartati added the area/flow-visibility Issues or PRs related to flow visibility support in Antrea label Sep 14, 2021

yanjunz97 force-pushed the optional-pod-labels branch from d642015 to 3d2f1f7 Compare September 14, 2021 17:58

srikartati requested a review from antoninbas September 15, 2021 00:36

srikartati previously approved these changes Sep 15, 2021

View reviewed changes

antoninbas reviewed Sep 21, 2021

View reviewed changes

antoninbas reviewed Sep 23, 2021

View reviewed changes

yanjunz97 dismissed srikartati’s stale review via 8433f92 September 23, 2021 21:11

yanjunz97 force-pushed the optional-pod-labels branch 2 times, most recently from b9d95d8 to 7f1ec92 Compare September 23, 2021 23:51

srikartati reviewed Sep 24, 2021

View reviewed changes

yanjunz97 force-pushed the optional-pod-labels branch from 7f1ec92 to 7ec7f44 Compare September 24, 2021 19:47

yanjunz97 requested a review from srikartati September 28, 2021 23:42

srikartati reviewed Sep 29, 2021

View reviewed changes

yanjunz97 force-pushed the optional-pod-labels branch from 7ec7f44 to 19777b9 Compare September 29, 2021 00:44

srikartati previously approved these changes Sep 29, 2021

View reviewed changes

antoninbas previously approved these changes Sep 29, 2021

View reviewed changes

yanjunz97 dismissed stale reviews from antoninbas and srikartati via 0302334 October 1, 2021 21:34

yanjunz97 force-pushed the optional-pod-labels branch 2 times, most recently from 0302334 to 4e89fd6 Compare October 1, 2021 21:37

antoninbas reviewed Oct 2, 2021

View reviewed changes

Set pod labels to be optional fields in the network flow records

4b37fbb

Signed-off-by: Yanjun Zhou <zhouya@vmware.com>

yanjunz97 force-pushed the optional-pod-labels branch from 4e89fd6 to 4b37fbb Compare October 2, 2021 00:35

antoninbas approved these changes Oct 2, 2021

View reviewed changes

srikartati approved these changes Oct 4, 2021

View reviewed changes

yanjunz97 added this to the Antrea v1.4 release milestone Oct 11, 2021

antoninbas merged commit ee5d3c4 into antrea-io:main Oct 18, 2021

	# Provide whether source and destination Pod labels will be included in the flow record.
	# Determine whether source and destination Pod labels will be included in the flow records.

	return 0, fmt.Errorf("%s not present. returned error: %v", ie, err)
	return 0, fmt.Errorf("error when getting InformationElement %s from registry: %v", ie, err)

		if fa.includePodLabels {
		if !fa.aggregationProcess.AreExternalFieldsFilled(*record) {

Set pod labels to be optional fields in the network flow records #2739

Set pod labels to be optional fields in the network flow records #2739

Conversation

yanjunz97 commented Sep 9, 2021 • edited

codecov-commenter commented Sep 9, 2021 • edited

Codecov Report

dreamtalen 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

srikartati Sep 10, 2021 • edited

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

srikartati Sep 10, 2021 • edited

Choose a reason for hiding this comment

dreamtalen commented Sep 10, 2021

yanjunz97 commented Sep 10, 2021

yanjunz97 commented Sep 13, 2021

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

yanjunz97 commented Sep 13, 2021

srikartati left a comment

Choose a reason for hiding this comment

srikartati commented Sep 13, 2021

yanjunz97 commented Sep 14, 2021

yanjunz97 commented Sep 14, 2021

yanjunz97 commented Sep 14, 2021

yanjunz97 commented Sep 14, 2021

srikartati left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati Sep 21, 2021 • edited

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

srikartati commented Sep 21, 2021

dreamtalen commented Sep 22, 2021

antoninbas commented Sep 22, 2021

srikartati commented Sep 22, 2021

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

antoninbas commented Sep 23, 2021

yanjunz97 commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanjunz97 commented Sep 24, 2021

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

srikartati left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Sep 29, 2021

yanjunz97 commented Sep 29, 2021

antoninbas commented Oct 1, 2021

yanjunz97 commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanjunz97 commented Sep 9, 2021 •

edited

codecov-commenter commented Sep 9, 2021 •

edited

srikartati Sep 10, 2021 •

edited

srikartati Sep 10, 2021 •

edited

srikartati Sep 21, 2021 •

edited