Add container logs collector for containerd runtime #73

shwetamurali · 2021-06-24T00:53:52Z

Azure Stack has switched its container runtime from docker to container-d. aks-periscope needs to support container log collection under the new runtime. This is the current error received when trying to save logs:
Error: Collector: containerlogs, collect data failed: Fail to run command on host: exit status 127

Therefore, I added a new container log collector to work with container-d. This collector works similarly to the existing container log collector but uses slightly different logic to avoid using docker commands.

It uses crictl command to retrieve a list of all containers, and filters to the ones with the targeted namespace (based on user input). Then, it retrieves the logs using the container ID and saves them to the storage account.

This change has been tested on both Azure Stack and Public Azure and succeeds in collecting logs for both, under a folder called containerdlogs. This change is imperative since the runtime switch has already been made so there is no working container log collector as of now.

add default .gitattributes file and git add --renormalize

Tatsinnit

💡 Thank you for taking time for this, I have added initial few review comments below. At the moment I don't quiet understand the use-case as to will it be opt-in vs always needs to be there so I have few basic build up questions as well.

Tatsinnit · 2021-06-24T02:03:52Z

cmd/aks-periscope/aks-periscope.go

@@ -49,6 +50,7 @@ func main() {
 	collectors = append(collectors, dnsCollector)
 	collectors = append(collectors, kubeObjectsCollector)
 	collectors = append(collectors, networkOutboundCollector)
+	collectors = append(collectors, containerDLogsCollector)


💡 Thanks for this, I have few questions to understand more about this work, what is the use-case with vanilla aks-periscope tool run with this please? can you add an example in description:

Few questions:

So currently this tool runs on AKS Cluster, this work enables this critical command with in collector to always collect containers logs?

Will there be any scenario where user would not want this auto enabled?

Actually, if you run the current aks-periscope on a public AKS cluster, it will NOT collect any container logs, as AKS has switched from docker to contianerd as default container runtime. I think this new collector should replace the existing "docker" container collecter.
As for crictl, so crictl is the CLI tool to interact with containerd (precisely cri). Please read this article for details: https://www.tutorialworks.com/difference-docker-containerd-runc-crio-oci/

Tatsinnit · 2021-06-24T02:07:35Z

pkg/collector/containerdlogs_collector.go

+		return err
+	}
+
+	output, err := utils.RunCommandOnHost("crictl", "ps", "-a", "-o", "json")


💡 https://kubernetes.io/docs/tasks/debug-application-cluster/crictl/

I am wondering will this be an issue?

No, I don't think so. We are not using crictl to create anything, we are using it to retrieve container metadata and retrieve logs.

As we want to remove the RunCommandOnHost, could we have a look at https://github.com/containerd/containerd instead?

Tatsinnit · 2021-06-24T02:08:53Z

pkg/collector/containerdlogs_collector.go

