operator: Add Pod annotations with node topology labels to support zone aware scheduling #9503

shwetaap · 2023-05-23T19:43:00Z

What this PR does / why we need it:
This PR adds support to zone aware scheduling by adding zone specific node topology labels to loki component pods
Which issue(s) this PR fixes:
Fixes #LOG-3834
Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

operator/controllers/loki/lokistack_zone_labeling_controller.go

operator/go.mod

operator/internal/handlers/lokistack_podannotation.go

operator/internal/manifests/config_test.go

operator/internal/manifests/distributor.go

operator/internal/manifests/internal/config/options.go

operator/internal/manifests/var.go

operator/controllers/loki/lokistack_zone_labeling_controller.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/distributor.go

operator/controllers/loki/lokistack_zone_labeling_controller.go

JoaoBraveCoding

First pass, looking really good! Just some minor comments

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/config.go

operator/main.go

operator/internal/manifests/var.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/internal/config/options.go

operator/internal/manifests/var.go

operator/controllers/loki/lokistack_zone_labeling_controller.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/config.go

operator/internal/manifests/distributor.go

periklis · 2023-06-22T11:52:25Z

After doing some testing on this using an OpenShift cluster with worker nodes on three different AWS zones on the same AWS region, I noticed that our patching scheduled pods strategy does not always work as expected. All not so often but very likely to happen I see pods missing the annotations that the lokistack_zone_labeling_controller is supposed to enforced:

❯ kubectl get pod -l app.kubernetes.io/instance=lokistack-dev -ocustom-columns='NAME:.metadata.name,NODE:.spec.nodeName,AVAILABILITY-ZONE:.metadata.annotations.loki\.grafana\.com\/availability\-zone' --sort-by=".metadata.labels.app\.kubernetes\.io\/component"

NAME                                            NODE                                            AVAILABILITY-ZONE
lokistack-dev-distributor-695c6d77fc-m9jd5      ip-10-0-169-238.eu-central-1.compute.internal   eu-central-1_eu-central-1b
lokistack-dev-distributor-695c6d77fc-w5wmj      ip-10-0-155-166.eu-central-1.compute.internal   eu-central-1_eu-central-1a
lokistack-dev-index-gateway-0                   ip-10-0-205-76.eu-central-1.compute.internal    eu-central-1_eu-central-1c
lokistack-dev-index-gateway-1                   ip-10-0-169-238.eu-central-1.compute.internal   <none>
lokistack-dev-ingester-0                        ip-10-0-205-76.eu-central-1.compute.internal    <none>
lokistack-dev-ingester-1                        ip-10-0-169-238.eu-central-1.compute.internal   eu-central-1_eu-central-1b
lokistack-dev-querier-76f87c9d95-gz6qx          ip-10-0-155-166.eu-central-1.compute.internal   eu-central-1_eu-central-1a
lokistack-dev-querier-76f87c9d95-z85kw          ip-10-0-169-238.eu-central-1.compute.internal   eu-central-1_eu-central-1b
lokistack-dev-query-frontend-59d794c575-htgsr   ip-10-0-155-166.eu-central-1.compute.internal   eu-central-1_eu-central-1a
lokistack-dev-query-frontend-59d794c575-v6vlg   ip-10-0-169-238.eu-central-1.compute.internal   eu-central-1_eu-central-1b

For the sake of completeness, here is how my worker nodes look like after installing the cluster on AWS:

❯ kubectl get node -L topology.kubernetes.io/region -L topology.kubernetes.io/zone

NAME                                            STATUS   ROLES   AGE     VERSION           REGION         ZONE
ip-10-0-155-166.eu-central-1.compute.internal   Ready    worker  7h12m   v1.26.5+7d22122   eu-central-1   eu-central-1a
ip-10-0-169-238.eu-central-1.compute.internal   Ready    worker  7h9m    v1.26.5+7d22122   eu-central-1   eu-central-1b
ip-10-0-205-76.eu-central-1.compute.internal    Ready    worker  7h12m   v1.26.5+7d22122   eu-central-1   eu-central-1c

PTAL (cc @xperimental )

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

xperimental

I have prepared a branch containing all the changes I have commented on. Have a look.

The main changes are:

Filter Pod reconciliations by label
Move Pod changes to a single place (unfortunately needed to modify the existing code a bit, pulling out podTemplate)
Move constants for annotations and labels into the API package

Unfortunately it took a while to prepare this, I thought there was a timing issue in there when setting the annotation on the Pod, but I was not able to reproduce this today anymore.

operator/controllers/loki/lokistack_zone_labeling_controller.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/zone_aware_config.go

operator/internal/manifests/memberlist.go

operator/internal/manifests/proxy_env.go

operator/internal/manifests/var.go

operator/main.go

operator/controllers/loki/lokistack_zone_labeling_controller.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

operator/internal/manifests/config.go

JoaoBraveCoding

lgtm, just some unit test missing. Still have to re-run it on my side

operator/internal/manifests/zone_aware_config.go

operator/internal/handlers/lokistack_enable_zone_awareness.go

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

operator/internal/handlers/lokistack_enable_zone_awareness_test.go

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

periklis

Here is a branch to capture a couple of improvements for the comments below.

Using mergo.Merge for configureReplication
Add test for rendering the Loki Config with instance_availability_zone
Add Changelog entry

shwetaap/loki@Log3834...periklis:loki:zone_awareness

operator/internal/manifests/internal/config/loki-config.yaml

periklis · 2023-07-14T08:03:29Z

operator/internal/manifests/zone_aware_config.go

+	for i := range podTemplate.Spec.Containers {
+		podTemplate.Spec.Containers[i].Env = append(podTemplate.Spec.Containers[i].Env, availabilityZoneEnvVar)
+	}
+
+	podTemplate.Labels = labels.Merge(podTemplate.Labels, map[string]string{
+		lokiv1.LabelZoneAwarePod: "enabled",
+	})
+	podTemplate.Annotations[lokiv1.AnnotationAvailabilityZoneLabels] = topologyKey


I suggest to stick with mergo.Merge here as we do with the other configure patching functions.

shwetaap · 2023-07-14T12:54:48Z

/retest

operator/CHANGELOG.md

operator/internal/handlers/lokistack_enable_zone_awareness_test.go

operator/internal/manifests/config.go

operator/internal/manifests/node_placement.go

operator/internal/manifests/node_placement_test.go

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

shwetaap requested a review from a team as a code owner May 23, 2023 19:43

pull-request-size bot added the size/L label May 23, 2023

github-actions bot added the sig/operator label May 23, 2023

shwetaap force-pushed the Log3834 branch 5 times, most recently from 6ecdc1c to 5e7e16b Compare May 31, 2023 03:53

shwetaap force-pushed the Log3834 branch 5 times, most recently from f7d9dc9 to 062df3f Compare June 6, 2023 02:43

shwetaap changed the title ~~[Draft] operator: Add Pod annotations with node topology labels to support zone aware scheduling~~ operator: Add Pod annotations with node topology labels to support zone aware scheduling Jun 6, 2023

shwetaap force-pushed the Log3834 branch 2 times, most recently from 98f73e3 to 11e09b6 Compare June 6, 2023 04:38

periklis requested changes Jun 6, 2023

View reviewed changes

shwetaap force-pushed the Log3834 branch from 11e09b6 to 4397d2c Compare June 8, 2023 04:03

periklis reviewed Jun 9, 2023

View reviewed changes

operator/controllers/loki/lokistack_zone_labeling_controller.go Outdated Show resolved Hide resolved

operator/internal/handlers/lokistack_enable_zone_awareness.go Outdated Show resolved Hide resolved

shwetaap force-pushed the Log3834 branch 4 times, most recently from 4e6a70b to d2541b6 Compare June 12, 2023 19:18

periklis requested changes Jun 13, 2023

View reviewed changes

operator/internal/handlers/lokistack_enable_zone_awareness.go Outdated Show resolved Hide resolved

operator/internal/manifests/distributor.go Outdated Show resolved Hide resolved

periklis reviewed Jun 20, 2023

View reviewed changes

operator/controllers/loki/lokistack_zone_labeling_controller.go Outdated Show resolved Hide resolved

operator/controllers/loki/lokistack_zone_labeling_controller.go Outdated Show resolved Hide resolved

JoaoBraveCoding reviewed Jun 20, 2023

View reviewed changes

periklis requested changes Jun 21, 2023

View reviewed changes

operator/internal/handlers/lokistack_enable_zone_awareness.go Outdated Show resolved Hide resolved

xperimental reviewed Jun 21, 2023

View reviewed changes

Zone aware:Add Pod annotations with node topology labels

b554608

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

xperimental added 2 commits July 3, 2023 21:52

Use constant value in configuration file

8d7c7dd

Refactor pod definition code

8c49863

xperimental reviewed Jul 3, 2023

View reviewed changes

Fix lint issues

84f529a

xperimental reviewed Jul 7, 2023

View reviewed changes

JoaoBraveCoding reviewed Jul 7, 2023

View reviewed changes

operator/internal/manifests/zone_aware_config.go Outdated Show resolved Hide resolved

shwetaap commented Jul 9, 2023

View reviewed changes

operator/internal/handlers/lokistack_enable_zone_awareness.go Show resolved Hide resolved

Fix minor issues and add unit tests

b5ad7fd

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

xperimental reviewed Jul 12, 2023

View reviewed changes

Fix the unit tests for Zone Aware Handler

616cee7

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

periklis mentioned this pull request Jul 13, 2023

operator: Use PodAntiAffinity for all components #9930

Merged

7 tasks

shwetaap and others added 6 commits July 13, 2023 18:32

Refactor code and add further tests

095a3ab

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

Merge remote-tracking branch 'upstream/main' into Log3834

631cd04

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

Rename zone_aware_config.go to node_placement.go

d6ae1ba

Use mergo.Merge for configureReplication

a9200ff

Add missing Loki config render test for instance_availability_zone

9066111

Add CHANGELOG entry

b938183

periklis reviewed Jul 14, 2023

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Jul 14, 2023

Merge remote-tracking branch 'upstream/main' into Log3834

3e04e1e

periklis approved these changes Jul 14, 2023

View reviewed changes

xperimental reviewed Jul 14, 2023

View reviewed changes

Addiitonal fixes from code review

0abe6c9

Signed-off-by: Shweta Padubidri <spadubid@redhat.com>

xperimental approved these changes Jul 14, 2023

View reviewed changes

periklis merged commit 5bd8c7d into grafana:main Jul 14, 2023

shwetaap mentioned this pull request Jul 14, 2023

operator: Gateway pods do not need to have INSTANCE_AVAILABILITY_ZONE env set in their containers #9950

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: Add Pod annotations with node topology labels to support zone aware scheduling #9503

operator: Add Pod annotations with node topology labels to support zone aware scheduling #9503

shwetaap commented May 23, 2023 •

edited by periklis

Loading

JoaoBraveCoding left a comment

periklis commented Jun 22, 2023 •

edited

Loading

xperimental left a comment

JoaoBraveCoding left a comment

periklis left a comment

periklis Jul 14, 2023

shwetaap commented Jul 14, 2023

operator: Add Pod annotations with node topology labels to support zone aware scheduling #9503

operator: Add Pod annotations with node topology labels to support zone aware scheduling #9503

Conversation

shwetaap commented May 23, 2023 • edited by periklis Loading

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

periklis commented Jun 22, 2023 • edited Loading

xperimental left a comment

Choose a reason for hiding this comment

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

periklis left a comment

Choose a reason for hiding this comment

periklis Jul 14, 2023

Choose a reason for hiding this comment

shwetaap commented Jul 14, 2023

shwetaap commented May 23, 2023 •

edited by periklis

Loading

periklis commented Jun 22, 2023 •

edited

Loading