Skip to content

Commit

Permalink
refactor: cleanups for k8sutils workload package. create enums and mo…
Browse files Browse the repository at this point in the history
…ve common functions (#1449)

This is another step in the way to move everything related to common
things into the common packages, and avoid duplications and untyped
usages in the project.

This PR:
- adds more files to the `workload` package in k8sutils to differentiate
between the different utils and enums
- enums for the workload kinds
- function to handle and convert workload kinds
- rename function to common format
- replace usages in controllers code to use enums where possible

There are countless places to go over and improve. This PR is touching
on the most easy and straight forward cases I spotted.
  • Loading branch information
blumamir authored Aug 18, 2024
1 parent 52da53c commit a8a45e9
Show file tree
Hide file tree
Showing 50 changed files with 534 additions and 321 deletions.
4 changes: 2 additions & 2 deletions autoscaler/controllers/datacollection/configmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func NewMockInstrumentedApplication(workloadObject client.Object) *odigosv1.Inst
gvk, _ := apiutil.GVKForObject(workloadObject, scheme.Scheme)
return &odigosv1.InstrumentedApplication{
ObjectMeta: metav1.ObjectMeta{
Name: workload.GetRuntimeObjectName(workloadObject.GetName(), gvk.Kind),
Name: workload.CalculateWorkloadRuntimeObjectName(workloadObject.GetName(), gvk.Kind),
Namespace: workloadObject.GetNamespace(),
OwnerReferences: []metav1.OwnerReference{
{
Expand All @@ -83,7 +83,7 @@ func NewMockInstrumentedApplicationWoOwner(workloadObject client.Object) *odigos
gvk, _ := apiutil.GVKForObject(workloadObject, scheme.Scheme)
return &odigosv1.InstrumentedApplication{
ObjectMeta: metav1.ObjectMeta{
Name: workload.GetRuntimeObjectName(workloadObject.GetName(), gvk.Kind),
Name: workload.CalculateWorkloadRuntimeObjectName(workloadObject.GetName(), gvk.Kind),
Namespace: workloadObject.GetNamespace(),
},
}
Expand Down
2 changes: 2 additions & 0 deletions autoscaler/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO
github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/tj/assert v0.0.3 h1:Df/BlaZ20mq6kuai7f5z2TvPFiwC3xaWJSDQNiIS3Rk=
github.com/tj/assert v0.0.3/go.mod h1:Ne6X72Q+TB1AteidzQncjw9PabbMp4PBMZ1k+vd1Pvk=
github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
go.opentelemetry.io/otel v1.28.0 h1:/SqNcYk+idO0CxKEUOtKQClMK/MimZihKYMruSMViUo=
Expand Down
2 changes: 1 addition & 1 deletion cli/cmd/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func getRelevantResources(ctx context.Context, client *kube.Client, workloadObj
return
}

runtimeObjectName := workload.GetRuntimeObjectName(workloadObj.GetName(), workloadObj.Kind)
runtimeObjectName := workload.CalculateWorkloadRuntimeObjectName(workloadObj.GetName(), workloadObj.Kind)
instrumentationConfig, err = client.OdigosClient.InstrumentationConfigs(ns).Get(ctx, runtimeObjectName, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
Expand Down
9 changes: 0 additions & 9 deletions cli/cmd/resources/odiglet.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,6 @@ func NewOdigletClusterRole(psp bool) *rbacv1.ClusterRole {
"nodes",
},
},
{
Verbs: []string{
"get",
"list",
"watch",
},
APIGroups: []string{"apps"},
Resources: []string{"replicasets"},
},
{
Verbs: []string{
"get",
Expand Down
2 changes: 2 additions & 0 deletions cli/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO
github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/tj/assert v0.0.3 h1:Df/BlaZ20mq6kuai7f5z2TvPFiwC3xaWJSDQNiIS3Rk=
github.com/tj/assert v0.0.3/go.mod h1:Ne6X72Q+TB1AteidzQncjw9PabbMp4PBMZ1k+vd1Pvk=
github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
go.opentelemetry.io/otel v1.28.0 h1:/SqNcYk+idO0CxKEUOtKQClMK/MimZihKYMruSMViUo=
Expand Down
9 changes: 5 additions & 4 deletions frontend/endpoints/collector_metrics/node_collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"sync"

"github.com/odigos-io/odigos/frontend/endpoints/common"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/pmetric"
)
Expand Down Expand Up @@ -196,13 +197,13 @@ func metricAttributesToSourceID(attrs pcommon.Map) (common.SourceID, error) {
return common.SourceID{}, errors.New("namespace not found")
}

var kind string
var kind workload.WorkloadKind
if _, ok := attrs.Get(K8SDeploymentNameKey); ok {
kind = "Deployment"
kind = workload.WorkloadKindDeployment
} else if _, ok := attrs.Get(K8SStatefulSetNameKey); ok {
kind = "StatefulSet"
kind = workload.WorkloadKindStatefulSet
} else if _, ok := attrs.Get(K8SDaemonSetNameKey); ok {
kind = "DaemonSet"
kind = workload.WorkloadKindDaemonSet
} else {
return common.SourceID{}, errors.New("kind not found")
}
Expand Down
4 changes: 2 additions & 2 deletions frontend/endpoints/collector_metrics/watchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type deleteNotification struct {
notificationType deletedObject
object string
// used for source deletion notification
sourceID common.SourceID
sourceID common.SourceID
}

type deleteWatcher struct {
Expand Down Expand Up @@ -125,7 +125,7 @@ func runWatcherLoop(ctx context.Context, w watchers, notifyChan chan<- deleteNot
switch event.Type {
case watch.Deleted:
app := event.Object.(*v1alpha1.InstrumentedApplication)
name, kind, err := commonutils.GetWorkloadInfoRuntimeName(app.Name)
name, kind, err := commonutils.ExtractWorkloadInfoFromRuntimeObjectName(app.Name)
if err != nil {
fmt.Printf("error getting workload info: %v\n", err)
}
Expand Down
10 changes: 6 additions & 4 deletions frontend/endpoints/common/source.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package common

import "github.com/odigos-io/odigos/k8sutils/pkg/workload"

type SourceID struct {
// combination of namespace, kind and name is unique
Name string `json:"name"`
Kind string `json:"kind"`
Namespace string `json:"namespace"`
}
Name string `json:"name"`
Kind workload.WorkloadKind `json:"kind"`
Namespace string `json:"namespace"`
}
3 changes: 2 additions & 1 deletion frontend/endpoints/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/gin-gonic/gin"
collectormetrics "github.com/odigos-io/odigos/frontend/endpoints/collector_metrics"
"github.com/odigos-io/odigos/frontend/endpoints/common"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
)

type singleSourceMetricsResponse struct {
Expand All @@ -22,7 +23,7 @@ func GetSingleSourceMetrics(c *gin.Context, m *collectormetrics.OdigosMetricsCon

sID := common.SourceID{
Namespace: ns,
Kind: kind,
Kind: workload.WorkloadKind(kind),
Name: name,
}
metric, ok := m.GetSingleSourceMetrics(sID)
Expand Down
8 changes: 4 additions & 4 deletions frontend/endpoints/sources.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func GetSources(c *gin.Context, odigosns string) {

for _, item := range items {
if item.nsItem.InstrumentationEffective {
id := common.SourceID{Namespace: item.namespace, Kind: string(item.nsItem.Kind), Name: item.nsItem.Name}
id := common.SourceID{Namespace: item.namespace, Kind: workload.WorkloadKind(item.nsItem.Kind), Name: item.nsItem.Name}
effectiveInstrumentedSources[id] = ThinSource{
NumberOfRunningInstances: item.nsItem.Instances,
SourceID: id,
Expand Down Expand Up @@ -118,7 +118,7 @@ func GetSource(c *gin.Context) {
ns := c.Param("namespace")
kind := c.Param("kind")
name := c.Param("name")
k8sObjectName := workload.GetRuntimeObjectName(name, kind)
k8sObjectName := workload.CalculateWorkloadRuntimeObjectName(name, kind)

owner, numberOfRunningInstances := getWorkloadObject(c, ns, kind, name)
if owner == nil {
Expand All @@ -136,7 +136,7 @@ func GetSource(c *gin.Context) {
ts := ThinSource{
SourceID: common.SourceID{
Namespace: ns,
Kind: kind,
Kind: workload.WorkloadKind(kind),
Name: name,
},
NumberOfRunningInstances: numberOfRunningInstances,
Expand Down Expand Up @@ -255,7 +255,7 @@ func DeleteSource(c *gin.Context) {
func k8sInstrumentedAppToThinSource(app *v1alpha1.InstrumentedApplication) ThinSource {
var source ThinSource
source.Name = app.OwnerReferences[0].Name
source.Kind = app.OwnerReferences[0].Kind
source.Kind = workload.WorkloadKind(app.OwnerReferences[0].Kind)
source.Namespace = app.Namespace
var conditions []metav1.Condition
for _, condition := range app.Status.Conditions {
Expand Down
2 changes: 2 additions & 0 deletions frontend/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o
github.com/stretchr/testify v1.8.3/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/tj/assert v0.0.3 h1:Df/BlaZ20mq6kuai7f5z2TvPFiwC3xaWJSDQNiIS3Rk=
github.com/tj/assert v0.0.3/go.mod h1:Ne6X72Q+TB1AteidzQncjw9PabbMp4PBMZ1k+vd1Pvk=
github.com/twitchyliquid64/golang-asm v0.15.1 h1:SU5vSMR7hnwNxj24w34ZyCi/FmDZTkS4MhqMhdFk5YI=
github.com/twitchyliquid64/golang-asm v0.15.1/go.mod h1:a1lVb/DtPvCB8fslRZhAngC2+aY1QWCk3Cedj/Gdt08=
github.com/ugorji/go v1.2.7/go.mod h1:nF9osbDWLy6bDVv/Rtoh6QgnvNDpmCalQV5urGCCS6M=
Expand Down
6 changes: 3 additions & 3 deletions frontend/kube/watchers/instrumentation_instance_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ func StartInstrumentationInstanceWatcher(ctx context.Context, namespace string)
Event: sse.MessageEventModified,
MessageType: sse.MessageTypeError,
Duration: 10 * time.Second,
CRDType: "InstrumentationInstance",
FailureBatchMessageFunc: func (batchSize int, crd string) string {
CRDType: "InstrumentationInstance",
FailureBatchMessageFunc: func(batchSize int, crd string) string {
return fmt.Sprintf("Failed to instrument %d instances", batchSize)
},
},
Expand Down Expand Up @@ -83,7 +83,7 @@ func handleModifiedInstrumentationInstance(event watch.Event) {
genericErrorMessage(sse.MessageEventModified, "InstrumentationInstance", "error getting instrumented app name from labels")
}

name, kind, err := commonutils.GetWorkloadInfoRuntimeName(instrumentedAppName)
name, kind, err := commonutils.ExtractWorkloadInfoFromRuntimeObjectName(instrumentedAppName)
if err != nil {
genericErrorMessage(sse.MessageEventModified, "InstrumentationInstance", "error getting workload info")
}
Expand Down
14 changes: 7 additions & 7 deletions frontend/kube/watchers/instrumented_application_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ var deletedEventBatcher *EventBatcher
func StartInstrumentedApplicationWatcher(ctx context.Context, namespace string) error {
addedEventBatcher = NewEventBatcher(
EventBatcherConfig{
Event: sse.MessageEventAdded,
CRDType: "InstrumentedApplication",
Event: sse.MessageEventAdded,
CRDType: "InstrumentedApplication",
SuccessBatchMessageFunc: func(count int, crdType string) string {
return fmt.Sprintf("successfully added %d sources", count)
},
Expand All @@ -31,8 +31,8 @@ func StartInstrumentedApplicationWatcher(ctx context.Context, namespace string)

deletedEventBatcher = NewEventBatcher(
EventBatcherConfig{
Event: sse.MessageEventDeleted,
CRDType: "InstrumentedApplication",
Event: sse.MessageEventDeleted,
CRDType: "InstrumentedApplication",
SuccessBatchMessageFunc: func(count int, crdType string) string {
return fmt.Sprintf("successfully deleted %d sources", count)
},
Expand All @@ -41,7 +41,7 @@ func StartInstrumentedApplicationWatcher(ctx context.Context, namespace string)
},
},
)

watcher, err := kube.DefaultClient.OdigosClient.InstrumentedApplications(namespace).Watch(context.Background(), metav1.ListOptions{})
if err != nil {
return fmt.Errorf("error creating watcher: %v", err)
Expand Down Expand Up @@ -75,7 +75,7 @@ func handleInstrumentedApplicationWatchEvents(ctx context.Context, watcher watch
}

func handleAddedEvent(app *v1alpha1.InstrumentedApplication) {
name, kind, err := commonutils.GetWorkloadInfoRuntimeName(app.Name)
name, kind, err := commonutils.ExtractWorkloadInfoFromRuntimeObjectName(app.Name)
if err != nil {
genericErrorMessage(sse.MessageEventAdded, "InstrumentedApplication", "error getting workload info")
return
Expand All @@ -87,7 +87,7 @@ func handleAddedEvent(app *v1alpha1.InstrumentedApplication) {
}

func handleDeletedEvent(app *v1alpha1.InstrumentedApplication) {
name, _, err := commonutils.GetWorkloadInfoRuntimeName(app.Name)
name, _, err := commonutils.ExtractWorkloadInfoFromRuntimeObjectName(app.Name)
if err != nil {
genericErrorMessage(sse.MessageEventDeleted, "InstrumentedApplication", "error getting workload info")
return
Expand Down
1 change: 0 additions & 1 deletion helm/odigos/templates/odiglet/clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ rules:
resources:
- daemonsets
- deployments
- replicasets
- statefulsets
verbs:
- get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ func deleteWorkloadInstrumentedApplication(ctx context.Context, kubeClient clien

ns := workloadObject.GetNamespace()
name := workloadObject.GetName()
kind := workload.GetWorkloadKind(workloadObject)
instrumentedApplicationName := workload.GetRuntimeObjectName(name, kind)
kind := workload.WorkloadKindFromClientObject(workloadObject)
instrumentedApplicationName := workload.CalculateWorkloadRuntimeObjectName(name, kind)

instAppErr := kubeClient.Delete(ctx, &odigosv1.InstrumentedApplication{
ObjectMeta: metav1.ObjectMeta{
Expand Down
49 changes: 18 additions & 31 deletions instrumentor/controllers/instrumentationdevice/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,32 +99,32 @@ func addInstrumentationDeviceToWorkload(ctx context.Context, kubeClient client.C
return nil
}

func removeInstrumentationDeviceFromWorkload(ctx context.Context, kubeClient client.Client, namespace string, workloadKind string, workloadName string, uninstrumentReason ApplyInstrumentationDeviceReason) error {
func removeInstrumentationDeviceFromWorkload(ctx context.Context, kubeClient client.Client, namespace string, workloadKind workload.WorkloadKind, workloadName string, uninstrumentReason ApplyInstrumentationDeviceReason) error {

obj, err := getObjectFromKindString(workloadKind)
if err != nil {
return err
workloadObj := workload.ClientObjectFromWorkloadKind(workloadKind)
if workloadObj == nil {
return errors.New("unknown kind")
}

err = kubeClient.Get(ctx, client.ObjectKey{
err := kubeClient.Get(ctx, client.ObjectKey{
Namespace: namespace,
Name: workloadName,
}, obj)
}, workloadObj)
if err != nil {
return client.IgnoreNotFound(err)
}

result, err := controllerutil.CreateOrPatch(ctx, kubeClient, obj, func() error {
result, err := controllerutil.CreateOrPatch(ctx, kubeClient, workloadObj, func() error {

// clear old ebpf instrumentation annotation, just in case it still exists
clearInstrumentationEbpf(obj)
podSpec, err := getPodSpecFromObject(obj)
clearInstrumentationEbpf(workloadObj)
podSpec, err := getPodSpecFromObject(workloadObj)
if err != nil {
return err
}

instrumentation.RevertInstrumentationDevices(podSpec)
err = instrumentation.RevertEnvOverwrites(obj, podSpec)
err = instrumentation.RevertEnvOverwrites(workloadObj, podSpec)
if err != nil {
return err
}
Expand All @@ -138,32 +138,32 @@ func removeInstrumentationDeviceFromWorkload(ctx context.Context, kubeClient cli
modified := result != controllerutil.OperationResultNone
if modified {
logger := log.FromContext(ctx)
logger.V(0).Info("removed instrumentation device from workload", "namespace", obj.GetNamespace(), "kind", obj.GetObjectKind(), "name", obj.GetName(), "reason", uninstrumentReason)
logger.V(0).Info("removed instrumentation device from workload", "namespace", workloadObj.GetNamespace(), "kind", workloadObj.GetObjectKind(), "name", workloadObj.GetName(), "reason", uninstrumentReason)
}

return nil
}

func getWorkloadObject(ctx context.Context, kubeClient client.Client, runtimeDetails *odigosv1.InstrumentedApplication) (client.Object, error) {
name, kind, err := workload.GetWorkloadInfoRuntimeName(runtimeDetails.Name)
name, kind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(runtimeDetails.Name)
if err != nil {
return nil, err
}

obj, err := getObjectFromKindString(kind)
if err != nil {
return nil, err
workloadObject := workload.ClientObjectFromWorkloadKind(kind)
if workloadObject == nil {
return nil, errors.New("unknown kind")
}

err = kubeClient.Get(ctx, client.ObjectKey{
Namespace: runtimeDetails.Namespace,
Name: name,
}, obj)
}, workloadObject)
if err != nil {
return nil, err
}

return obj, nil
return workloadObject, nil
}

func getPodSpecFromObject(obj client.Object) (*corev1.PodTemplateSpec, error) {
Expand All @@ -179,25 +179,12 @@ func getPodSpecFromObject(obj client.Object) (*corev1.PodTemplateSpec, error) {
}
}

func getObjectFromKindString(kind string) (client.Object, error) {
switch kind {
case "Deployment":
return &appsv1.Deployment{}, nil
case "StatefulSet":
return &appsv1.StatefulSet{}, nil
case "DaemonSet":
return &appsv1.DaemonSet{}, nil
default:
return nil, errors.New("unknown kind")
}
}

// reconciles a single workload, which might be triggered by a change in multiple resources.
// each time a relevant resource changes, this function is called to reconcile the workload
// and always writes the status into the InstrumentedApplication CR
func reconcileSingleWorkload(ctx context.Context, kubeClient client.Client, runtimeDetails *odigosv1.InstrumentedApplication, isNodeCollectorReady bool) error {

workloadName, workloadKind, err := workload.GetWorkloadInfoRuntimeName(runtimeDetails.Name)
workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(runtimeDetails.Name)
if err != nil {
conditions.UpdateStatusConditions(ctx, kubeClient, runtimeDetails, &runtimeDetails.Status.Conditions, metav1.ConditionFalse, appliedInstrumentationDeviceType, string(ApplyInstrumentationDeviceReasonErrRemoving), err.Error())
return err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c
}

// runtime details deleted: remove instrumentation from resource requests
workloadName, workloadKind, err := workload.GetWorkloadInfoRuntimeName(req.Name)
workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(req.Name)
if err != nil {
logger.Error(err, "error parsing workload info from runtime object name")
return ctrl.Result{}, err
Expand Down
Loading

0 comments on commit a8a45e9

Please sign in to comment.