Refactor kubernetes autodiscover to avoid skipping short-living pods

[jsoriano]

What does this PR do?

Refactor logic in kubernetes autodiscover that decides when to generate events to try to address #22718.

Kubernetes autodiscover can generate events without network information now (without host or port/ports). This allows to generate events for pods that haven't started yet, or have succeeded/failed before generating a running event. These events still include the container id, so they can be used to collect logs. Still, no start event is generated if no pod ip and no container ids are available.

Some helpers have been added to obtain relevant information from pods and their containers.

Some additional small refactors are done to improve readability.

Why is it important?

Current logic is checking similar things at the pod and container levels, try to simplify this logic focusing in containers only.

No matter what is the state of the pod, if there is a container running or trying to run, even if it is unhealthy, some configuration should be generated, so logs can be collected.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works. But more tests would be needed.
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

• Fix update so it definitely stops configurations, respecting the cleanup timeout.
• Double-check that nothing breaks with events without data.host.
  This is logged at the debug level:

  2021-04-08T12:53:34.453+0200	DEBUG	[autodiscover]	template/config.go:156	Configuration template cannot be resolved: field 'data.host' not available in event or environment accessing 'hosts' (source:'/home/jaime/tmp/metricbeat-autodiscover-k8s.yml')
  

• Actually check that the refactor solves the issues and doesn't break anything else.
• Try to add more testing. A couple of cases added to current tests.
• Check that nothing breaks with hints including data.host or data.ports.
  • data.ports.* doesn't seem to be working, check if this is a regression. Named ports were not added to container events. They are now.
  • Multiple configurations generated for pods with multiple containers, expected? compare with master. False alarm.
• Check autodiscover with heartbeat.

Author's notes for the future

• When using autodiscover templates, if the condition matches pod conditions, container events will match too because they also include pod metadata. In pods with multiple containers this may lead to events with metadata of the incorrect container. This could be possibly avoided by matching per pod metadata only in the pod events. This is also happening in master, not solving in this PR.
• data.ports is not included in container-level events, so it doesn't work when conditions for specific containers are used.

How to test this PR locally

• Check that everything keeps running with some autodiscover happy cases.
• Check that logs are collected from a pod in CrashLoopBackOff (kubectl run --image=redis redis -- echo foo).

Related issues

• Closes Kubernetes autodiscover doesn't discover short living jobs (and pods?) #22718.

Use cases

Collect logs from containers in short-living or failing pods.

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: jsoriano commented: /test

• Start Time: 2021-04-20T07:56:16.321+0000

• Duration: 46 min 55 sec

• Commit: 93995c7

Test stats 🧪

Test	Results
Failed	0
Passed	353
Skipped	115
Total	468

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	353
Skipped	115
Total	468

[hukaixuan]

Face the same issue, thanks for your great job! When will this refactor be done?

[jsoriano]

@hukaixuan thanks for your interest, I expect to dedicate some time to this in the following days, but I cannot give a fixed timeline, stay tunned to this PR 🙂

[segevmatuti1]

Hey @jsoriano !
encountered the same issue as well, appreciate your work here!
do you have a docker image of your changes I can use to test on my end?

[jsoriano]

Hey @jsoriano !
encountered the same issue as well, appreciate your work here!
do you have a docker image of your changes I can use to test on my end?

Hi, this is still a draft, it still doesn't cover all cases, I actually know it doesn't work well with some basic cases.

[jsoriano]

/test

[ChrsMark]

Overall looks good so far 🍻 :

1. spotted one tricky line that might be a bug (maybe not :)). I might need to have a second pass with a clearer mind for hidden bug hunting (I believe there are none though)
2. @jsoriano do you think you can improve testing notes a bit? I guess you have some specific scenarios you used to test this one. Would be nice to have them logged in this PR both for manual testing but also for future reference maybe.

[ChrsMark]

🤔 Will this just keep only the last "accessed" port of the loop because we create a new MapStr each time?

[jsoriano]

Yes, I have changed the behaviour here, I have moved data.ports from the pod event to each container/port event. Let me know if you think this can be a problem.

Before, data.ports was included only in the pod event, so when configurations matched they didn't include the container metadata, only the pod metadata.

Code in this PR doesn't set the whole collection of data.ports in the pod event, it sets a single data.ports.<portname> field in to each container/port event, so when configurations match they include also the container metadata. By doing this each event with a named port is going to contain a single port.

This also fixes something I found misleading during my tests. As data.ports was included only in pod events, templates with conditions matching container fields, like the following one, didn't work. They work now with the changes here.

      templates:
        - condition:
            contains.kubernetes.container.image: redis
          config:
            - module: redis
              metricsets: ["info", "keyspace"]
              hosts: "${data.host}:${data.ports.redis}"

[jsoriano]

You can see more clearly how this is different now in these test cases: https://github.com/elastic/beats/pull/24742/files#diff-9f24ba274864b933a726842c2b9fb8ee487a13545579938f9e373d0463611142R557

[jsoriano]

Ok, what doesn't work now is this case. I will revert this change and do some more tests.

      templates:
        - condition:
            contains.kubernetes.pod.name: redis
          config:
            - module: redis
              hosts: "${data.host}:${data.ports.redis}"
            - module: nginx
              hosts: "${data.host}:${data.ports.http}"

We need more testing to cover all these cases 😣

[ChrsMark]

Ah I see, it makes sense!

[jsoriano]

Reverted in 3301827, I will follow with this in a separate PR.

[jsoriano]

2. @jroriano do you think you can improve testing notes a bit? I guess you have some specific scenarios you used to test this one. Would be nice to have them logged in this PR both for manual testing but also for future reference maybe.

@ChrsMark I have some quite simple kubernetes manifests, that represent some cases in a list in a private manual test case. I would like to translate this at some moment to system or e2e tests.

[jsoriano]

/test

[ChrsMark]

lgtm, thank you!

[ChrsMark]

/test

[exekias]

LGTM! let's make sure we test this thoroughly before releasing 😇

[jsoriano]

@ChrsMark @exekias Thanks for the reviews!

I did some additional tests and I found that the issue was not actually solved for hints-based autodiscover in filebeat, c9a3515 was missing. Could any of you please do a quick review of this latest change?

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b short-living-pods upstream/short-living-pods
git merge upstream/master
git push upstream short-living-pods

[jsoriano]

/test