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 K8S node labels to be propagated as Pod annotations #5054

Merged
merged 7 commits into from Nov 30, 2021

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Nov 18, 2021

Fixes #3933

How to test (tested on GKE)

  • Start the operator with the following flag: --exposed-node-labels="topology.kubernetes.io/.*,failure-domain.beta.kubernetes.io/.*"
  • Annotate an Elasticsearch deployment: k annotate --overwrite es/elasticsearch-sample eck.k8s.elastic.co/downward-node-labels="topology.kubernetes.io/zone,topology.kubernetes.io/region"
  • Wait for the Pods to be restarted
  • Check the annotations: k get pod -l common.k8s.elastic.co/type=elasticsearch -o go-template='{{range .items}}{{.metadata.annotations}}{{printf "\n"}}{{end}}'

To get the logs of the init script you can run k logs elasticsearch-sample-es-default-0 -c elastic-internal-init-filesystem -f

Misc

  • I eventually decided to reuse the elastic-internal-init-filesystem init container to wait for the annotations to be set.
  • This PR should not restart the current RUNNING Pods. The elastic-internal-init-filesystem init-container should be left untouched as long as the feature is not used on an Elasticsearch resource.
  • Pods are patched early in the reconciliation loop. I'm not sure if there's an ideal place (or a place we should avoid)
  • A Pod is considered as scheduled when both its Spec.NodeName has a non-empty value and the relevant condition PodScheduled is set to True (see K8S binding code here)
  • The primary intent for this PR is to propagate topology node labels, hence node labels are considered as immutable and K8S Nodes are not watched.
  • When an Elasticsearch resource is submitted the admission controller only check that the node label is allowed. Node labels are not checked until the Pod is scheduled. This is mostly because node labels may be inconsistent.
  • I'll try to think about some e2e tests in a subsequent PR.

To be addressed in follow up PRs

@barkbay barkbay added >feature Adds or discusses adding a feature to the product v2.0.0 labels Nov 18, 2021
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Looks really good. 👍 I did a few simple tests and it worked as advertised. I have only a few small comments nothing major. Naming suggestions are totally optional of course.

pkg/apis/elasticsearch/v1/elasticsearch_types.go Outdated Show resolved Hide resolved
pkg/apis/elasticsearch/v1/elasticsearch_types.go Outdated Show resolved Hide resolved
pkg/apis/elasticsearch/v1/elasticsearch_types.go Outdated Show resolved Hide resolved
pkg/controller/common/operator/parameters.go Outdated Show resolved Hide resolved
pkg/controller/elasticsearch/nodespec/podspec.go Outdated Show resolved Hide resolved
@sebgl
Copy link
Contributor

sebgl commented Nov 22, 2021

(applies to anything:) how do we decide whether a feature is specified as an annotation or as a field in the spec?
We went the annotation way here but I'm not sure about the reasoning. There is an "alpha" feel to the annotation, but I suspect we'd still have to commit to maintaining this working over time anyway.

From a user pov, I'm wondering if i'd rather want to do something like this, much higher-level:

spec:
  allocationAwareness:
    enable: true # defaults to false
    attribute: "zone" # default
    nodeLabel: "topology.kubernetes.io/zone" # default

and have the operator automatically handle the Elasticsearch configuration part + the downward API + the environment variable propagation?

@barkbay
Copy link
Contributor Author

barkbay commented Nov 22, 2021

I suspect we'd still have to commit to maintaining this working over time anyway.

I think one of the reason is that I have the feeling to implement something which might be part of K8S at some point. I'm still hoping for an "official" solution. It feels then easier to deprecate and remove an annotation than a field in the spec (assuming we don't want to maintain a feature which is natively available in K8S).
It also allows us to get some feedback early, and adjust the feature if needed. This PR might then be a building block for a more advanced feature as the one outlined in your message.
Happy to reconsider if we think it deserves an update of the Elasticsearch schema.

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM, I think this will be really useful 🚀

pkg/controller/elasticsearch/driver/driver.go Show resolved Hide resolved
@barkbay barkbay merged commit 11e776d into elastic:master Nov 30, 2021
@barkbay barkbay deleted the feature/zone-awareness-main branch November 30, 2021 06:52
@pebrc pebrc changed the title Allow K8S node labels to be propagated as pod annotations Allow K8S node labels to be propagated as Pod annotations Jan 25, 2022
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
* Allow K8S node labels to be set as pod annotations

* Reuse elasticsearch.k8s.elastic.co/config-hash
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.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify zone awareness configuration
3 participants