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

Update kubernetes provider documentation and 8.6 release notes #2552

Conversation

MichaelKatsoulis
Copy link
Contributor

@MichaelKatsoulis MichaelKatsoulis commented Jan 26, 2023

Update kubernetes provider documentation to clearly state the correct usage of kubernetes labels in autodiscover conditions and 8.6 release notes with a breaking change.

This is due to changes done in elastic/elastic-agent#1398 effective in 8.6 release onwards.

Release notes have been updated, because even if this is considered a bug fix, there may be clients that have already set some templates in standalone agent or conditions in packages (the few that support it).
If in those conditions they make use of dedoted labels then when upgrading to agent 8.6, the data sets affected won't collect logs/metrics as the conditions won't match.
They need to manually update their configuration.

Updates of this PR can be found in #2552 (comment) Fleet and Elastic Agent Guide in three pages.

  1. Release notes
  2. Configure standalone Elastic Agents --> Providers --> Kubernetes Provider
  3. Configure standalone Elastic Agents --> Kubernetes autodiscovery with Elastic Agent --> Conditions based autodiscover

…usage of kubernetes labels in autodiscover conditions
@MichaelKatsoulis MichaelKatsoulis marked this pull request as draft January 26, 2023 14:24
@github-actions
Copy link
Contributor

A documentation preview will be available soon:

@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2023

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

  • backport-/d./d is the label to automatically backport to the /d./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 26, 2023
@MichaelKatsoulis MichaelKatsoulis added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Jan 26, 2023
@MichaelKatsoulis MichaelKatsoulis marked this pull request as ready for review January 26, 2023 14:57
@@ -84,6 +86,13 @@ To set the target host dynamically for a targeted Pod based on its labels, use a
condition: ${kubernetes.labels.component} == 'kube-scheduler'
----

NOTE: Pods labels and annotations can be used in autodiscover conditions. In the case of labels or annotations that include dots(`.`), they can be used in conditions exactly as
they are defined in the pods. For example `condition: ${kubernetes.labels.app.kubernetes.io/name} == 'ingress-nginx'`. This should not be mistaken with optional dedoted labels and annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think can we enhance the examples here?
We like users to understand what is the actual label and what is the condition.

Also an example with annotation?

I think we should so a condition example before and after 8.6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, there is already an example with the usage of labels in conditions. It is mentioned above that The condition ${kubernetes.labels.app} == 'redis' will make the Elastic Agent look for a Pod with the label app:redis within the scope defined in its manifest.
The more complex labels that are not dedoted any more is not a new case. It is just another label to be used. I believe that this note explains it.
@ChrsMark what do you think ?

Copy link
Member

@ChrsMark ChrsMark Jan 27, 2023

Choose a reason for hiding this comment

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

From my perspective this paragraph achieves the goal to explain how labels should be used in conditions specially regarding the dedoting concpet. Maybe we can add one extra example here for an annotation or provide a bunch of examples in a list like:

1. To target a Pod labeled with `app: apache` use the condition X
2. To target a Pod with the annotation .... use ....

It's up to you to decide how detailed you want to make it.

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 can add some examples of conditions with complex labels, annotations and host.ip without having to create detailed examples that do not offer a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM now

@@ -84,6 +86,13 @@ To set the target host dynamically for a targeted Pod based on its labels, use a
condition: ${kubernetes.labels.component} == 'kube-scheduler'
----

NOTE: Pods labels and annotations can be used in autodiscover conditions. In the case of labels or annotations that include dots(`.`), they can be used in conditions exactly as
they are defined in the pods. For example `condition: ${kubernetes.labels.app.kubernetes.io/name} == 'ingress-nginx'`. This should not be mistaken with optional dedoted labels and annotations
Copy link
Contributor

@gizas gizas Jan 26, 2023

Choose a reason for hiding this comment

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

This should not be mistaken with optional dedoted labels and annotations stored into Elasticsearch([kubernetes-provider]).

This confused me more here. Do you think shall we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which line confused you?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above updated.

I mean do users need this link there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean this link Elasticsearch(<<kubernetes-provider>>). I have added that as it is the page that includes all the info regarding label and annotations dedoting

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Overall looks good. I've left some suggestions, feel free to include as many of them you find useful.

MichaelKatsoulis and others added 7 commits January 27, 2023 10:12
…overy/kubernetes-conditions-autodiscover.asciidoc

Co-authored-by: Chris Mark <[email protected]>
…overy/kubernetes-conditions-autodiscover.asciidoc

Co-authored-by: Chris Mark <[email protected]>
…overy/kubernetes-conditions-autodiscover.asciidoc

Co-authored-by: Chris Mark <[email protected]>
…overy/kubernetes-conditions-autodiscover.asciidoc

Co-authored-by: Chris Mark <[email protected]>
…s/kubernetes-provider.asciidoc

Co-authored-by: Chris Mark <[email protected]>
…s/kubernetes-provider.asciidoc

Co-authored-by: Chris Mark <[email protected]>
@@ -189,6 +189,9 @@ The available keys are:


These are the fields available within config templating. The `kubernetes.*` fields will be available on each emitted event.
`kubernetes.labels.*` and `kubernetes.annotations.*` used in config templating are not dedoted and should not be confused with
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right. I will highlight with NOTE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -67,7 +71,9 @@ You should now be able to see Redis data flowing in on index `metrics-redis.info

NOTE: All assets (dashboards, ingest pipelines, and so on) related to the Redis integration are not installed. You need to explicitly <<install-uninstall-integration-assets,install them through {kib}>>.

To set the target host dynamically for a targeted Pod based on its labels, use a variable in the {agent} policy to return path information from the provider:
Conditions can also be used in the `Kubernetes` package in order to set the target host dynamically for a targeted Pod based on its labels.
Copy link
Contributor

Choose a reason for hiding this comment

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

by Kubernetes package are you referring to the https://docs.elastic.co/en/integrations/kubernetes ?

for standalone we usually use in documentation input instead of package, for example - here or here

can we keep it consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input can have whatever name the user wants so kubernetes input would not be accurate wording. Also in each input we have the package information inside the meta. And although not used anywhere, the name of the package is the same used in integrations.
Also I initially had written input. But I changed it after @ChrsMark comment (#2552 (comment)).
Chris are your thoughts the same as mine in this?

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for clarifications, I don't have a strong opinion on it

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion either :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok as long you don't have a strong opinion I decided to use input instead of package to keep consistency because I am not sure if the user at this point is 100% familiar with packages.

@@ -37,6 +37,10 @@ To automatically identify a Redis Pod and monitor it with the Redis integration,
The condition `${kubernetes.labels.app} == 'redis'` will make the {agent} look for a Pod with the label `app:redis` within the scope defined in its manifest.

For a list of provider fields that you can use in conditions, refer to <<kubernetes-provider>>.
Some examples of conditions usage are:
1. For a pod with label `app.kubernetes.io/name=ingress-nginx` the condition should be `condition: ${kubernetes.labels.app.kubernetes.io/name} == "ingress-nginx"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@MichaelKatsoulis MichaelKatsoulis merged commit 829c303 into elastic:main Jan 31, 2023
@MichaelKatsoulis MichaelKatsoulis self-assigned this Jan 31, 2023
bmorelli25 pushed a commit to bmorelli25/observability-docs that referenced this pull request Apr 11, 2023
…ic#2552)

* Update  kubernetes provder documentation to clearly state the correct usage of kubernetes labels in autodiscover conditions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants