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

Allow predicates to be disabled on a case-by-case basis via annotation. #5284

Merged
merged 13 commits into from
Feb 9, 2022

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Jan 25, 2022

Closes #2092

This change will allow ECK user to disable certain upgrade predicates, by name, via an annotation on the Elasticsearch CRD, thus allowing clusters to be "forced" to be upgraded in scenarios where the default predicates (that disallow upgrade because of a set of rules) are too restrictive.

Use Case 1:

Cluster is a "red" state because of an un-assignable shard. This cluster would never be allowed to be upgraded with the standard set of upgrade predicates in place.

If the following annotation was added to the cluster

apiVersion: elasticsearch.k8s.elastic.co/v1
kind: Elasticsearch
metadata:
  name: testing
  annotations:
    eck.k8s.elastic.co/disable-upgrade-predicates: "only_restart_healthy_node_if_green_or_yellow"
spec:
  version: 7.15.1

And the version was bumped to 7.15.2, then this cluster's nodes would be allowed to be upgraded in this scenario, even with the cluster in a "red" state.

Use Case 2:

A test Elasticsearch cluster has been setup, and regardless of any state of the cluster, a user wants to force an upgrade.

With the following annotation, and a version bump to 7.15.2, all predicates will be disabled, allowing an upgrade to proceed regardless of the state of the cluster:

apiVersion: elasticsearch.k8s.elastic.co/v1
kind: Elasticsearch
metadata:
  name: testing
  annotations:
    eck.k8s.elastic.co/disable-upgrade-predicates: "*"
spec:
  version: 7.15.1

Discussion Points

  • Do we want to allow anything other than disabling one predicate, or disabling all predicates? Such as disabling 2 or more predicates specifically? The way this is designed at the moment, this wouldn't be allowed.. This limit was assumed, as there appeared to be a 256 character limit on the values of annotations, but since kubectl.kubernetes.io/last-applied-configuration clearly uses more characters than that, this assumption was wrong. A comma separated list should suffice here for > 1 predicate. I'll get this change into this PR.

Todo

  • Write documentation
  • Add e2e test(s)

Allow all predicates to be disabled via annotation.
Tests for many upgrade scenarios.
@naemono naemono added the >feature Adds or discusses adding a feature to the product label Jan 25, 2022
@naemono naemono changed the title Allow predicates to disabled on a case-by-case basis via annotation. Allow predicates to be disabled on a case-by-case basis via annotation. Jan 25, 2022
@naemono
Copy link
Contributor Author

naemono commented Jan 26, 2022

run full pr build

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

A comma separated list should suffice here for > 1 predicate. I'll get this change into this PR.

👍 I think it would be great to be able to disable more than one replica.

@barkbay barkbay added the v2.1.0 label Jan 31, 2022
Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

I left a few comments but LGTM.
Since we have some documentation here I would maybe request a review from @alaudazzi

Moving annotation constant to be next to others.
Adjusting tense of documentation.
Caching logic prior to loop instead of calling same method multiple times.
Adjusting e2e test to be more simple for testing predicate disabling.
@naemono naemono requested a review from alaudazzi February 7, 2022 20:24
@naemono
Copy link
Contributor Author

naemono commented Feb 7, 2022

will merge after review by @alaudazzi, as suggested.

Copy link
Contributor

@alaudazzi alaudazzi left a comment

Choose a reason for hiding this comment

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

Left a few suggestions in the Asciidoc file, otherwise LGTM.

naemono and others added 4 commits February 8, 2022 08:02
…ions/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>
…sticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>
…ications/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>
…elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>
@naemono naemono merged commit ac4dccb into elastic:main Feb 9, 2022
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
…n. (elastic#5284)

* Allow predicates to disabled on a case-by-case basis via annotation.
Allow all predicates to be disabled via annotation.
Tests for many upgrade scenarios.

* Adding documentation for the feature.

* Add e2e test allowing cluster upgrade while cluster is red by disabling predicate.

* fix annotation spelling

Co-authored-by: Michael Morello <[email protected]>

* better logging of disabling predicate

Co-authored-by: Michael Morello <[email protected]>

* Move disabled predicate logic to pkg/apis
Update documentation per review comments

* Add a predicate check to the CI checks per the review comments.

* Adjusting warning in documentation around disabling predicates.
Moving annotation constant to be next to others.
Adjusting tense of documentation.
Caching logic prior to loop instead of calling same method multiple times.
Adjusting e2e test to be more simple for testing predicate disabling.

* remove unused annotations var

* Adjust predicate warning in docs/orchestrating-elastic-stack-applications/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>

* lowercase yellow in docs/orchestrating-elastic-stack-applications/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>

* Adjust predicate explanation in docs/orchestrating-elastic-stack-applications/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>

* Correct misspelling in docs/orchestrating-elastic-stack-applications/elasticsearch/orchestration.asciidoc

Co-authored-by: Arianna Laudazzi <[email protected]>

Co-authored-by: Michael Morello <[email protected]>
Co-authored-by: Arianna Laudazzi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature Adds or discusses adding a feature to the product v2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow unsafe rolling upgrades if specified by the user
4 participants