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

[add_cloud_metadata] fix the orchestrator metadata for the aws cloud provider #37651

Conversation

tetianakravchenko
Copy link
Contributor

@tetianakravchenko tetianakravchenko commented Jan 16, 2024

Proposed commit message

  • WHAT: orchestrator object must be placed on the top level, instead of under the cloud
  • WHY: these fields are available under the cloud object now, which is not correct because
  1. those fields are needed for the kubernetes environment and we rely on them for k8s dashboards - https://github.com/elastic/beats/blob/main/metricbeat/module/kubernetes/_meta/docs.asciidoc#dashboard
  2. it does not align with the ecs schema - https://www.elastic.co/guide/en/ecs/current/ecs-orchestrator.html#field-orchestrator-cluster-id and https://www.elastic.co/guide/en/ecs/current/ecs-orchestrator.html#field-orchestrator-cluster-name

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
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

  1. Switch to the EKS cluster
  2. cd dev-tools/kubernetes and modify the Tiltfile:
beat(beat="metricbeat",
     mode="run",
     arch="amd64",
     k8s_env="aws",
     k8s_cluster="single",
     )

In order to make configuration above working:
3.1. mode="run" - dev-tools/kubernetes/metricbeat/manifest.run.yaml:
modify this:

processors:
      - add_cloud_metadata:

to this:

  processors:
      - add_cloud_metadata:
          providers:
            - "aws"

Also because it will be running in cloud - change the ES credentials to use elastic cloud
3.2. k8s_env="aws"
run
aws ecr get-login-password --region us-east-2 | docker login --username AWS --password-stdin XXXXX.dkr.ecr.us-east-2.amazonaws.com/metricbeat-debug
and edit docker_registry accordingly:

if k8s_env == "aws":
        # In order to push to AWS you need to run:
        #   aws ecr get-login-password --region us-east-2 | docker login --username AWS --password-stdin XXXXX.dkr.ecr.us-east-2.amazonaws.com/metricbeat-debug
        #
        # More info at https://docs.aws.amazon.com/AmazonECR/latest/userguide/docker-push-ecr-image.html
        docker_registry = "XXXXX.dkr.ecr.us-east-2.amazonaws.com".format(. # <- replace!
            docker_image)

3.3. for simplicity I use the cluster with 1 node - k8s_cluster="single"
4. run tilt up

Related issues

Use cases

Screenshots

Screenshot 2024-01-23 at 18 05 13

Logs

@tetianakravchenko tetianakravchenko requested a review from a team as a code owner January 16, 2024 19:26
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 16, 2024
@botelastic
Copy link

botelastic bot commented Jan 16, 2024

This pull request doesn't have a Team:<team> label.

Copy link
Contributor

mergify bot commented Jan 16, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tetianakravchenko? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

@elasticmachine
Copy link
Collaborator

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 139 min 5 sec

Pipeline error 1

This error is likely related to the pipeline itself. Click here
and then you will see the error (either incorrect syntax or an invalid configuration).

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@gizas
Copy link
Contributor

gizas commented Jan 17, 2024

@tetianakravchenko can you add online description about the fix and what is doing?
Could you also update the sections How to test this PR locally and Screenshots for proof of the fix? It will be really helpful even for people outside the team to review this

@tetianakravchenko
Copy link
Contributor Author

@tetianakravchenko can you add online description about the fix and what is doing?
Could you also update the sections How to test this PR locally and Screenshots for proof of the fix? It will be really helpful even for people outside the team to review this

@gizas done, please check issue description

@@ -198,6 +198,12 @@ func TestRetrieveAWSMetadataEC2(t *testing.T) {
"cloud": mapstr.M{
"instance": mapstr.M{"id": instanceIDDoc2},
},
"orchestrator": mapstr.M{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this test here

based on documentation:

The third optional configuration setting is `overwrite`. When overwrite is true, add_cloud_metadata overwrites existing `cloud.*` fields (false by default).

now when the orchestrator is not part of the cloud object, this will not be overwritten.
Any thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for delaying to test this Tania. I think that this page strictly speaking does not refer to orchestrator at all.

I think we can leave it for now or of you want to add a note in the doc

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 133 min 46 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 25, 2024

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 130 min 51 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@gizas
Copy link
Contributor

gizas commented Jan 26, 2024

The integrations tests run successfully and I also see correct values in my local debugging:

Screenshot 2024-01-26 at 4 58 21 PM

(See above that in metadata both cloud and orchestrator return)
Only issue that I have is still to run an e2e test because I can not push to my remote ECR. FYI:

❯ aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 627286350134.dkr.ecr.us-east-1.amazonaws.com/gizas
Login Succeeded

I change my tiltfile and I still see:

STEP 4/5 — Pushing 627286350134.dkr.ecr.us-east-1.amazonaws.com/gizas/metricbeat-run-image:tilt-88d998cc7274ce04
     Pushing with Docker client
     Authenticating to image repo: 627286350134.dkr.ecr.us-east-1.amazonaws.com
     Sending image data
     2b0e5c46e9bc: Retrying in 1 second 
     1d8c74287bd8: Retrying in 1 second 
     ed7b7f3aa65e: Retrying in 1 second 
     2040408c8c93: Retrying in 1 second 
     4c1c7a723dc2: Retrying in 1 second 

Build Failed: docker push: pushing image "627286350134.dkr.ecr.us-east-1.amazonaws.com/gizas/metricbeat-run-image": EOF

Will keep troubleshooting. Do I miss sth in the test steps @tetianakravchenko ?

@tetianakravchenko
Copy link
Contributor Author

@gizas
Copy link
Contributor

gizas commented Jan 31, 2024

@tetianakravchenko all tests are succeful on myside

Screenshot 2024-01-31 at 10 13 20 AM

(I had to create the ecr 627286350134.dkr.ecr.us-east-1.amazonaws.com/gizas/metricbeat-run-image to be able to push the metricbeat-run-image inside it. It might be a little confusing sometimes. Just mentioning)

Copy link
Contributor

mergify bot commented Feb 5, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tetianakravchenko? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @tetianakravchenko

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

LGTM

@tetianakravchenko tetianakravchenko added the backport-v8.11.0 Automated backport with mergify label Feb 12, 2024
@tetianakravchenko tetianakravchenko added the backport-v8.12.0 Automated backport with mergify label Feb 12, 2024
@tetianakravchenko tetianakravchenko merged commit 435e729 into elastic:main Feb 12, 2024
106 checks passed
mergify bot pushed a commit that referenced this pull request Feb 12, 2024
…provider (#37651)

* add_cloud_metadata: fix the orchestrator metadata for the aws cloud provider

Signed-off-by: Tetiana Kravchenko <[email protected]>

* rename the map name: cloud -> meta; fix tests

Signed-off-by: Tetiana Kravchenko <[email protected]>

---------

Signed-off-by: Tetiana Kravchenko <[email protected]>
(cherry picked from commit 435e729)
mergify bot pushed a commit that referenced this pull request Feb 12, 2024
…provider (#37651)

* add_cloud_metadata: fix the orchestrator metadata for the aws cloud provider

Signed-off-by: Tetiana Kravchenko <[email protected]>

* rename the map name: cloud -> meta; fix tests

Signed-off-by: Tetiana Kravchenko <[email protected]>

---------

Signed-off-by: Tetiana Kravchenko <[email protected]>
(cherry picked from commit 435e729)
@tetianakravchenko tetianakravchenko added the backport-v8.13.0 Automated backport with mergify label Feb 14, 2024
tetianakravchenko added a commit that referenced this pull request Feb 14, 2024
…provider (#37651) (#37976)

* add_cloud_metadata: fix the orchestrator metadata for the aws cloud provider

Signed-off-by: Tetiana Kravchenko <[email protected]>

* rename the map name: cloud -> meta; fix tests

Signed-off-by: Tetiana Kravchenko <[email protected]>

---------

Signed-off-by: Tetiana Kravchenko <[email protected]>
(cherry picked from commit 435e729)

Co-authored-by: Tetiana Kravchenko <[email protected]>
@tetianakravchenko
Copy link
Contributor Author

@Mergifyio backport 8.13

Copy link
Contributor

mergify bot commented Feb 14, 2024

backport 8.13

❌ No backport have been created

  • Backport to branch 8.13 failed

GitHub error: Branch not found

@sang-elastic
Copy link

Will this also fix the Azure cloud provider issue as well ? Thanks in advance.

@tetianakravchenko
Copy link
Contributor Author

@sang-elastic

Will this also fix the Azure cloud provider issue as well ? Thanks in advance.

not sure what issue do you mean. But this suppose to impact only the AWS provider.

FYI: there is this PR open - #37685 to add AKS cluster name and id

@sang-elastic
Copy link

@tetianakravchenko. Thanks for your inputs.
@kaiyan-sheng was looking at (https://github.com/elastic/sdh-beats/issues/4390) issue and linked to the case to this.

It think it seems like similar issue with meta data but this one is on AZURE provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.11.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify backport-v8.13.0 Automated backport with mergify bug needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
6 participants