Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 2 additions & 2 deletions operator/internal/manifests/distributor.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func configureDistributorGRPCServicePKI(deployment *appsv1.Deployment, opts Opti
// Distributor pods.
func newDistributorPodDisruptionBudget(opts Options) *policyv1.PodDisruptionBudget {
l := ComponentLabels(LabelDistributorComponent, opts.Name)
mu := intstr.FromInt(1)
maxUnavailable := intstr.FromInt32(1)
return &policyv1.PodDisruptionBudget{
TypeMeta: metav1.TypeMeta{
Kind: "PodDisruptionBudget",
Expand All @@ -251,7 +251,7 @@ func newDistributorPodDisruptionBudget(opts Options) *policyv1.PodDisruptionBudg
Selector: &metav1.LabelSelector{
MatchLabels: l,
},
MinAvailable: &mu,
MaxUnavailable: &maxUnavailable,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this can be "modernized" to this (ptr is already used in this file):

Suggested change
MaxUnavailable: &maxUnavailable,
MaxUnavailable: ptr.To(intstr.FromInt32(1)),

},
}
}
4 changes: 2 additions & 2 deletions operator/internal/manifests/distributor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ func TestBuildDistributor_PodDisruptionBudget(t *testing.T) {
require.NotNil(t, pdb)
require.Equal(t, "abcd-distributor", pdb.Name)
require.Equal(t, "efgh", pdb.Namespace)
require.NotNil(t, pdb.Spec.MinAvailable.IntVal)
require.Equal(t, int32(1), pdb.Spec.MinAvailable.IntVal)
require.NotNil(t, pdb.Spec.MaxUnavailable)
require.Equal(t, int32(1), pdb.Spec.MaxUnavailable.IntVal)
require.EqualValues(t, ComponentLabels(LabelDistributorComponent, opts.Name), pdb.Spec.Selector.MatchLabels)
}

Expand Down
14 changes: 3 additions & 11 deletions operator/internal/manifests/ingester.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func NewIngesterStatefulSet(opts Options) *appsv1.StatefulSet {
fmt.Sprintf("-config.file=%s", path.Join(config.LokiConfigMountDir, config.LokiConfigFileName)),
fmt.Sprintf("-runtime-config.file=%s", path.Join(config.LokiConfigMountDir, config.LokiRuntimeConfigFileName)),
"-config.expand-env=true",
fmt.Sprintf("-ingester.tokens-file-path=%s", ingesterTokensFilePath),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(optional: This is less a comment for the purpose of adding this option for a test and more for when we later add it to the operator permanently)

We had moved "all" of the configuration to the config file at one point and, for now, I think I would move this option to the configuration file as well.

Having everything in the configuration file has its own set of advantages/disadvantages, which I have recently been thinking about again with some new insight, but I would like to make it a conscious decision.

For testing the impact of the option on running the ingesters it's fine where it is though 👍

},
ReadinessProbe: lokiReadinessProbe(),
LivenessProbe: lokiLivenessProbe(),
Expand Down Expand Up @@ -290,16 +291,7 @@ func configureIngesterGRPCServicePKI(sts *appsv1.StatefulSet, opts Options) erro
// Ingester pods.
func newIngesterPodDisruptionBudget(opts Options) *policyv1.PodDisruptionBudget {
l := ComponentLabels(LabelIngesterComponent, opts.Name)
rf := DefaultLokiStackSpec(opts.Stack.Size).Replication.Factor
if opts.Stack.Replication != nil && opts.Stack.Replication.Factor != 0 {
rf = opts.Stack.Replication.Factor
}

pdbMinAvailable := rf
if pdbMinAvailable >= opts.Stack.Template.Ingester.Replicas {
pdbMinAvailable = max(1, opts.Stack.Template.Ingester.Replicas-1)
}
minAvailable := intstr.FromInt32(pdbMinAvailable)
maxUnavailable := intstr.FromInt32(1)

return &policyv1.PodDisruptionBudget{
TypeMeta: metav1.TypeMeta{
Expand All @@ -315,7 +307,7 @@ func newIngesterPodDisruptionBudget(opts Options) *policyv1.PodDisruptionBudget
Selector: &metav1.LabelSelector{
MatchLabels: l,
},
MinAvailable: &minAvailable,
MaxUnavailable: &maxUnavailable,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
MaxUnavailable: &maxUnavailable,
MaxUnavailable: ptr.To(intstr.FromInt32(1)),

},
}
}
96 changes: 43 additions & 53 deletions operator/internal/manifests/ingester_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package manifests

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -104,61 +105,50 @@ func TestNewIngesterStatefulSet_SelectorMatchesLabels(t *testing.T) {
}

func TestBuildIngester_PodDisruptionBudget(t *testing.T) {
for _, tc := range []struct {
Name string
Replicas int
ExpectedMinAvailable int
Size lokiv1.LokiStackSizeType
}{
{
Name: "replication factor = replicas",
Size: lokiv1.SizeOneXExtraSmall,
Replicas: 2,
ExpectedMinAvailable: 1,
},
{
Name: "replication factor < replicas",
Size: lokiv1.SizeOneXMedium,
Replicas: 3,
ExpectedMinAvailable: 2,
},
{
Name: "replication factor > replicas",
Size: lokiv1.SizeOneXDemo,
Replicas: 1,
ExpectedMinAvailable: 1,
},
} {
t.Run(tc.Name, func(t *testing.T) {
opts := Options{
Name: "abcd",
Namespace: "efgh",
Gates: v1.FeatureGates{},
Stack: lokiv1.LokiStackSpec{
Size: tc.Size,
Template: &lokiv1.LokiTemplateSpec{
Ingester: &lokiv1.LokiComponentSpec{
Replicas: int32(tc.Replicas),
},
},
Tenants: &lokiv1.TenantsSpec{
Mode: lokiv1.OpenshiftLogging,
},
opts := Options{
Name: "abcd",
Namespace: "efgh",
Gates: v1.FeatureGates{},
Stack: lokiv1.LokiStackSpec{
Template: &lokiv1.LokiTemplateSpec{
Ingester: &lokiv1.LokiComponentSpec{
Replicas: 3,
},
}
objs, err := BuildIngester(opts)
require.NoError(t, err)
require.Len(t, objs, 4)

pdb := objs[3].(*policyv1.PodDisruptionBudget)
require.NotNil(t, pdb)
require.Equal(t, "abcd-ingester", pdb.Name)
require.Equal(t, "efgh", pdb.Namespace)
require.NotNil(t, pdb.Spec.MinAvailable.IntVal)
require.Equal(t, int32(tc.ExpectedMinAvailable), pdb.Spec.MinAvailable.IntVal)
require.EqualValues(t, ComponentLabels(LabelIngesterComponent, opts.Name), pdb.Spec.Selector.MatchLabels)
})
},
Tenants: &lokiv1.TenantsSpec{
Mode: lokiv1.OpenshiftLogging,
},
},
}
objs, err := BuildIngester(opts)
require.NoError(t, err)
require.Len(t, objs, 4)

pdb := objs[3].(*policyv1.PodDisruptionBudget)
require.NotNil(t, pdb)
require.Equal(t, "abcd-ingester", pdb.Name)
require.Equal(t, "efgh", pdb.Namespace)
require.NotNil(t, pdb.Spec.MaxUnavailable)
require.Equal(t, int32(1), pdb.Spec.MaxUnavailable.IntVal)
require.EqualValues(t, ComponentLabels(LabelIngesterComponent, opts.Name), pdb.Spec.Selector.MatchLabels)
}

func TestNewIngesterStatefulSet_HasTokensFileArg(t *testing.T) {
ss := NewIngesterStatefulSet(Options{
Name: "abcd",
Namespace: "efgh",
Stack: lokiv1.LokiStackSpec{
StorageClassName: "standard",
Template: &lokiv1.LokiTemplateSpec{
Ingester: &lokiv1.LokiComponentSpec{
Replicas: 1,
},
},
},
})

container := ss.Spec.Template.Spec.Containers[0]
require.Contains(t, container.Args, fmt.Sprintf("-ingester.tokens-file-path=%s", ingesterTokensFilePath))
}

func TestNewIngesterStatefulSet_TopologySpreadConstraints(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions operator/internal/manifests/var.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ const (
dataDirectory = "/tmp/loki"
rulesStorageDirectory = "/tmp/rules"

ingesterTokensFilePath = dataDirectory + "/ring-tokens"

rulerContainerName = "loki-ruler"

// EnvRelatedImageLoki is the environment variable to fetch the Loki image pullspec.
Expand Down
Loading