+			containerID := container.ID
+			containerNameSpace, ok := container.Labels["io.kubernetes.pod.namespace"]
+			if !ok {
+				log.Printf("Unable to retrieve namespace of container: %+v", containerID)


Minor: consistency: see line 75 --> https://github.com/Azure/aks-periscope/pull/73/files#diff-a767a7e0a1997c77cf6883c7707d9559ab33c388e714275ffd439509335825f3R75 - message starts with upper case.

Sorry, I'm a bit confused by this comment? Should they both start with lowercase?

fmt.Errorf should start with a lowercase and use %w to wrap errors
log.Printf should start with a capital letter and you can't use %w there. So you did good here, nothing to change :)

pkg/collector/containerdlogs_collector.go

Tatsinnit · 2021-06-24T02:22:11Z

pkg/collector/containerdlogs_collector.go

+				if strings.HasPrefix(containerName, containerLogParts[1]) && containerNameSpace == targetNameSpace {
+					containerNames[containerID] = containerName
+				}
+			} else {


💡 This is interesting, what is the this if-else clause doing?

i.e. in both cases it achieves containerNames[containerID] = containerName resultset?

if len(containerLogParts) == 2 { if strings.HasPrefix(containerName, containerLogParts[1]) && containerNameSpace == targetNameSpace { containerNames[containerID] = containerName } } else { if containerNameSpace == targetNameSpace { containerNames[containerID] = containerName } }

Based on the README file in the root, users can specify containers like this:

az aks kollect \ -g MyResourceGroup \ -n MyManagedCluster \ --container-logs "mynamespace1/mypod1 myns2"

So --container-logs flag accepts two formats: "namespace/container" or just "namespace".
Here the logic is, if the length of containerLogParts is 2, meaning the input format matches "namespace/container", we need to make sure both namespace and container name match. Otherwise, we only need to check if the namespace matches.

BTW, if you check the existing container collector, it's the same logic over there.

Isn't it a duplicate of DIAGNOSTIC_CONTAINERLOGS_LIST? We might want to remove this

--container-logs "mynamespace1/mypod1 myns2" is used with az aks kollect (the az cli command), under the hood this just sets the value of DIAGNOSTIC_CONTAINERLOGS_LIST before deploying

Tatsinnit · 2021-06-24T02:24:11Z

pkg/collector/containerdlogs_collector.go

+				continue
+			}
+
+			containerName, ok := container.Labels["io.kubernetes.container.name"]


💡 How is this containerName used? i.e. see the line below where code have !ok the logic still continues does that means containerName will always be there? but !ok suggest otherwise.

Here is an assumption that not all containers are managed by Kubernetes, so when we try to retrieve the container name using this label, the key "io.Kubernetes.container.name" may not exist. In this case, we just skip collecting logs of this container, thus we use continue to skip this cycle of for loop.

Tatsinnit · 2021-06-24T02:33:58Z

pkg/collector/containerdlogs_collector.go

