Skip to content

Commit

Permalink
address reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
@JoaoBraveCoding
JoaoBraveCoding committed Jun 3, 2024
1 parent e245bee commit 36a164a
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 43 deletions.
36 changes: 22 additions & 14 deletions internal/addon/addon.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
"open-cluster-management.io/addon-framework/pkg/agent"
"open-cluster-management.io/addon-framework/pkg/utils"
addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
workapiv1 "open-cluster-management.io/api/work/v1"
workv1 "open-cluster-management.io/api/work/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var (
errUnavailable = errors.New("probe condition is not satisfied")
errValueNotProbed = errors.New("value not probed")
errProbeConditionNotSatisfied = errors.New("probe condition is not satisfied")
errProbeValueIsNil = errors.New("probe value is nil")
errValueNotProbed = errors.New("value not probed")
)

func NewRegistrationOption(agentName string) *agent.RegistrationOption {
Expand Down Expand Up @@ -48,16 +49,16 @@ func AgentHealthProber() *agent.HealthProber {
WorkProber: &agent.WorkHealthProber{
ProbeFields: []agent.ProbeField{
{
ResourceIdentifier: workapiv1.ResourceIdentifier{
ResourceIdentifier: workv1.ResourceIdentifier{
Group: loggingv1.GroupVersion.Group,
Resource: ClusterLogForwardersResource,
Name: SpokeCLFName,
Namespace: SpokeCLFNamespace,
},
ProbeRules: []workapiv1.FeedbackRule{
ProbeRules: []workv1.FeedbackRule{
{
Type: workapiv1.JSONPathsType,
JsonPaths: []workapiv1.JsonPath{
Type: workv1.JSONPathsType,
JsonPaths: []workv1.JsonPath{
{
Name: clfProbeKey,
Path: clfProbePath,
Expand All @@ -67,16 +68,16 @@ func AgentHealthProber() *agent.HealthProber {
},
},
{
ResourceIdentifier: workapiv1.ResourceIdentifier{
ResourceIdentifier: workv1.ResourceIdentifier{
Group: otelv1alpha1.GroupVersion.Group,
Resource: OpenTelemetryCollectorsResource,
Name: SpokeOTELColName,
Namespace: SpokeOTELColNamespace,
},
ProbeRules: []workapiv1.FeedbackRule{
ProbeRules: []workv1.FeedbackRule{
{
Type: workapiv1.JSONPathsType,
JsonPaths: []workapiv1.JsonPath{
Type: workv1.JSONPathsType,
JsonPaths: []workv1.JsonPath{
{
Name: otelColProbeKey,
Path: otelColProbePath,
Expand All @@ -86,26 +87,33 @@ func AgentHealthProber() *agent.HealthProber {
},
},
},
HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error {
HealthCheck: func(identifier workv1.ResourceIdentifier, result workv1.StatusFeedbackResult) error {
for _, value := range result.Values {
switch {
case identifier.Resource == ClusterLogForwardersResource:
if value.Name != clfProbeKey {
continue
}

if value.Value.Integer == nil {
return fmt.Errorf("%w: clusterlogforwarder with key %s/%s", errProbeValueIsNil, identifier.Namespace, identifier.Name)
}

if *value.Value.String != "True" {
return fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, *value.Value.String, identifier.Namespace, identifier.Name)
return fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errProbeConditionNotSatisfied, *value.Value.String, identifier.Namespace, identifier.Name)
}

return nil
case identifier.Resource == OpenTelemetryCollectorsResource:
if value.Name != otelColProbeKey {
continue
}
if value.Value.Integer == nil {
return fmt.Errorf("%w: opentelemetrycollector with key %s/%s", errProbeValueIsNil, identifier.Namespace, identifier.Name)
}

if *value.Value.Integer < 1 {
return fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, *value.Value.Integer, identifier.Namespace, identifier.Name)
return fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errProbeConditionNotSatisfied, *value.Value.Integer, identifier.Namespace, identifier.Name)
}

return nil
Expand Down
26 changes: 13 additions & 13 deletions internal/addon/addon_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import (
"testing"

"github.com/stretchr/testify/require"
v1 "open-cluster-management.io/api/work/v1"
workv1 "open-cluster-management.io/api/work/v1"

otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
loggingv1 "github.com/openshift/cluster-logging-operator/apis/logging/v1"
)

func Test_AgentHealthProber_CLF(t *testing.T) {
unhealthyError := fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, "False", SpokeCLFNamespace, SpokeCLFName)
unhealthyError := fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errProbeConditionNotSatisfied, "False", SpokeCLFNamespace, SpokeCLFName)
for _, tc := range []struct {
name string
status string
Expand All @@ -30,17 +30,17 @@ func Test_AgentHealthProber_CLF(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
healthProber := AgentHealthProber()
err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{
err := healthProber.WorkProber.HealthCheck(workv1.ResourceIdentifier{
Group: loggingv1.GroupVersion.Group,
Resource: ClusterLogForwardersResource,
Name: SpokeCLFName,
Namespace: SpokeCLFNamespace,
}, v1.StatusFeedbackResult{
Values: []v1.FeedbackValue{
}, workv1.StatusFeedbackResult{
Values: []workv1.FeedbackValue{
{
Name: "isReady",
Value: v1.FieldValue{
Type: v1.String,
Value: workv1.FieldValue{
Type: workv1.String,
String: &tc.status,
},
},
Expand All @@ -56,7 +56,7 @@ func Test_AgentHealthProber_CLF(t *testing.T) {
}

func Test_AgentHealthProber_OTELCol(t *testing.T) {
unhealthyError := fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, 0, SpokeOTELColNamespace, SpokeOTELColName)
unhealthyError := fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errProbeConditionNotSatisfied, 0, SpokeOTELColNamespace, SpokeOTELColName)
for _, tc := range []struct {
name string
replicas int64
Expand All @@ -74,17 +74,17 @@ func Test_AgentHealthProber_OTELCol(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
healthProber := AgentHealthProber()
err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{
err := healthProber.WorkProber.HealthCheck(workv1.ResourceIdentifier{
Group: otelv1alpha1.GroupVersion.Group,
Resource: OpenTelemetryCollectorsResource,
Name: SpokeOTELColName,
Namespace: SpokeOTELColNamespace,
}, v1.StatusFeedbackResult{
Values: []v1.FeedbackValue{
}, workv1.StatusFeedbackResult{
Values: []workv1.FeedbackValue{
{
Name: "replicas",
Value: v1.FieldValue{
Type: v1.Integer,
Value: workv1.FieldValue{
Type: workv1.Integer,
Integer: &tc.replicas,
},
},
Expand Down
2 changes: 1 addition & 1 deletion internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterlogforwarder.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
apiVersion: logging.openshift.io/v1
kind: ClusterLogForwarder
metadata:
name: instance
name: mcoa-instance
namespace: openshift-logging
labels:
app: {{ template "logginghelm.name" . }}
Expand Down
2 changes: 1 addition & 1 deletion internal/addon/manifests/charts/mcoa/charts/tracing/templates/namespace.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ metadata:
apiVersion: v1
kind: Namespace
metadata:
name: spoke-otelcol
name: mcoa-opentelemetry
labels:
app: {{ template "tracinghelm.name" . }}
chart: {{ template "tracinghelm.chart" . }}
Expand Down
4 changes: 2 additions & 2 deletions internal/addon/manifests/charts/mcoa/charts/tracing/templates/opentelemetrycollector.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
name: spoke-otelcol
namespace: spoke-otelcol
name: mcoa-instance
namespace: mcoa-opentelemetry
labels:
app: {{ template "tracinghelm.name" . }}
chart: {{ template "tracinghelm.chart" . }}
Expand Down
2 changes: 1 addition & 1 deletion internal/addon/manifests/charts/mcoa/charts/tracing/templates/secret.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ apiVersion: v1
kind: Secret
metadata:
name: {{ $secret_config.name }}
namespace: spoke-otelcol
namespace: mcoa-opentelemetry
labels:
app: {{ template "tracinghelm.name" $ }}
chart: {{ template "tracinghelm.chart" $ }}
Expand Down
6 changes: 3 additions & 3 deletions internal/addon/var.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ const (
AdcTracingisabledKey = "tracingDisabled"

ClusterLogForwardersResource = "clusterlogforwarders"
SpokeCLFName = "instance"
SpokeCLFName = "mcoa-instance"
SpokeCLFNamespace = "openshift-logging"
clfProbeKey = "isReady"
clfProbePath = ".status.conditions[?(@.type==\"Ready\")].status"

OpenTelemetryCollectorsResource = "opentelemetrycollectors"
SpokeOTELColName = "spoke-otelcol"
SpokeOTELColNamespace = "spoke-otelcol"
SpokeOTELColName = "mcoa-instance"
SpokeOTELColNamespace = "mcoa-opentelemetry"
otelColProbeKey = "replicas"
otelColProbePath = ".spec.replicas"
)
Expand Down
6 changes: 3 additions & 3 deletions internal/logging/helm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,15 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) {
},
ConfigReferent: addonapiv1alpha1.ConfigReferent{
Namespace: "open-cluster-management",
Name: "instance",
Name: "mcoa-instance",
},
},
}

// Setup configuration resources: ClusterLogForwarder, AddOnDeploymentConfig, Secrets, ConfigMaps
clf = &loggingv1.ClusterLogForwarder{
ObjectMeta: metav1.ObjectMeta{
Name: "instance",
Name: "mcoa-instance",
Namespace: "open-cluster-management",
Annotations: map[string]string{
"authentication.mcoa.openshift.io/app-logs": "SecretReference",
Expand Down Expand Up @@ -225,7 +225,7 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) {
require.NotNil(t, obj.Spec.Outputs[1].Secret)
require.Equal(t, "static-authentication", obj.Spec.Outputs[0].Secret.Name)
require.Equal(t, "static-authentication", obj.Spec.Outputs[1].Secret.Name)
// Check name and namespace to make usre that if we change the helm
// Check name and namespace to make sure that if we change the helm
// manifests that we don't break the addon probes
require.Equal(t, addon.SpokeCLFName, obj.Name)
require.Equal(t, addon.SpokeCLFNamespace, obj.Namespace)
Expand Down
4 changes: 2 additions & 2 deletions internal/logging/manifests/logging_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func Test_BuildCLFSpec(t *testing.T) {
},
ConfigReferent: addonapiv1alpha1.ConfigReferent{
Namespace: "open-cluster-management",
Name: "instance",
Name: "mcoa-instance",
},
},
{
Expand Down Expand Up @@ -147,7 +147,7 @@ func Test_BuildCLFSpec(t *testing.T) {
// Setup configuration resources: ClusterLogForwarder, AddOnDeploymentConfig, Secrets, ConfigMaps
clf = &loggingv1.ClusterLogForwarder{
ObjectMeta: metav1.ObjectMeta{
Name: "instance",
Name: "mcoa-instance",
Namespace: "open-cluster-management",
},
Spec: loggingv1.ClusterLogForwarderSpec{
Expand Down
6 changes: 3 additions & 3 deletions internal/tracing/helm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) {
},
ConfigReferent: addonapiv1alpha1.ConfigReferent{
Namespace: "open-cluster-management",
Name: "spoke-otelcol",
Name: "mcoa-instance",
},
},
}
Expand All @@ -114,7 +114,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) {

otelCol = &otelv1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "spoke-otelcol",
Name: "mcoa-instance",
Namespace: "open-cluster-management",
},
Spec: otelv1alpha1.OpenTelemetryCollectorSpec{
Expand Down Expand Up @@ -188,7 +188,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) {
for _, obj := range objects {
switch obj := obj.(type) {
case *otelv1alpha1.OpenTelemetryCollector:
// Check name and namespace to make usre that if we change the helm
// Check name and namespace to make sure that if we change the helm
// manifests that we don't break the addon probes
require.Equal(t, addon.SpokeOTELColName, obj.Name)
require.Equal(t, addon.SpokeOTELColNamespace, obj.Namespace)
Expand Down

0 comments on commit 36a164a

Please sign in to comment.