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

feat: Updated SS and microservices deployment docs #13083

Merged
merged 16 commits into from
Jun 5, 2024
Merged

Conversation

Jayclifford345
Copy link
Contributor

This PR updates our helm documentation inline with our 3.0 deployment for:

  • Simple Scalable
  • Microservices

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label May 30, 2024
Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

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

[docs team]

- **IndexGateway component** (2 replicas, maxUnavailable: 1): Handles indexing. Up to 1 replica can be unavailable during updates.


<!--TODO - Update when meta-monitoring chart releases-->
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - It has been released, just not publicized yet.

https://github.com/grafana/meta-monitoring-chart/releases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really cool!! I noticed we had a monitoring and alerting section in deployments. I have linked to that for now. I can create a new PR with updated instructions for that section of the docs with the repo instructions


## Prerequisites

To run through this tutorial, you need the following tools installed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly sure if we need this topic, since we don't have a "testing" topic for the other installation methods. I would assume that customers would test in a test environment before installing on a production environment. Have you seen different behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I have changed the name slightly to help. Essentially I wanted to provide our community with an easy to step by step guide on how to practise deploying this locally just so they can understand how the helm deploys without needing to set up a full cloud k8's stack. It mostly helped me as I was testing the new deployment helms that the team created :). But happy to remove if its overkill as might also be confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

After you explained in Ward's meeting today, I understand what you're doing here. Let's keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL, just saw Trevor's comment and that you'd pulled it. That's OK too.

docs/sources/setup/install/helm/install-scalable/_index.md Outdated Show resolved Hide resolved
@@ -91,5 +134,77 @@ It is not recommended to run scalable mode with `filesystem` storage.
helm upgrade --values values.yaml loki grafana/loki
```

## AWS S3 Configuration

To configure Loki to use AWS S3 as the object storage, you need to provide the following values in the `values.yaml` file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not combine this with the example above? Curious as to why it's called out separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was going to ask you about this one. I currently have the first example using MinIO for test purposes and then provide the cloud storage configurations after. I have added the S3 one for now but currently testing Azure blog to also add next

@Jayclifford345 Jayclifford345 marked this pull request as ready for review June 3, 2024 10:14
@Jayclifford345 Jayclifford345 requested a review from a team as a code owner June 3, 2024 10:14
@Jayclifford345
Copy link
Contributor Author

Marking this as ready for review as we are getting quite a few questions in the community over helm deployment. Will create subsequent PR's with other further additions.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

this looks good, but I don't think we should include the instructions for deploying with kind. I think that will just lead to questions about kind we are not experts in. There's a lot of ways to spin up a local k8s cluster, I think it's reasonable to expect someone familiar with helm to bring their own k8s cluster.

@Jayclifford345
Copy link
Contributor Author

@JStickler @trevorwhitney , that's the kind deployment instructions deleted. Let me know if there are any other parts you think might need some rework

@grafanabot
Copy link
Collaborator

This PR must be merged before a backport PR will be created.

@JStickler JStickler merged commit 1b80458 into main Jun 5, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-3.0.x size/XL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants