Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

Merged
merged 56 commits into from
May 9, 2024

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Mar 5, 2024

Note: Dont merge until elastic/elastic-agent-autodiscover#81 is merged.
We need to udpate go.mod of the elastic-agent with v0.6.9 PR is merged

  • Enhancement

What does this PR do?

Relates to:

Why is it important?

It adds a helpful feature for those that use Hints Autodiscovery. It can be helpful to identify configuration issues

  • 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 (Some tests updated here)
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

How to test this PR locally

Follow same instructions as #3107

  1. Checkout this branch
  2. Update hints.go file of your /go/pkg/mod/github.com/elastic/[email protected]/utils as per hints.go
  3. Build elastic agent image
EXTERNAL=true DEV=true SNAPSHOT=true PLATFORMS=linux/arm64 PACKAGES=docker mage package
cd build/package/elastic-agent/elastic-agent-linux-arm64.docker/docker-build
docker build -t custom-agent-image .\n
kind load docker-image custom-agent-image:latest\n
  1. Install elastic-agent-standalone with hints enabled. Sample here
    elastic-agent-standalone.txt

  2. Configure some "wrong" co.elastic.hints annotation inside a pod.

annotations:        
        co.elastic.hints/enabled: 'true'
        co.elastic.hints/package: "container_logs"
        co.elastic.hints/hostssssss: '128.30.40.50'
  1. Warning messages should be available

Related issues

Logs

Deploy following pod with wrong annotations hosts and steam:

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: nginx
  name: nginx
  namespace: default
spec:
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
      annotations:
        co.elastic.hints/enabled: 'true'
        co.elastic.hints/hosts: '128.30.40.50'
        co.elastic.hints/steam: 'stdout'
    spec:
      containers:
      - image: nginx
        name: nginx
{"log.level":"warn","@timestamp":"2024-03-07T13:15:22.599Z","log.logger":"composable.providers.kubernetes","log.origin":{"file.name":"kubernetes/pod.go","file.line":219},"message":"provided hint: co.elastic/hints/hosts is not in the supported list for pod nginx-68d79864cb-rgjbb in namespace default","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}
{"log.level":"warn","@timestamp":"2024-03-07T13:15:22.599Z","log.logger":"composable.providers.kubernetes","log.origin":{"file.name":"kubernetes/pod.go","file.line":219},"message":"provided hint: co.elastic/hints/steam is not in the supported list for pod nginx-68d79864cb-rgjbb in namespace default","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}

Check that only annotations that start with co.elastic are the ones that are checked for correctness.

Screenshots

Screenshot 2024-03-07 at 3 21 19 PM

Copy link
Contributor

mergify bot commented Mar 5, 2024

This pull request does not have a backport label. Could you fix it @gizas? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot assigned gizas Mar 5, 2024
@mergify mergify bot added the backport-skip label Mar 5, 2024
@gizas
Copy link
Contributor Author

gizas commented Mar 5, 2024

  1. I might need to add integration tests -> Added https://github.com/elastic/elastic-agent-autodiscover/pull/81/files#diff-2f7d9a00d75cb68d90813219822383fdda49c60b39cc05f7d2713f3d9e18a84a
  2. To update changelog when is done
  3. The for loops is also present in elastic-agent-autodiscover library. Wondering if I can put there the block of checks to minmimise the for loops that happen. Now we add iterate again to take annotations of pods I moved the implementation to elastic-agent-autodiscovery function for performance

@gizas gizas marked this pull request as ready for review March 6, 2024 12:26
@gizas gizas requested review from a team as code owners March 6, 2024 12:26
//We check whether the provided annotation follows the supported format and vocabulary. The check happens for annotations that start with co.elastic.hints
hints, err := utils.GenerateHints(annotations, "", p.config.Prefix, allSupportedHints)
if err != nil {
p.logger.Warnf("%v for pod %v in namespace", err, pod.Name, pod.ObjectMeta.Namespace)
Copy link
Contributor Author

@gizas gizas Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment I set it as warning.
This kind of errors I think must be visible with default installation

But almost every 2sec we have a warning for one pod per wrong annotation. This can add noise
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be added as a warning like you did, but having it every 2sec is quite annoying...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will have it every two seconds until they fix it. If the only reason to get this warning is a wrong hint annotation, this will force them to fix it.
I would leave it as a warning.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Mar 6, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@pierrehilbert pierrehilbert requested a review from blakerouse March 6, 2024 14:09
@pchila
Copy link
Member

pchila commented Apr 30, 2024

@pchila thank you for the inputs here.

I merged with main but still elastic-agent build fails. I see >> (linux-arm64-ubuntu-2204-default) Test output (non-sudo) (stdout): === FAIL: testing/integration TestOtelFileProcessing (188.30s) is the one that always fails

This is tracked by #4617 as it shows up on main as well.

Moreover for the Sonarcube coverage, my additions are made in private emitRunning function which is called on Add event. This fucntion was not covered even before introduction of SonarCube

I added a test here but obviously is not matched with specific function. Can we merge this despite the sonarcube failure?

Without having a green sonarqube check, someone with admin privileges should force the merge
I didn't have the time to fully review your merge yet but I would really prefer if we could cover new code/functionality with unit tests rather than force-merge without automated unit tests

@gizas
Copy link
Contributor Author

gizas commented May 1, 2024

/test

@gizas
Copy link
Contributor Author

gizas commented May 1, 2024

@pchila , @blakerouse I keep trying merging and with main. I hope that soon the build will work :)

I have also reformatted the code and introduced a function to call in order to create a test for coverage
Locally I see now that my code is covered:
Screenshot 2024-05-01 at 11 31 11 AM

But now I have another problem that SonarCube seems stack and never reports anything back.
Any ideas ?

@gizas
Copy link
Contributor Author

gizas commented May 1, 2024

/test

@gizas gizas enabled auto-merge (squash) May 2, 2024 09:59
@gizas
Copy link
Contributor Author

gizas commented May 2, 2024

Nice!!!! it seems all tests are successful !!!!
@blakerouse , @pchila can I have final review and merge if no objection? (I have enabled auto-merge)

@ycombinator ycombinator requested a review from blakerouse May 2, 2024 12:32
@cmacknz
Copy link
Member

cmacknz commented May 3, 2024

@blakerouse please re-review, looks like go.mod has been fixed.

@cmacknz
Copy link
Member

cmacknz commented May 3, 2024

buildkite test it

Copy link
Contributor

mergify bot commented May 7, 2024

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 logshints upstream/logshints
git merge upstream/main
git push upstream logshints

@gizas
Copy link
Contributor Author

gizas commented May 8, 2024

@blakerouse please any objection before merging?

Copy link
Contributor

mergify bot commented May 8, 2024

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 logshints upstream/logshints
git merge upstream/main
git push upstream logshints

@gizas
Copy link
Contributor Author

gizas commented May 9, 2024

/test

@gizas
Copy link
Contributor Author

gizas commented May 9, 2024

@blakerouse another ping because the merging is blocked here

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@gizas gizas merged commit 62958d0 into main May 9, 2024
9 checks passed
@gizas gizas deleted the logshints branch May 9, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.