From a328f78a7357d72f26df24538987bd5edaef8e9e Mon Sep 17 00:00:00 2001 From: Travis Nielsen Date: Mon, 23 Mar 2020 16:28:09 -0600 Subject: [PATCH] ceph: avoid duplication of mon anti-affinity The default pod anti-affinity for the mons that rook adds automatically intended to be appended to any anti-affinity that is specified in the cluster CR. There is a bug in the ApplyToPodSpec() method that has long existed. The issue is that when antiaffinity is appended, it will append not only to the pod spec, but will modify the original placement spec. Thus, each mon that is started will have one more antiaffinity clause than the previous mon. This condition is rarely hit or noticed because it commonly is only hit in the canary pods. Since these pods are immediately deleted, there are no side effects of the duplicate affinity clauses. The place where it becomes an issue is when the mons are backed by a PVC. In this case, the mons do not have node affinity and will hit the code path that appends to the antiaffinity and thus modifies the original antiaffinity. Signed-off-by: Travis Nielsen --- pkg/apis/rook.io/v1/placement.go | 6 ++-- pkg/apis/rook.io/v1/placement_test.go | 42 ++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/pkg/apis/rook.io/v1/placement.go b/pkg/apis/rook.io/v1/placement.go index b21cfea8a..62f24184e 100644 --- a/pkg/apis/rook.io/v1/placement.go +++ b/pkg/apis/rook.io/v1/placement.go @@ -29,13 +29,13 @@ func (p Placement) ApplyToPodSpec(t *v1.PodSpec) { t.Affinity = &v1.Affinity{} } if p.NodeAffinity != nil { - t.Affinity.NodeAffinity = p.NodeAffinity + t.Affinity.NodeAffinity = p.NodeAffinity.DeepCopy() } if p.PodAffinity != nil { - t.Affinity.PodAffinity = p.PodAffinity + t.Affinity.PodAffinity = p.PodAffinity.DeepCopy() } if p.PodAntiAffinity != nil { - t.Affinity.PodAntiAffinity = p.PodAntiAffinity + t.Affinity.PodAntiAffinity = p.PodAntiAffinity.DeepCopy() } if p.Tolerations != nil { diff --git a/pkg/apis/rook.io/v1/placement_test.go b/pkg/apis/rook.io/v1/placement_test.go index 8c005beac..e40a680fd 100644 --- a/pkg/apis/rook.io/v1/placement_test.go +++ b/pkg/apis/rook.io/v1/placement_test.go @@ -22,6 +22,7 @@ import ( "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestPlacement_spec(t *testing.T) { @@ -74,32 +75,41 @@ tolerations: assert.Equal(t, expected, placement) } -func TestPlacement_ApplyToPodSpec(t *testing.T) { +func TestPlacementApplyToPodSpec(t *testing.T) { to := placementTestGetTolerations("foo", "bar") na := placementTestGenerateNodeAffinity() - expected := &v1.PodSpec{Affinity: &v1.Affinity{NodeAffinity: na}, Tolerations: to} + antiaffinity := placementAntiAffinity("v1") + expected := &v1.PodSpec{Affinity: &v1.Affinity{NodeAffinity: na, PodAntiAffinity: antiaffinity}, Tolerations: to} var p Placement var ps *v1.PodSpec - p = Placement{NodeAffinity: na, Tolerations: to} + p = Placement{NodeAffinity: na, Tolerations: to, PodAntiAffinity: antiaffinity} ps = &v1.PodSpec{} p.ApplyToPodSpec(ps) assert.Equal(t, expected, ps) + assert.Equal(t, 1, len(ps.Affinity.PodAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution)) + + // Appending some other antiaffinity to the pod spec should not alter the original placement antiaffinity + otherAntiAffinity := placementAntiAffinity("v2") + ps.Affinity.PodAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution = append( + ps.Affinity.PodAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, + otherAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution...) + assert.Equal(t, 1, len(antiaffinity.PreferredDuringSchedulingIgnoredDuringExecution)) // partial update - p = Placement{NodeAffinity: na} + p = Placement{NodeAffinity: na, PodAntiAffinity: antiaffinity} ps = &v1.PodSpec{Tolerations: to} p.ApplyToPodSpec(ps) assert.Equal(t, expected, ps) // overridden attributes - p = Placement{NodeAffinity: na, Tolerations: to} + p = Placement{NodeAffinity: na, Tolerations: to, PodAntiAffinity: antiaffinity} ps = &v1.PodSpec{Tolerations: placementTestGetTolerations("bar", "baz")} p.ApplyToPodSpec(ps) assert.Equal(t, expected, ps) - p = Placement{NodeAffinity: na} + p = Placement{NodeAffinity: na, PodAntiAffinity: antiaffinity} nap := placementTestGenerateNodeAffinity() nap.PreferredDuringSchedulingIgnoredDuringExecution[0].Weight = 5 ps = &v1.PodSpec{Affinity: &v1.Affinity{NodeAffinity: nap}, Tolerations: to} @@ -107,7 +117,7 @@ func TestPlacement_ApplyToPodSpec(t *testing.T) { assert.Equal(t, expected, ps) } -func TestPlacement_Merge(t *testing.T) { +func TestPlacementMerge(t *testing.T) { to := placementTestGetTolerations("foo", "bar") na := placementTestGenerateNodeAffinity() @@ -145,6 +155,24 @@ func placementTestGetTolerations(key, value string) []v1.Toleration { } } +func placementAntiAffinity(value string) *v1.PodAntiAffinity { + return &v1.PodAntiAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []v1.WeightedPodAffinityTerm{ + { + Weight: 50, + PodAffinityTerm: v1.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": value, + }, + }, + TopologyKey: v1.LabelHostname, + }, + }, + }, + } +} + func placementTestGenerateNodeAffinity() *v1.NodeAffinity { return &v1.NodeAffinity{ RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{