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

Jsonnet: add support for read-write deployment mode #3379

Merged
merged 5 commits into from Nov 4, 2022

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Nov 3, 2022

What this PR does

In this PR I'm upstreaming the read-write deployment mode jsonnet setup we're using at Grafana Labs. To make the
jsonnet overrides correctly work for the overrides-exporter, I've changed jsonnet to always import overrides-exporter.libsonnet and conditionally enable it using a new configuration option (this approach follows how other optional components are enabled too, so it's also more consistent with the rest of jsonnet).

Which issue(s) this PR fixes or relates to

Part of #3364

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci requested a review from a team as a code owner November 3, 2022 13:43
@pracucci pracucci marked this pull request as draft November 3, 2022 13:50
…evert it when read-write deployment mode is enabled

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review November 3, 2022 14:21
CHANGELOG.md Outdated Show resolved Hide resolved

storage_backend: 's3',
blocks_storage_bucket_name: 'blocks-bucket',
bucket_index_enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this is the default value, I'd skip it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. Will do in a separate PR, cause affects other tests too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See: #3388

storage_backend: 's3',
blocks_storage_bucket_name: 'blocks-bucket',
bucket_index_enabled: true,
query_scheduler_enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, this is the default value, I'd also skip it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. Will do in a separate PR, cause affects other tests too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See: #3388

Signed-off-by: Marco Pracucci <marco@pracucci.com>
check_memberlist_ring: if !$._config.read_write_deployment_enabled || $._config.memberlist_ring_enabled then null else
error 'please set memberlist_ring_enabled to true when using Mimir read-write deployment mode',

// Uninstall resources used by microservices deployment mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess some day we'll need to either include one or another, instead of uninstalling them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Is this significantly different enough that it warrants a whole new jsonnet project? eg operations/mimir-read-write-deployment? Especially since switching is a destructive operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't want separate jsonnet projects. Two projects means doing changes in 2 places every time and it's harder to keep consistency between the two. However I agree with Oleg that a follow up work should be creating the microservices exported resources (deployments, statefulsets, ...) conditionally instead of uninstalling them. I will add as a TODO to my work.

// Creates a headless service for the per-zone mimir-backends StatefulSet. We don't use it
// but we need to create it anyway because it's responsible for the network identity of
// the StatefulSet pods. For more information, see:
// https://v1-18.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#statefulset-v1-apps
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this link doesn't seem to work anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@jhesketh
Copy link
Contributor

jhesketh commented Nov 4, 2022

The more I look at this the more I think it should be its own jsonnet lib. Thoughts?

@colega
Copy link
Contributor

colega commented Nov 4, 2022

I think there will be too much overlap among both (how things are configured,which are the valid options, etc., it's all the same), I'd keep them in the same package.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci enabled auto-merge (squash) November 4, 2022 08:48
@pracucci pracucci merged commit c934456 into main Nov 4, 2022
@pracucci pracucci deleted the upstream-read-write-deployment-mode-jsonnet branch November 4, 2022 09:03
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
* Jsonnet: add support for read-write deployment mode

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Always import overrides-exporter.libsonnet so that we can correctly revert it when read-write deployment mode is enabled

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Disable overrides-exporter when read-write deployment mode is enabled

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Clarified the CHANGELOG entry

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Fixed K8S reference link

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants