cOdiglet to use criwrapper to create ic #2015

tamirdavid1 · 2024-12-16T15:20:56Z

No description provided.

edeNFed · 2024-12-17T10:10:29Z

api/config/crd/bases/odigos.io_instrumentationconfigs.yaml

              runtimeDetailsByContainer:
                description: Capture Runtime Details for the workloads that this CR
                  applies to.
                items:
                  properties:
                    containerName:
                      type: string
+                    criErrorMessage:


isnt status a better place for error messages instead of spec?

edeNFed · 2024-12-17T10:31:05Z

k8sutils/pkg/cri/criwrapper.go

+	for _, endpoint := range defaultRuntimeEndpoints {
+		rc.Logger.Info("Attempting to connect to CRI runtime", "endpoint", endpoint)
+
+		rc.conn, err = grpc.NewClient(


Are you sure this is actually trying to connect and not just creating a client?
Few thoughts:

As only one of the default Endpoints will respond - maybe worth putting a timeout when opening a connection as some of them will just time out so we can bail quickly

I think the CRI implementation is part of the container id in Kubernetes maybe we can use that (for example getting the container id of the current Odiglet Pod) and connector only to the right one instead of trying all the options

I Warp it up as a Connect() - The function’s primary purpose is to connect to the CRI runtime, which it achieves by establishing and validating a gRPC connection.

I change the logic so it will try to find the unix domain socket in the FS so we wont need to iterate over them and try to connect to each one of them

edeNFed · 2024-12-17T10:31:55Z

k8sutils/pkg/cri/criwrapper.go

+	}
+
+	// Call the ContainerStatus API
+	response, err := rc.client.ContainerStatus(ctx, &criapi.ContainerStatusRequest{


Maybe we want to put a timeout on that? In case the remote CRI does not respond in time / unavailable?

edeNFed · 2024-12-17T10:38:37Z

odiglet/pkg/kube/runtime_details/inspection.go

+		return
+	}
+
+	// Verify if environment variables already exist in the container manifest.


Why not always collect the relevant env vars from Dokcerfile?
I don't have a good scenario in my head, but my feeling is that the more stateless we can make everything the simpler it will be

edeNFed · 2024-12-17T10:55:14Z

odiglet/pkg/kube/runtime_details/inspection.go

+	}
+
+	// Check if status is already set
+	if len(currentConfig.Status.RuntimeDetailsByContainer) != 0 {


Why not update always? This means only the first Odiglet that runs this wins, so we will not update the value if the user changed the docker image?

api/odigos/v1alpha1/instrumentedapplication_types.go

blumamir · 2024-12-19T07:09:01Z

api/odigos/v1alpha1/instrumentedapplication_types.go

+	CriErrorMessage       *string                    `json:"criErrorMessage,omitempty"`
+	EnvVarsFromDockerFile []EnvVar                   `json:"envVarsFromDockerFile,omitempty"`


nit: add a comment here describing what these values record and why we need them

api/odigos/v1alpha1/instrumentedapplication_types.go

k8sutils/pkg/cri/criwrapper.go

blumamir · 2024-12-22T11:39:22Z

k8sutils/pkg/cri/criwrapper.go

+
+	// Validate the connection by invoking a lightweight method
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*2)
+	defer cancel()


This will run after rc.client.Version has returned, right at which point cancel is useless

Is it needed?

According to what i've read:
You need defer cancel() to properly release resources associated with the context. It's a best practice whenever you create a context with WithTimeout or WithCancel, even if the timeout expires or the function exits early. This habit helps prevent subtle resource leaks and keeps your program efficient.

In addition to it, we're doing that in everyplace we're creating context with timeout (config_provider, kubelet communication and more)

k8sutils/pkg/cri/criwrapper.go

blumamir · 2024-12-22T12:32:32Z

odiglet/pkg/kube/runtime_details/inspection.go

+	return ""
+}
+
+func checkEnvVarsInContainerManifest(container corev1.Container, envVarNames []string) bool {


We should do it per environment not per container, no?

blumamir · 2024-12-22T12:33:39Z

api/odigos/v1alpha1/instrumentedapplication_types.go

+	LibCType                *common.LibCType           `json:"libCType,omitempty"`
+	CriErrorMessage         *string                    `json:"criErrorMessage,omitempty"`
+	EnvFromContainerRuntime []EnvVar                   `json:"envFromContainerRuntime,omitempty"`
+	RuntimeUpdateState      *ProcessingState           `json:"runtimeUpdateState,omitempty"` // Tracks whether the new runtime detection process has been executed. If empty, the process has not been executed.


Does it make sense to save this per workload and not per container?

blumamir · 2024-12-22T12:35:53Z

odiglet/pkg/kube/runtime_details/instrumentationconfigs_controller.go

 	odigosConfig, err := k8sutils.GetCurrentOdigosConfig(ctx, r.Client)
 	if err != nil {
 		return reconcile.Result{}, err
 	}

-	runtimeResults, err := runtimeInspection([]corev1.Pod{*selectedPodForInspection}, odigosConfig.IgnoredContainers)
+	runtimeResults, err := runtimeInspection(ctx, pods, odigosConfig.IgnoredContainers, r.CriClient)


consider passing just one pod for efficiency

blumamir · 2024-12-22T13:03:40Z

api/odigos/v1alpha1/instrumentedapplication_types.go

+	LibCType                *common.LibCType           `json:"libCType,omitempty"`
+	CriErrorMessage         *string                    `json:"criErrorMessage,omitempty"`
+	EnvFromContainerRuntime []EnvVar                   `json:"envFromContainerRuntime,omitempty"`
+	RuntimeUpdateState      *ProcessingState           `json:"runtimeUpdateState,omitempty"` // Tracks whether the new runtime detection process has been executed. If empty, the process has not been executed.


Add a comment that this property is temporary and only used for the migration and showed be removed after some time

blumamir · 2024-12-22T13:10:06Z

odiglet/pkg/kube/runtime_details/inspection.go

@@ -72,7 +74,7 @@ func inspectRuntimesOfRunningPods(ctx context.Context, logger *logr.Logger, labe
 	return nil
 }

-func runtimeInspection(pods []corev1.Pod, ignoredContainers []string) ([]odigosv1.RuntimeDetailsByContainer, error) {
+func runtimeInspection(ctx context.Context, pods []corev1.Pod, ignoredContainers []string, criClient *criwrapper.CriClient) ([]odigosv1.RuntimeDetailsByContainer, error) {


ignoredContainers - make sure it still works after the refactor

tamirdavid1 force-pushed the odiglet-to-use-criwrapper-to-create-ic branch from 298dd4b to 613ebf4 Compare December 16, 2024 15:27

tamirdavid1 added 18 commits December 17, 2024 09:19

feat: initial commit - identify where env vars from

0d646fd

feat: test on odiglet main to be deleted

341dda6

feat: initial commit

10c8c79

chore: remove makefile addition + comment

5f879c1

chore: adjust comments and change how grpc connectec

59579f3

chore: remove unused ctx

c410796

feat: move cri connect out of go routine

a666ecd

fix: rebase

1f77020

fix: rebase

6ab4aaa

fix: rebase

68e3def

chore: remove newline

f4aa885

fix: run make go-mod-tidy

7a83e2b

fix: use-kube-manager-options

4b37c04

fix: remove not used observedWorkloadGeneration

3fef066

fix: remove odigos env var in the instrumentationconfig

a071d5d

feat: add test that PYTHONPATH exists in dockerfile for inventory

16a55ab

fix: change PYTHONPATH value

03bb7e7

feat: remove tests

b2978f7

tamirdavid1 force-pushed the odiglet-to-use-criwrapper-to-create-ic branch from 7311ce9 to b2978f7 Compare December 17, 2024 07:22

tamirdavid1 added 4 commits December 17, 2024 09:24

fix: rebase

f0259b0

fix: rebase conflicts

6df6b2a

fix: rebasing

8eaa3bd

fix: rebasing

722a49e

edeNFed reviewed Dec 17, 2024

View reviewed changes

RonFed reviewed Dec 17, 2024

View reviewed changes

api/odigos/v1alpha1/instrumentedapplication_types.go Outdated Show resolved Hide resolved

tamirdavid1 added 2 commits December 18, 2024 15:21

fix: based on PR comments

8fb407c

feat: remove not used runtime_details/utils.go anymore

1c14d62

tamirdavid1 force-pushed the odiglet-to-use-criwrapper-to-create-ic branch from 71831ff to 1c14d62 Compare December 22, 2024 09:51

tamirdavid1 added 6 commits December 22, 2024 12:05

fix: rebase and fix conclits

2a7b28c

fix: run make generate && make manifests

3262150

fix: remove no-cache from Makefile > added for tests proposes

57ff741

chore: rename EnvVarsFromDockerFile > EnvFromContainerRuntime

68967e0

chore: remove conflicts

0d1f240

chore: run make go-mod-tidy

ace5022

blumamir reviewed Dec 22, 2024

View reviewed changes

tamirdavid1 added 5 commits December 22, 2024 17:41

chore: based on comment use main ctx in criwrapper connect

19edc24

chore: following pr comments

bdde925

chore: validate extractContainerID returned value

3053147

chore: handle grpc close error if back

673c395

chore: rename warpper to client based on PR comments

8a410d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cOdiglet to use criwrapper to create ic #2015

cOdiglet to use criwrapper to create ic #2015

tamirdavid1 commented Dec 16, 2024

edeNFed Dec 17, 2024

edeNFed Dec 17, 2024

tamirdavid1 Dec 18, 2024

edeNFed Dec 17, 2024

edeNFed Dec 17, 2024

edeNFed Dec 17, 2024

blumamir Dec 19, 2024

blumamir Dec 22, 2024

tamirdavid1 Dec 22, 2024

blumamir Dec 22, 2024

blumamir Dec 22, 2024

blumamir Dec 22, 2024

blumamir Dec 22, 2024

blumamir Dec 22, 2024

		CriErrorMessage *string `json:"criErrorMessage,omitempty"`
		EnvVarsFromDockerFile []EnvVar `json:"envVarsFromDockerFile,omitempty"`

cOdiglet to use criwrapper to create ic #2015

Are you sure you want to change the base?

cOdiglet to use criwrapper to create ic #2015

Conversation

tamirdavid1 commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment