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

Fix service-mirror template when running in HA mode #11609

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

mateiidavid
Copy link
Member

@mateiidavid mateiidavid commented Nov 13, 2023

Two clusters can be linked in HA mode. When HA values are used, the service-mirror deployment receives some pod affinity rules to ensure fair scheduling of pods across a cluster's nodes.

The service-mirror Deployment's template seems to be broken at the moment when using HA values. Affinity rules are incorrectly grouped under a top-level podAntiAffinity field. The Kubernetes API requires the rules to be grouped under a top-level affinity field. This change rectifies that by introducing the missing parent.

Fixes #11603


Tests

  • The issue was introduced since we avoided including the entire linkerd.affinity subchart (i.e. partial). Instead of bringing in parts of the subchart, we are now copying the install values and including the entire partial template. Based on the values, it should populate the affinity rules.
  • I encourage reviewers to do their own testing.

Before:

:; linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443" --ha | k apply -f -
secret/cluster-credentials-target created
secret/cluster-credentials-target created
link.multicluster.linkerd.io/target created
clusterrole.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target created
role.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target created
rolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target created
serviceaccount/linkerd-service-mirror-target created
error: error validating "STDIN": error validating data: ValidationError(Deployment.spec.template.spec): unknown field "podAntiAffinity" in io.k8s.api.core.v1.PodSpec; if you choose to ignore these errors, turn validation off with --validate=false

After:

:; bin/linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443" --ha | k apply -f -
secret/cluster-credentials-target configured
secret/cluster-credentials-target configured
link.multicluster.linkerd.io/target unchanged
clusterrole.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target unchanged
clusterrolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target unchanged
role.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target unchanged
rolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target unchanged
serviceaccount/linkerd-service-mirror-target unchanged
deployment.apps/linkerd-service-mirror-target configured
poddisruptionbudget.policy/linkerd-service-mirror-target configured
service/probe-gateway-target unchanged

We're not breaking non-ha:

:; bin/linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443" | k apply -f -
secret/cluster-credentials-target configured
secret/cluster-credentials-target configured
link.multicluster.linkerd.io/target unchanged
clusterrole.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target unchanged
clusterrolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-access-local-resources-target unchanged
role.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target unchanged
rolebinding.rbac.authorization.k8s.io/linkerd-service-mirror-read-remote-creds-target unchanged
serviceaccount/linkerd-service-mirror-target unchanged
deployment.apps/linkerd-service-mirror-target configured
service/probe-gateway-target unchanged

Render diff:

# < : bug.yaml
# > : fix.yaml
161,164c161,174
<       podAntiAffinity:
<         preferredDuringSchedulingIgnoredDuringExecution:
<         - podAffinityTerm:
<             labelSelector:
---
>       affinity:
>         podAntiAffinity:
>           preferredDuringSchedulingIgnoredDuringExecution:
>           - podAffinityTerm:
>               labelSelector:
>                 matchExpressions:
>                 - key: mirror.linkerd.io/cluster-name
>                   operator: In
>                   values:
>                   - target
>               topologyKey: topology.kubernetes.io/zone
>             weight: 100
>           requiredDuringSchedulingIgnoredDuringExecution:
>           - labelSelector:
170,179c180
<             topologyKey: topology.kubernetes.io/zone
<           weight: 100
<         requiredDuringSchedulingIgnoredDuringExecution:
<         - labelSelector:
<             matchExpressions:
<             - key: mirror.linkerd.io/cluster-name
<               operator: In
<               values:
<               - target
<           topologyKey: kubernetes.io/hostname
---
>             topologyKey: kubernetes.io/hostname
189c190
<         image: cr.l5d.io/linkerd/controller:stable-2.14.0
---
>         image: cr.l5d.io/linkerd/controller:git-006f6e2c
217c218
<     linkerd.io/created-by: linkerd/cli stable-2.14.0
---
>     linkerd.io/created-by: linkerd/cli git-006f6e2c

Two clusters can be linked in HA mode. When HA values are used, the
service-mirror deployment receives some pod affinity rules to ensure
fair scheduling of pods across a cluster's nodes.

The service-mirror Deployment's template seems to be broken at the
moment when using HA values. Affinity rules are incorrectly grouped
under a top-level `podAntiAffinity` field. The Kubernetes API requires
the rules to be grouped under a top-level `affinity` field. This change
rectifies that by introducing the missing parent.

Fixes #11603

Signed-off-by: Matei David <[email protected]>
@mateiidavid mateiidavid requested a review from a team as a code owner November 13, 2023 16:07
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

Would a test in multicluster/cmd/link_test.go for HA mode have caught this? can we add one?

@mateiidavid
Copy link
Member Author

@adleong added a test, good suggestion! I was actually unaware we already had some fixture-based tests for link output. It might've saved us when we introduced the change, it maybe would've forced us to re-read the manifest and notice the missing key. Ah well, moving forward we will have a golden file to compare against.

I noticed we had an awkward newline when not using HA:

157-    spec:
158-
159:      containers:
160-      - args:
161-        - service-mirror
162-        - -log-level=info

Go somehow still renders the newline even if the subtemplate eats the whitespace correctly... using a guard is the only way I managed to fix this. It makes it a bit more complicated than it has to be but at least it looks good. Last few things I did:

  • Use a lexical scope to copy the .Values tree, makes it look cleaner imo and we only copy it when we enable affinity
  • Fixed a typo in the original fixture name (default service mirror values).

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mateiidavid mateiidavid merged commit 97ac3dd into main Nov 22, 2023
36 checks passed
@mateiidavid mateiidavid deleted the matei/service-mirror-ha-tmpl branch November 22, 2023 17:35
@hawkw hawkw mentioned this pull request Nov 22, 2023
hawkw added a commit that referenced this pull request Nov 22, 2023
## edge-23.11.4

This edge release introduces support for the native sidecar containers
entering beta support in Kubernetes 1.29. This improves the startup and
shutdown ordering for the proxy relative to other containers, fixing the
long-standing shutdown issue with injected `Job`s. Furthermore, traffic
from other `initContainer`s can now be proxied by Linkerd.

In addition, this edge release includes Helm chart improvements, and
improvements to the multicluster extension.

* Added a new `config.alpha.linkerd.io/proxy-enable-native-sidecar`
  annotation and `Proxy.NativeSidecar` Helm option that causes the proxy
  container to run as an init-container (thanks @teejaded!) (#11465;
  fixes #11461)
* Fixed broken affinity rules for the multicluster `service-mirror` when
  running in HA mode (#11609; fixes #11603)
* Added a new check to `linkerd check` that ensures all extension
  namespaces are configured properly (#11629; fixes #11509)
* Updated the Prometheus Docker image used by the `linkerd-viz`
  extension to v2.48.0, resolving a number of CVEs in older Prometheus
  versions (#11633)
* Added `nodeAffinity` to `deployment` templates in the `linkerd-viz`
  and `linkerd-jaeger` Helm charts (thanks @naing2victor!) (#11464;
  fixes #10680)
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.

linkerd multicluster link --ha generates invalid deployment spec
3 participants