Implement Source CRD instrumentation #2059
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RonFed
reviewed
Dec 24, 2024
| // GetSourceListForWorkload returns a SourceList of all Sources that have matching | ||
| // workload name, namespace, and kind labels for an object. In theory, this should only | ||
| // ever return a list with 0 or 1 items, but due diligence should handle unexpected cases. | ||
| func GetSourceListForWorkload(ctx context.Context, kubeClient client.Client, obj client.Object) (*SourceList, error) { |
There was a problem hiding this comment.
nit:
Suggested change
| func GetSourceListForWorkload(ctx context.Context, kubeClient client.Client, obj client.Object) (*SourceList, error) { | |
| func GetSourceListForWorkload(ctx context.Context, kubeClient client.Client, obj client.Object) (SourceList, error) { |
| // Added by startlangdetection controller when Source is created | ||
| var instrumentedApplicationFinalizer = "odigos.io/source-instrumentedapplication-finalizer" | ||
|
|
||
| type SourceDeletedPredicate struct{} |
There was a problem hiding this comment.
We have a utils package for predicates.
This one exists in k8sutils/pkg.predicate/deletion.go
| source.Labels[workloadNamespaceLabel] = source.Spec.Workload.Namespace | ||
| source.Labels[workloadKindLabel] = string(source.Spec.Workload.Kind) | ||
|
|
||
| if err := r.Update(ctx, source); err != nil { |
There was a problem hiding this comment.
I'm not sure it is possible here but since we are using Get modify Update we could get a conflict error if the update is on a stale object.
We have k8sutils/pkg/utils/retryconflict.go if it is relevant here.
| // GetSourceListForWorkload returns a SourceList of all Sources that have matching | ||
| // workload name, namespace, and kind labels for an object. In theory, this should only | ||
| // ever return a list with 0 or 1 items, but due diligence should handle unexpected cases. | ||
| func GetSourceListForWorkload(ctx context.Context, kubeClient client.Client, obj client.Object) (*SourceList, error) { |
There was a problem hiding this comment.
Another thought: maybe we could wrap this function in another once returning a single source or an error.
Comment on lines
+22
to
+24
| workloadNameLabel = "odigos.io/workload-name" | ||
| workloadNamespaceLabel = "odigos.io/workload-namespace" | ||
| workloadKindLabel = "odigos.io/workload-kind" |
There was a problem hiding this comment.
I assume these consts will be used in other places as well, so maybe worth considering placing them in k8sutils/pkg/consts
| @@ -74,5 +76,17 @@ func SetupWithManager(mgr ctrl.Manager) error { | |||
| return err | |||
| } | |||
|
|
|||
| err = builder. | |||
| ControllerManagedBy(mgr). | |||
| Named("startlangdetection-source"). | |||
There was a problem hiding this comment.
Should we add an event filter here to pass only Create events?
Comment on lines
+63
to
+70
| // Check if a Source object exists for this workload | ||
| sourceList, err := v1alpha1.GetSourceListForWorkload(ctx, k8sClient, obj) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if len(sourceList.Items) == 0 { | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
Which cases are handled here?
BenElferink
reviewed
Dec 24, 2024
| @@ -200,6 +200,11 @@ cli-install: | |||
| @echo "Installing odigos from source. version: $(ODIGOS_CLI_VERSION)" | |||
| cd ./cli ; go run -tags=embed_manifests . install --version $(ODIGOS_CLI_VERSION) | |||
|
|
|||
| .PHONY: cli-uninstall | |||
| cli-uninstall: | |||
| @echo "Installing odigos from source. version: $(ODIGOS_CLI_VERSION)" | |||
There was a problem hiding this comment.
Typo:
Suggested change
| @echo "Installing odigos from source. version: $(ODIGOS_CLI_VERSION)" | |
| @echo "Uninstalling odigos from source. version: $(ODIGOS_CLI_VERSION)" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces the basic implementation for instrumenting/uninstrumenting via
Sourceobjects (#2037)This does not remove current instrumentation with the
odigos-instrumentationlabel, and does not do any checks to avoid conflicts etc. That work will follow. Right now this just drops in the basic workflow alongside the current label approach.Changes:
SourceReconcilerto thestartlangdetectionInstrumentor controller.SourceobjectSourceis not being deleted, add 2 finalizers (one used forSourcedeletion, and another used by thedeleteinstrumentedapplicationcontroller), add workload labels to theSourcefor lookup, and continue with flow of creating anInstrumentationConfigby callingrequestOdigletsToCalculateRuntimeDetailsSourceis being deleted (determined by aDeletionTimestampbeing present in an Update event, with the finalizer blocking deletion), remove the source finalizer and delete theInstrumentationConfig.GetSourceListForWorkloadeverywhere we currently callworkload.IsWorkloadInstrumentationEffectiveEnabledthat returns anySourceobjects referencing the workload.SourceReconcilertodeleteinstrumentedapplicationInstrumentor controllerSourceobject is deleted, remove instrumented application finalizer and continue with reconciling the workload, which should trigger uninstrumentation based on the new checks forGetSourceListForWorkloadthroughoutI'm not a fan of using 2 finalizers and worry that we could end up with a deadlock/race. This is mostly due to InstrumentedApplication, and deprecating InstrumentedApplication should simplify the work required for this greatly.
Still needs:
odigos-instrumentationlabel to be shorthand for creating aSourceon that workload/namespaceworkload.IsWorkloadInstrumentationEffectiveEnabled