WIP - support for KubernetesInDocker (kind) clusters #75

davidkydd · 2021-06-28T09:41:44Z

Reverted previous premature merge (original PR here: #69 )

test image available @ dakyddacr.azurecr.io/periscope/kind:v25

- support multiple exporters - refactor main() to better encapsulate logic in sub-funcs - add multierror package, currently only used when collector.export errors; to prevent one failing exporter preventing the others from running - removed zip and export mode flag for now

…p level deployment yaml - to avoid changing the one used by downstream tools (vscode / cli)

(cherry picked from commit 910ed97)

add default .gitattributes file and git add --renormalize

# Conflicts: # cmd/aks-periscope/aks-periscope.go # go.mod # go.sum # pkg/collector/collector.go # pkg/exporter/azureblob_exporter.go # pkg/utils/helper.go

…LECTOR_LIST. New temp deployment config for kind, to be soon replaced with Kustomize version

…conversion

…be plumbed into main app

…t file

… Rename var to avoid collision with imported package

arnaud-tincelin

Sorry but I'm lost in this PR :(

arnaud-tincelin · 2021-07-02T10:23:19Z

cmd/aks-periscope/aks-periscope.go

+		zipAndExport = true
+	}
+
+	if zipAndExport {


The PR is about Kind cluster. Why all these changes in this file? What are we doing here?

Concurrent refactor to use more smaller funcs with clearly defined purpose in main execution loop.
ZipAndExport step made configurable also

Let's have 3 PR:

configuration of exporters + mechanism to run (or not) collectors / diagnosers

PR for zip (not sure what we need to do here)

PR to support kind cluster: if others PR are done, this one shall be very small

ZipAndExport step shall not exist. We will remove it asap but I know other tools such as vscode are using it.
This zip step is a duplicate of the single exports per collector / diagnoser

arnaud-tincelin · 2021-07-02T10:24:03Z

cmd/aks-periscope/aks-periscope.go

+	var enabledCollectorNames []string
+
+	//TODO try get partners to move from COLLECTOR_LIST to use ENABLED_COLLECTORS instead, for now COLLECTOR_LIST takes precedence if defined
+	collectorList, found := os.LookupEnv("COLLECTOR_LIST")


it's just an env variable, we can drop it safely this won't break anything

arnaud-tincelin · 2021-07-02T10:24:36Z

cmd/aks-periscope/aks-periscope.go

+}
+
+// selectedExporters select the exporters to run
+func selectExporters(allExporters map[string]interfaces.Exporter) []interfaces.Exporter {


This is out of the PR scope

This was part of earlier refactor to make which components are run configurable, exporters done in same consistent pattern as collectors and diagnosers. Intended to support Local Machine Exporter scenario also

I understand but right now the PR is hard to review and you have to deal with our numerous comments.
I think exporters shall not be treated the same way as collectors and diagnosers: exporters shall be configured by the user (export to blob, to disk, ...) but as we already discussed, I believe collectors and diagnosers should be 'hidden' from the users.

May be before a PR to support Kind cluster we should address these points in a dedicated PR ?

arnaud-tincelin · 2021-07-02T10:25:57Z

deployment/daemon-set.yaml

@@ -31,6 +31,12 @@ spec:
            name: kubeobjects-config
        - configMapRef:
            name: nodelogs-config
+        - configMapRef:


What's the link with Kind cluster?

The ability to define which collectors to run in the yaml is intended as a replacement for the COLLECTOR_LIST env var which seems less obvious in what it will do. Diagnosers and Exporters done in same way for consistency

Necessity to filter which collectors are run was required when adding support for kind clusters

I mean - its possible we could instead refactor all the collectors to be resilient to any possible errors across any possible environments as you have mentioned previously.

However the ability to define which collectors / diagnosers / exporters to run is still a valuable feature outside of this immediate requirement, and it is easier to do this now than to refactor all the components as described above.

arnaud-tincelin · 2021-07-02T10:32:49Z

pkg/collector/containerlogs_collector_containerd.go

+
+//GetAllContainerLogFilesThatHaveEverRunOnHost gets the list of log files for all containers that have ever run on the host
+func (collector *ContainerLogsCollectorContainerD) GetAllContainerLogFilesThatHaveEverRunOnHost() ([]string, error) {
+	output, err := utils.RunCommandOnHost("ls", containerLogDirectory)


Yeah the idea is to use adequate libraries : for kubectl use https://github.com/kubernetes/client-go, for helm use https://github.com/helm/helm/blob/main/pkg/kube/client.go, to access files we can mount volumes and use io / ioutils libraries, etc ...
I'm not saying to remove RunCmdOnHost now but aks-periscope is not aimed to be a wraper to call cli tools. We have libraries to do it properly so let's do it :)
Leave old code as is but for new code, let's use the libraries, especially regarding files

arnaud-tincelin · 2021-07-02T10:33:28Z

pkg/exporter/azureblob_exporter.go

@@ -32,48 +33,8 @@ func NewAzureBlobExporter(creationTime, hostname string) *AzureBlobExporter {
 	}
 }

