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

Chore: refactoring jsonnet to explicitly include the deployment mode in the config #3475

Merged
merged 3 commits into from Nov 22, 2022

Conversation

pracucci
Copy link
Collaborator

What this PR does

#3379 introduced read-write deployment mode support in jsonnet. When enabled, the read-write deployment mode jsonnet "uninstall" all microservices K8S resources (e.g. ingester statefulset). This was a trick introduced to not touch the microservices deployment at all in a initial stage.

In this PR I'm proposing a refactoring to introduce deployment_mode in the jsonnet config (defaults to microservices) and then use it when defining K8S resources (e.g. do not define ingester_statefulset at all if deployment_mode != 'microservices').

This PR is expected to be a no-op with regards to all jsonnet tests.

Which issue(s) this PR fixes or relates to

Fixes #3364

Checklist

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

…in the config

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

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

Not a jsonnet expert, but LGTM

@pracucci
Copy link
Collaborator Author

I will keep this PR open for at least 1 business day to see if anyone has any objection.

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

LGTM.

One small nit is that I feel comparing to a string value can be a little bit fragile.

We could consider having $._config.is_microservices_deployment_mode and $._config.is_read_write_deployment_mode variables and referencing those, making the compiler enforce the correctness, and have a validation in the config to ensure that exactly one is set.

… mode is enabled

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci
Copy link
Collaborator Author

LGTM.

One small nit is that I feel comparing to a string value can be a little bit fragile.

We could consider having $._config.is_microservices_deployment_mode and $._config.is_read_write_deployment_mode variables and referencing those, making the compiler enforce the correctness, and have a validation in the config to ensure that exactly one is set.

@colega Good feedback. WDYT about d3ed143 ?

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM, one small suggestion.

operations/mimir/config.libsonnet Outdated Show resolved Hide resolved
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci merged commit ca8ce6d into main Nov 22, 2022
@pracucci pracucci deleted the jsonnet-add-deployment-mode-config-option branch November 22, 2022 08:28
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
…in the config (grafana#3475)

* Chore: refactoring jsonnet to explicitly include the deployment mode in the config

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

* Add explicit booleans (checked by compiler) to check which deployment mode is enabled

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

* Rephrase error message to clarify what config option should be used

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.

Jsonnet: upstream read-write deployment mode support
4 participants