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.

Sign up for GitHub

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

Prevent operator from overriding .Spec.Replicas #979

Merged
merged 2 commits into from
Mar 20, 2020
Merged

Prevent operator from overriding .Spec.Replicas #979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be78029
Prevent operator from overriding .Spec.Replicas
jpkrohling Mar 19, 2020
009b087
Removed default replicas for query and ingester
jpkrohling Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions pkg/deployment/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,6 @@ func (c *Collector) Get() *appsv1.Deployment {
// see https://github.com/jaegertracing/jaeger-operator/issues/334
sort.Strings(options)

replicaSize := c.jaeger.Spec.Collector.Replicas
if replicaSize == nil || *replicaSize < 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Since we no longer have to check on nill. We might remove the pointer and use plain int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand your comment. The Deployment's Spec.Replica has the same type as our .Spec.Collector.Replicas, and nil means "not set".

Copy link
Member

Choose a reason for hiding this comment

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

c.jaeger.Spec.Collector.Replicas is a pointer. The question is whether we can change it to plain int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deployment's .Spec.Replicas is also a pointer. If we change our side to plain int, it suddenly has a value, like 0, which is not the same as nil. Once we get this ambiguity, our fix in the inventory package won't work anymore.

jaeger-operator/pkg/inventory/deployment.go

Lines 30 to 35 in 009b087

// if we have a nil value for the replicas in the desired deployment
// but we have a specific value in the current deployment, we override the desired with the current
// as this might have been written by an HPA
if tp.Spec.Replicas != nil && v.Spec.Replicas == nil {
v.Spec.Replicas = tp.Spec.Replicas
}

s := int32(1)
replicaSize = &s
}

return &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
APIVersion: "apps/v1",
Expand All @@ -115,7 +109,7 @@ func (c *Collector) Get() *appsv1.Deployment {
},
},
Spec: appsv1.DeploymentSpec{
Replicas: replicaSize,
Replicas: c.jaeger.Spec.Collector.Replicas,
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Expand Down
19 changes: 17 additions & 2 deletions pkg/deployment/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ func TestNegativeReplicas(t *testing.T) {

collector := NewCollector(jaeger)
dep := collector.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Equal(t, size, *dep.Spec.Replicas)
}

func TestDefaultSize(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})

collector := NewCollector(jaeger)
dep := collector.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Nil(t, dep.Spec.Replicas) // we let Kubernetes define the default
}

func TestReplicaSize(t *testing.T) {
Expand Down Expand Up @@ -488,6 +488,21 @@ func TestAutoscalersDisabledByExplicitOption(t *testing.T) {
assert.Len(t, a, 0)
}

func TestAutoscalersSetMaxReplicas(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what is going on. Could you please explain this test and is the name of the test correct?

Does the autoscaler set the number of replicas? The replicas are set directly in the CR and then autoscaler has the same value.

Copy link
Contributor Author

@jpkrohling jpkrohling Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This test is covering a hole in coverage that I found when working on this PR. Basically, when MaxReplicas is set in our CR, we pass it along to the HPA's MaxReplicas value. If we don't have an explicit value, we set an hard upper bound, 100.

Replicas set directly in the deployment CR are seen as "initial replicas", which are then overridden by the autoscaler as needed. So, it's possible to have the autoscaler set for 2 as MinReplicas, 10 as MaxReplicas and 5 as the deployment's Replicas. In that case, it first gets set to 5, then scales down to 2 or up to 10 based on the actual demand. The problem in this CR is that we were explicitly setting a default value for Replicas in the deployments, which would cause deployments that were autoscaled to be scaled back to 1 when no Replicas were set at our CR level.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this func test all replica fields Replicas, MinReplicas and MaxReplicas?

It would help me to understand the behavior if we had all combinations in the single test via:

tests := []struct{

replicas int,
minReplicas int,
maxReplicas int,
//...
//expected values for these fields
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have mentioned that the behavior I described is the Deployment's and HPA's. With our operator as part of the mix, setting an explicit value for Replicas will disable HPA, as we understand that people would want a fixed amount of replicas. I just checked and we have a test describing this case (TestAutoscalersDisabledByExplicitReplicaSize)

// prepare
maxReplicas := int32(2)
jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})
jaeger.Spec.Collector.MaxReplicas = &maxReplicas
c := NewCollector(jaeger)

// test
a := c.Autoscalers()

// verify
assert.Len(t, a, 1)
assert.Equal(t, maxReplicas, a[0].Spec.MaxReplicas)
}

func TestCollectoArgumentsOpenshiftTLS(t *testing.T) {
viper.Set("platform", v1.FlagPlatformOpenShift)
defer viper.Reset()
Expand Down
8 changes: 1 addition & 7 deletions pkg/deployment/ingester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,6 @@ func (i *Ingester) Get() *appsv1.Deployment {
// see https://github.com/jaegertracing/jaeger-operator/issues/334
sort.Strings(options)

replicaSize := i.jaeger.Spec.Ingester.Replicas
if replicaSize == nil || *replicaSize < 0 {
s := int32(1)
replicaSize = &s
}

return &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
APIVersion: "apps/v1",
Expand All @@ -97,7 +91,7 @@ func (i *Ingester) Get() *appsv1.Deployment {
},
},
Spec: appsv1.DeploymentSpec{
Replicas: replicaSize,
Replicas: i.jaeger.Spec.Ingester.Replicas,
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/deployment/ingester_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ func TestIngesterNegativeReplicas(t *testing.T) {

ingester := NewIngester(jaeger)
dep := ingester.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Equal(t, size, *dep.Spec.Replicas)
}

func TestIngesterDefaultSize(t *testing.T) {
jaeger := newIngesterJaeger("TestIngesterDefaultSize")

ingester := NewIngester(jaeger)
dep := ingester.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Nil(t, dep.Spec.Replicas) // we let Kubernetes define the default
}

func TestIngesterReplicaSize(t *testing.T) {
Expand Down
8 changes: 1 addition & 7 deletions pkg/deployment/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ func (q *Query) Get() *appsv1.Deployment {
// see https://github.com/jaegertracing/jaeger-operator/issues/334
sort.Strings(options)

replicaSize := q.jaeger.Spec.Query.Replicas
if replicaSize == nil || *replicaSize < 0 {
s := int32(1)
replicaSize = &s
}

return &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
APIVersion: "apps/v1",
Expand All @@ -103,7 +97,7 @@ func (q *Query) Get() *appsv1.Deployment {
},
},
Spec: appsv1.DeploymentSpec{
Replicas: replicaSize,
Replicas: q.jaeger.Spec.Query.Replicas,
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/deployment/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ func TestQueryNegativeReplicas(t *testing.T) {

query := NewQuery(jaeger)
dep := query.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Equal(t, size, *dep.Spec.Replicas)
}

func TestQueryDefaultSize(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestQueryDefaultSize"})

query := NewQuery(jaeger)
dep := query.Get()
assert.Equal(t, int32(1), *dep.Spec.Replicas)
assert.Nil(t, dep.Spec.Replicas) // we let Kubernetes define the default
}

func TestQueryReplicaSize(t *testing.T) {
Expand Down
7 changes: 7 additions & 0 deletions pkg/inventory/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ func ForDeployments(existing []appsv1.Deployment, desired []appsv1.Deployment) D
tp := t.DeepCopy()
util.InitObjectMeta(tp)

// if we have a nil value for the replicas in the desired deployment
// but we have a specific value in the current deployment, we override the desired with the current
// as this might have been written by an HPA
if tp.Spec.Replicas != nil && v.Spec.Replicas == nil {
v.Spec.Replicas = tp.Spec.Replicas
}

// we can't blindly DeepCopyInto, so, we select what we bring from the new to the old object
tp.Spec = v.Spec
tp.Spec = inject.PropagateOAuthCookieSecret(t.Spec, v.Spec)
Expand Down
34 changes: 34 additions & 0 deletions pkg/inventory/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,37 @@ func TestDeploymentInventoryNewWithSameNameAsExisting(t *testing.T) {

assert.Len(t, inv.Delete, 0)
}

func TestDeploymentKeepReplicasWhenDesiredIsNil(t *testing.T) {
replicas := int32(2)
existing := []appsv1.Deployment{{
Spec: appsv1.DeploymentSpec{
Replicas: &replicas,
},
}}
desired := []appsv1.Deployment{{}}

inv := ForDeployments(existing, desired)
assert.Len(t, inv.Update, 1)
assert.Equal(t, replicas, *inv.Update[0].Spec.Replicas)
}

func TestDeploymentSetReplicasWhenDesiredIsNotNil(t *testing.T) {
replicas := int32(2)
existing := []appsv1.Deployment{{
Spec: appsv1.DeploymentSpec{
Replicas: &replicas,
},
}}

desiredReplicas := int32(1)
desired := []appsv1.Deployment{{
Spec: appsv1.DeploymentSpec{
Replicas: &desiredReplicas,
},
}}

inv := ForDeployments(existing, desired)
assert.Len(t, inv.Update, 1)
assert.Equal(t, desiredReplicas, *inv.Update[0].Spec.Replicas)
}