-func createContainerURL() (azblob.ContainerURL, error) {


why the exporter changes?

as below, we needed to make the storage container name generation logic more resilient for additional environments, examples can be seen in tests. Also made exporter consistent with collector / diagnoser pattern and exposed it in config similarly.

ok, cool. I'd suggest to move this whole thing to another PR

Tatsinnit · 2021-07-02T11:43:55Z

cmd/aks-periscope/aks-periscope.go

-
-	// TODO: Hack: for now AKS-Periscope is running as a deamonset so it shall not stop (or the pod will be restarted)
-	// Revert from https://github.com/Azure/aks-periscope/blob/b98d66a238e942158ef2628a9315b58937ff9c8f/cmd/aks-periscope/aks-periscope.go#L70
-	select {}


💡side note: See the comment above, this is needed at its current state. Unless something is changed which fixes this.

this is still here - just moved to end of main.go function (line 83) which is better place for it as it is more obvious that it is (and needs to be) the final command in the program. In main() it is less likely to be refactored to a position where it ends up running NOT at the very end of the program, which would introduce a bug

Tatsinnit · 2021-07-02T11:44:52Z

cmd/aks-periscope/aks-periscope.go

 }

-func contains(flagsList []string, flag string) bool {


💡Key: Not sure what are these changes, and what was the intention of removing this function?

"contains" was moved to helpers.go which is a more natural home

pkg/exporter/azureblob_exporter.go

Tatsinnit · 2021-07-02T11:47:26Z

pkg/utils/helper.go

 	lines := strings.Split(output, "\n")
 	for _, line := range lines {
 		index := strings.Index(line, "server: ")
 		if index >= 0 {
 			fqdn := line[index+len("server: "):]
-			fqdn = strings.Replace(fqdn, "https://", "", -1)


🐛 Why is this removed? Test for private cluster, this was specific for that. (I think)

replaced with equivalent but more robust code in lines 116-126 which parses fqdn urlString to a url type, and then splits off the host to return. Test cases added to helper_test

Tatsinnit · 2021-07-02T11:49:18Z

pkg/utils/helper.go

@@ -312,6 +360,16 @@ func GetResourceList(kubeCmds []string, separator string) ([]string, error) {
 	return strings.Split(strings.Trim(resourceList, "\""), separator), nil
 }

+//contains checks if an array contains a value
+func Contains(flagsList []string, flag string) bool {


💡 Not able to follow this moving around of code because previously this funct was privately used in the main aks-perisceop.go file

its been moved to helpers.go unchanged and any references updated. There are tonnes of methods in helpers.go which could be considered private methods of aks-periscope.go, determination is based on what the method does - this is definitely a utils kind of helper method so doesn't need to stay in aks-periscope.go

Tatsinnit

💡 Thanks for this,I will add bit more latter. I am not able to follow changes in this PR because it seems this PR is more then adding Kind support (as title suggest) and some of the code movement around is something I am still going through and understanding.

Regarding supporting the actual kind cluster, we should implement well thought mechanism could be something like detailed here: https://github.com/Azure/aks-periscope/pull/75/files#r662950855

cc: @arnaud-tincelin, - for more thoughts on other changes.

🙏 Thank you

… method is robust enough to handle all scenarios (as shown by tests)

…beconfig locations before erroring (/var/lib/kubelet/kubeconfig first)

…//kubernetes.io/docs/concepts/storage/volumes/#hostpath and move it to /var/hostlog so periscope can still write to /var/log

davidkydd

Sorry but I'm lost in this PR :(

It might be easier to review just the raw code itself outside of diff form by checking out the branch the PR is on.

I usually find this easier with larger PR's which change lots of bits of files due to refactoring, e.g. I had to do this recently for your PR #65

davidkydd · 2021-07-02T11:46:17Z

cmd/aks-periscope/aks-periscope.go

+		zipAndExport = true
+	}
+
+	if zipAndExport {


Concurrent refactor to use more smaller funcs with clearly defined purpose in main execution loop.
ZipAndExport step made configurable also

davidkydd · 2021-07-02T11:50:26Z

cmd/aks-periscope/aks-periscope.go

+}
+
+// selectedExporters select the exporters to run
+func selectExporters(allExporters map[string]interfaces.Exporter) []interfaces.Exporter {


This was part of earlier refactor to make which components are run configurable, exporters done in same consistent pattern as collectors and diagnosers. Intended to support Local Machine Exporter scenario also

davidkydd · 2021-07-02T11:55:17Z

deployment/daemon-set.yaml

@@ -31,6 +31,12 @@ spec:
            name: kubeobjects-config
        - configMapRef:
            name: nodelogs-config
+        - configMapRef:


The ability to define which collectors to run in the yaml is intended as a replacement for the COLLECTOR_LIST env var which seems less obvious in what it will do. Diagnosers and Exporters done in same way for consistency

davidkydd · 2021-07-02T11:56:32Z

deployment/daemon-set.yaml

@@ -31,6 +31,12 @@ spec:
            name: kubeobjects-config
        - configMapRef:
            name: nodelogs-config
+        - configMapRef:


Necessity to filter which collectors are run was required when adding support for kind clusters

davidkydd · 2021-07-02T19:58:03Z

cmd/aks-periscope/aks-periscope.go

-
-	// TODO: Hack: for now AKS-Periscope is running as a deamonset so it shall not stop (or the pod will be restarted)
-	// Revert from https://github.com/Azure/aks-periscope/blob/b98d66a238e942158ef2628a9315b58937ff9c8f/cmd/aks-periscope/aks-periscope.go#L70
-	select {}


this is still here - just moved to end of main.go function (line 83) which is better place for it as it is more obvious that it is (and needs to be) the final command in the program. In main() it is less likely to be refactored to a position where it ends up running NOT at the very end of the program, which would introduce a bug

davidkydd · 2021-07-02T20:34:06Z

pkg/exporter/azureblob_exporter.go

-	containerName := strings.Replace(APIServerFQDN, ".", "-", -1)
-	containerLen := strings.Index(containerName, "-hcp-")
-	if containerLen == -1 {
-		containerLen = maxContainerNameLength
-	}
-	containerName = strings.TrimRight(containerName[:containerLen], "-")


this is the only section of the original code which is changed, extracted into new function getStorageContainerName

davidkydd · 2021-07-02T20:38:03Z

pkg/exporter/azureblob_exporter.go

@@ -32,48 +33,8 @@ func NewAzureBlobExporter(creationTime, hostname string) *AzureBlobExporter {
 	}
 }

-func createContainerURL() (azblob.ContainerURL, error) {


as below, we needed to make the storage container name generation logic more resilient for additional environments, examples can be seen in tests. Also made exporter consistent with collector / diagnoser pattern and exposed it in config similarly.

pkg/utils/helper.go

davidkydd · 2021-07-02T20:56:59Z

pkg/utils/helper.go

 	lines := strings.Split(output, "\n")
 	for _, line := range lines {
 		index := strings.Index(line, "server: ")
 		if index >= 0 {
 			fqdn := line[index+len("server: "):]
-			fqdn = strings.Replace(fqdn, "https://", "", -1)


replaced with equivalent but more robust code in lines 116-126 which parses fqdn urlString to a url type, and then splits off the host to return. Test cases added to helper_test

davidkydd · 2021-07-02T21:04:23Z

pkg/utils/helper.go

@@ -312,6 +360,16 @@ func GetResourceList(kubeCmds []string, separator string) ([]string, error) {
 	return strings.Split(strings.Trim(resourceList, "\""), separator), nil
 }

+//contains checks if an array contains a value
+func Contains(flagsList []string, flag string) bool {


its been moved to helpers.go unchanged and any references updated. There are tonnes of methods in helpers.go which could be considered private methods of aks-periscope.go, determination is based on what the method does - this is definitely a utils kind of helper method so doesn't need to stay in aks-periscope.go

Tatsinnit · 2021-07-06T05:14:34Z

💡 Important note: Kubernetes in docker is deprecated now, is the key purpose of this PR will be affected by this?

FYI: https://kubernetes.io/blog/2020/12/02/dont-panic-kubernetes-and-docker/

Thanks,

davidkydd · 2021-07-06T06:31:42Z

💡 Important note: Kubernetes in docker is deprecated now, is the key purpose of this PR will be affected by this?

FYI: https://kubernetes.io/blog/2020/12/02/dont-panic-kubernetes-and-docker/

Thanks,

Hey, no this PR isn't affected - also its not quite right to say that "Kubernetes in Docker is being deprecated", as its the dockershim container runtime which is being deprecated and replaced with containerd. KIND certainly isn't being deprecated (see banner at the tope of https://kind.sigs.k8s.io/).

As this PR adds a containerd based containerLog collector you could say this PR is even MORE applicable considering this deprecation 😄

Tatsinnit

💡 Thank you for this, here are some final observations:

This PR is not just Specific k8sindocker support but 3 other things, i.e. containerd, some local machine support along with code move around hence it's little confusing to follow the flow of logic.
- If we divide this into smaller feature based PR we might be able to understand better.
  - Especially, LocalMachine mechanism, and specifics of k8sindocker support.
Might be split into smaller PR: I agree with initial comments of @arnaud-tincelin that it seems like this PR is not just Support for some specific Kind but became more then 3 features in 1. It will be much easy to spilt into smaller more chunks for better understanding. Unless it cannot be separated.
For tests I would recommend Gomega which is already used within this project and much better assertion notation.
For localmachine work: What is it trying to achieve? how are you defining where to store logs in user machine?

I will defer to @arnaud-tincelin for further comments, suggestions or recommendation.

I guess with test published image, testing all scenarios will be key as usual.

Thank you. 🙏

Tatsinnit · 2021-07-07T08:15:08Z

pkg/utils/helper_test.go

+
+// GetStorageContainerName get storage container name
+func TestParseAPIServerFQDNFromKubeConfig(t *testing.T) {
+	for _, tt := range parseAPIServerFQDNFromKubeConfigTests {


💡 what is tt acronym for?

Tatsinnit · 2021-07-07T08:27:26Z

pkg/utils/helper.go

+	output, err := RunCommandOnHost("cat", "/var/lib/kubelet/kubeconfig")
+	if err != nil {
+		result = multierror.Append(result, err)
+		output, err = RunCommandOnHost("cat", "/etc/kubernetes/kubelet.conf")


💡 I would recommend commenting the reason of this config use: /etc/kubernetes/kubelet.conf, especially what specific scenario is fallback consist. (Comment to clarify that this not for the support for wider kind cluster behaviour like GKE, AWS et. al. its specific to fall back 1 scenario).

just some thoughts.

@Tatsinnit, the reason for using /etc/kubernetes/kubelet.conf is to override the hardcoded value /var/lib/kubelet/kubeconfig. For kind and kubeadm distros, we will get a "can't read kubeconfig file" if we try to get the APIServerFQDN from the filepath /var/lib/kubelet/kubeconfig.

Yep, @sophsoph321, thanks, Indeed, that is why as recommended, I think it is important to leave a comment i.e. add a code level comment for this: commenting the reason within file this config is use, especially when specific code if for k8sindocker / (KubeKDM) kind it is not every kind Kubernetes cluster?

For anyone to look at the code it will make it clear that what all this change was specifically for.

Tatsinnit · 2021-07-07T08:32:02Z

pkg/utils/helper.go

+		output, err = RunCommandOnHost("cat", "/etc/kubernetes/kubelet.conf")
+		if err != nil {
+			result = multierror.Append(result, err)
+			return "", fmt.Errorf("open kubeconfig file at /etc/kubernetes/kubelet.conf or /var/lib/kubelet/kubeconfig\": %+v", result)


💡 if you extract these 2 config with more apt names, it will be easy to avoid repetition. /etc/kubernetes/kubelet.conf and /var/lib/kubelet/kubeconfig

also why do we need escape here: \":?

Tatsinnit · 2021-07-07T08:44:41Z

pkg/utils/helper_test.go

+		t.Run(tt.APIServerFQDN, func(t *testing.T) {
+			APIServerFQDN, err := ParseAPIServerFQDNFromKubeConfig(tt.kubeConfig)
+			if err != nil {
+				t.Errorf("utils.TestParseAPIServerFQDNFromKubeConfig(%q) Error: %q, expected %q",


💡Please use Gomega, which is part of this project, which will help in built in verbs to test for expect, Not, HaveOccurred etc. more here: https://onsi.github.io/gomega/

Tatsinnit · 2021-07-07T08:46:00Z

pkg/utils/helper_test.go

@@ -0,0 +1,69 @@
+package utils


💡 Is this intentional to only test 1 function, why not other moved functs which are part of this PR?

Tatsinnit · 2021-07-07T08:47:56Z

pkg/exporter/azureblob_exporter_test.go

+			containerName, _ := getStorageContainerName(tt.apiServerFqdn)
+
+			if containerName != tt.containerName {
+				t.Errorf("TestGetStorageContainerName(%q) => %q, want %q",


💡recommendation: Please use Gomega. (its already part of this project)

Tatsinnit · 2021-07-07T08:54:11Z

pkg/collector/containerlogs_collector_containerd.go

@@ -0,0 +1,153 @@
+package collector


💡 Can we please keep same naming consistency pattern like other files: containerlogs_and_containerd_collector.go

Tatsinnit · 2021-07-07T08:58:05Z

pkg/utils/helper_test.go

@@ -0,0 +1,69 @@
+package utils
+


💡 Not able to see private cluster test in helper_test as mentioned here: https://github.com/Azure/aks-periscope/pull/75/files#r662953574

arnaud-tincelin

I think we should split this PR as it's taking too long to review, Tatsat and I are getting lost about the reasons behind several changes and we disagree on some points

arnaud-tincelin · 2021-07-07T13:35:32Z

pkg/collector/containerlogs_collector_containerd.go

+
+//GetAllContainerLogFilesThatHaveEverRunOnHost gets the list of log files for all containers that have ever run on the host
+func (collector *ContainerLogsCollectorContainerD) GetAllContainerLogFilesThatHaveEverRunOnHost() ([]string, error) {
+	output, err := utils.RunCommandOnHost("ls", containerLogDirectory)


This is the reason why we mount /var/log:

volumes: - name: varlog hostPath: path: /var/log

nicoletaz3 · 2021-07-08T18:05:08Z

Hi Tatsat, Docker is deprecated as a container runtime in Kubernetes, but not KIND which is a tool for running Kubernetes in a lightweight manner.

In reply to: 874466230

Tatsinnit · 2021-07-08T19:50:12Z

Hi Tatsat, Docker is deprecated as a container runtime in Kubernetes, but not KIND which is a tool for running Kubernetes in a lightweight manner.

In reply to: 874466230

Thank you @nicoletaz3 , yep sounds good. Really appreciate your reply, As a archival history I left this question since there is no other design-doc but this PR for this work. 🙏

Regarding this PR: There are bunch of key comments which are about other changes getting checked in as part of this PR and some are not clear, hence Arnaud’s and mine comments above in case you are reviewing this PR please feel free to add comments. Thank you so much.

davidkydd · 2021-07-12T22:09:06Z

Closing - will be refactored into multiple smaller PRs

davidkydd and others added 30 commits April 22, 2021 14:01

support for collecting from CRDs using default deployment yaml

dc4884b

Merge branch 'master' into dakydd/master

bc21285

Merge branch 'master' of https://github.com/Azure/aks-periscope

b516b5c

revert additional resource permissions

f2683a6

expose a feature flag for collectors and diagnosers

9a8e367

typo

5d344b9

fix targeting configmap rather than config itself

5104ef7

minor tidy

ecfe3ff

create dev deployment yaml in its own subdir and revert changes to to…

864297a

…p level deployment yaml - to avoid changing the one used by downstream tools (vscode / cli)

go fmt

790744a

rename for clarity

2b66c39

Helm chart for azure-k8s-periscope

308e350

(cherry picked from commit 910ed97)

add new configmaps to chart

d569772

WIP - support for KubernetesInDocker (kind) clusters

4e69de1

simplify err => boolean

a377636

reset line endings to crlf

228cf6b

Add text=auto in .gitattributes (Azure#70)

bdb3847

add default .gitattributes file and git add --renormalize

Merge branch 'collectorDiagnoserConfig' into kindClusterSupport

a905fe9

# Conflicts: # cmd/aks-periscope/aks-periscope.go # go.mod # go.sum # pkg/collector/collector.go # pkg/exporter/azureblob_exporter.go # pkg/utils/helper.go

Fix additional merge issues

7c5c80d

overload collectors-config to support both ENABLED_COLLECTORS and COL…

9b9a582

…LECTOR_LIST. New temp deployment config for kind, to be soon replaced with Kustomize version

Remove helm

915b745

Remove dev periscope yaml

7032976

Handle kubeadm (perhaps poorly) and aks-engine fqdn -> containerName …

5a1df46

…conversion

additional test cases

92a767c

new containerlogs collector for containerd clusters - still needs to …

12bbfd3

…be plumbed into main app

register containerD collector

0a5aa64

reverse the kind vs. non-kind check to be AKS vs. non-AKS

2bb4a37

register the containerlogscontainerd collector for the kind deploymen…

0c61fed

…t file

remove unused config

fc7bc03

davidkydd added 6 commits July 2, 2021 16:24

Contains as exported method of utils / helper. Fix test error string.…

f284b5d

… Rename var to avoid collision with imported package

fix containerd logs tests

77dd262

remove golangci-lint

f759365

recalc go.sum

7f957de

correct test expectation

562f1bf

replace additional deployment yaml with kustomize

0439046

arnaud-tincelin suggested changes Jul 2, 2021

View reviewed changes

Tatsinnit reviewed Jul 2, 2021

View reviewed changes

pkg/exporter/azureblob_exporter.go Show resolved Hide resolved

Tatsinnit reviewed Jul 2, 2021

View reviewed changes

Tatsinnit requested changes Jul 2, 2021

View reviewed changes

davidkydd added 5 commits July 3, 2021 08:18

remove AKS specificity in getStorageContainerName now that the shared…

ff57ee0

… method is robust enough to handle all scenarios (as shown by tests)

Merge branch 'upstream-master' into kindClusterSupport

f7b431f

remove IsRunningInAks check, ReadKubeletConfig now just tries both ku…

6967c8b

…beconfig locations before erroring (/var/lib/kubelet/kubeconfig first)

return combined result. Change errorf string format

5335b60

enforce varlog volume as a directory. Make it readonly as per: https:…

fb94b99

…//kubernetes.io/docs/concepts/storage/volumes/#hostpath and move it to /var/hostlog so periscope can still write to /var/log

davidkydd commented Jul 6, 2021

View reviewed changes

fix name of collector

b4963c3

Tatsinnit requested changes Jul 7, 2021

View reviewed changes

arnaud-tincelin suggested changes Jul 7, 2021

View reviewed changes

Merge branch 'upstream-master' into kindClusterSupport

1747a88

davidkydd closed this Jul 12, 2021

Tatsinnit mentioned this pull request Jul 12, 2021

Local Machine exporter #57

Closed

WIP - support for KubernetesInDocker (kind) clusters #75

WIP - support for KubernetesInDocker (kind) clusters #75

Conversation

davidkydd commented Jun 28, 2021 • edited Loading

arnaud-tincelin 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

Tatsinnit Jul 2, 2021 • 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

Tatsinnit left a comment • edited Loading

Choose a reason for hiding this comment

davidkydd 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

Tatsinnit commented Jul 6, 2021 • edited Loading

davidkydd commented Jul 6, 2021

Tatsinnit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatsinnit Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatsinnit Jul 7, 2021 • 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

arnaud-tincelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoletaz3 commented Jul 8, 2021

Tatsinnit commented Jul 8, 2021 • edited Loading

davidkydd commented Jul 12, 2021

davidkydd commented Jun 28, 2021 •

edited

Loading

Tatsinnit Jul 2, 2021 •

edited

Loading

Tatsinnit left a comment •

edited

Loading

Tatsinnit commented Jul 6, 2021 •

edited

Loading

Tatsinnit left a comment •

edited

Loading

Tatsinnit Jul 7, 2021 •

edited

Loading

Tatsinnit Jul 7, 2021 •

edited

Loading

Tatsinnit commented Jul 8, 2021 •

edited

Loading