+		}
+
+		for containerID, containerName := range containerNames {
+			containerLog := filepath.Join(rootPath, targetNameSpace+"_"+containerName)


💡 targetNameSpace+"_"+containerName what this leads to please? might be add a comment? also, can we string concatenate in other way possible as well?

Similarly, if you see the existing docker container collector, it's the same logic -- we just borrow the code from there. But sure, we can improve this.

pkg/collector/collector.go

jadarsie · 2021-06-24T16:48:51Z

pkg/collector/containerdlogs_collector.go

+		targetNameSpace := containerLogParts[0]
+		containerNames := map[string]string{}
+
+		for _, container := range containers.ContainersList {


Runtime performance can be improved here. This code can potentially range over the entire set of node containers as many times as fields in DIAGNOSTIC_CONTAINERLOGS_LIST. It should be able to achieve the same result in one pass by using a map.

pkg/collector/containerdlogs_collector.go

davidkydd · 2021-06-25T04:41:05Z

Note I have also implemented a containerd containerLog collector here:
https://github.com/Azure/aks-periscope/pull/69/files#diff-7520e2334630376456ed95388019d5b5a7abcd6f77f2003a2c68bb7afab5e237

My implementation is slightly different in that instead of using crictl I just directly read the relevant logfiles from /var/logs/containers. Your implementation is complementary in that it allows collecting logs from only those containers which are running, whereas mine will collect all containers that have ever run on the node (and that match one of the filters).

Suggest this PR is merged first and then I will pull into my branch and add a simple collector-config flag which will allow selecting either "onlyRunning" or "all" behaviour, as I can see value in supporting both.

arnaud-tincelin · 2021-06-25T08:11:15Z

Note I have also implemented a containerd containerLog collector here:
https://github.com/Azure/aks-periscope/pull/69/files#diff-7520e2334630376456ed95388019d5b5a7abcd6f77f2003a2c68bb7afab5e237

My implementation is slightly different in that instead of using crictl I just directly read the relevant logfiles from /var/logs/containers. Your implementation is complementary in that it allows collecting logs from only those containers which are running, whereas mine will collect all containers that have ever run on the node (and that match one of the filters).

Suggest this PR is merged first and then I will pull into my branch and add a simple collector-config flag which will allow selecting either "onlyRunning" or "all" behaviour, as I can see value in supporting both.

I didn't look at the implementation but I like the idea better since we won't use any RunCmd and we won't have to rely on any tool installed on the host / container. Additionally, files can be streamed and that could be interesting for the future iterations

arnaud-tincelin · 2021-07-07T13:44:43Z

pkg/collector/containerdlogs_collector.go

+			if err != nil {
+				return err
+			}
+			err = utils.WriteToFile(containerLog, output)


You need to rebase this branch, we are not using WriteToFile anymore. I'll approve once you rebased and changed this code

Please don't merge this PR @arnaud-tincelin. It has been superseded by the approach in my KIND PR.

shwetamurali · 2021-07-08T18:58:50Z

Will be following David's approach instead.

davidkydd and others added 2 commits June 22, 2021 12:43

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

bdb3847

add default .gitattributes file and git add --renormalize

add container logs collector for container-d runtime

5cfce65

Tatsinnit requested review from arnaud-tincelin, Tatsinnit, davidkydd and JunSun17 and removed request for arnaud-tincelin June 24, 2021 02:35

Tatsinnit requested changes Jun 24, 2021

View reviewed changes

Tatsinnit assigned shwetamurali Jun 24, 2021

Tatsinnit linked an issue Jun 24, 2021 that may be closed by this pull request

Add new collector for logs with container-d runtime #68

Closed

shwetamurali changed the title ~~Add container logs collector for container-d runtime~~ Add container logs collector for containerd runtime Jun 24, 2021

jadarsie reviewed Jun 24, 2021

View reviewed changes

Tatsinnit added the do-not merge ❌🔀 Avoid merging for now label Jun 27, 2021

davidkydd force-pushed the master branch from 435de71 to d6fc553 Compare June 28, 2021 09:08

arnaud-tincelin suggested changes Jul 7, 2021

View reviewed changes

Tatsinnit removed the do-not merge ❌🔀 Avoid merging for now label Jul 7, 2021

shwetamurali closed this Jul 8, 2021

Tatsinnit mentioned this pull request Jul 22, 2021

support for kind clusters #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container logs collector for containerd runtime #73

Add container logs collector for containerd runtime #73

shwetamurali commented Jun 24, 2021

Tatsinnit left a comment

Tatsinnit Jun 24, 2021

songjiaxun Jun 24, 2021 •

edited

Loading

Tatsinnit Jun 24, 2021

songjiaxun Jun 24, 2021

arnaud-tincelin Jul 7, 2021

Tatsinnit Jun 24, 2021

shwetamurali Jun 24, 2021

arnaud-tincelin Jun 25, 2021

Tatsinnit Jun 24, 2021

songjiaxun Jun 24, 2021 •

edited

Loading

songjiaxun Jun 24, 2021

arnaud-tincelin Jun 25, 2021

davidkydd Jun 28, 2021

Tatsinnit Jun 24, 2021

songjiaxun Jun 24, 2021 •

edited

Loading

Tatsinnit Jun 24, 2021

songjiaxun Jun 24, 2021

jadarsie Jun 24, 2021

davidkydd commented Jun 25, 2021

arnaud-tincelin commented Jun 25, 2021

arnaud-tincelin Jul 7, 2021

davidkydd Jul 8, 2021

shwetamurali commented Jul 8, 2021

Add container logs collector for containerd runtime #73

Add container logs collector for containerd runtime #73

Conversation

shwetamurali commented Jun 24, 2021

Tatsinnit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songjiaxun Jun 24, 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

Choose a reason for hiding this comment

songjiaxun Jun 24, 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

songjiaxun Jun 24, 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

davidkydd commented Jun 25, 2021

arnaud-tincelin commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shwetamurali commented Jul 8, 2021

songjiaxun Jun 24, 2021 •

edited

Loading

songjiaxun Jun 24, 2021 •

edited

Loading

songjiaxun Jun 24, 2021 •

edited

Loading