Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add config option for mount method hostpath or virtual device #2459

Merged
merged 21 commits into from
Feb 17, 2025

Conversation

blumamir
Copy link
Collaborator

This allows the user to choose how to apply the mount.

  • k8s-host-path will add volume to the pod with "host-path".
  • k8s-virtual-device will add the "instrumentation.odigos.io/generic" device to the resource part of relevant containers.

This allows control of how the mounting is achieved.

Future work: auto detect if hostpath may fail and automatically fallback to virtual device.

@@ -0,0 +1,94 @@
---
title: "Agent Mount Method"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about:

  1. Agent Injection Mode
  2. Instrumentation Mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think injection and instrumentaiton can mean many things. mount makes what this option is for explicit.

I am open to hearing alternatives

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought to abstract it for the user, basically the mount used to inject the agent for languages that needed.
I feel like Agent mount is not clear, but maybe lets hear another opinion

@@ -69,6 +70,17 @@ func (p *PodsWebhook) Default(ctx context.Context, obj runtime.Object) error {
return nil
}

odigosConfig, err := k8sutils.GetCurrentOdigosConfig(ctx, p.Client)
Copy link
Collaborator

@tamirdavid1 tamirdavid1 Feb 17, 2025

Choose a reason for hiding this comment

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

is it possible to take it to other func
verifyAgentInjectionModeSet so it will be more clear ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not only verifying the value but also fetching it to be used by injectOdigosToContainer.
Not sure I understand what you are suggesting.


If it is supported in the cluster, it is preferred to use over the VirtualDevice method which requires odiglet component to run.

#### Enabling VirtualDevice
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#### Enabling VirtualDevice
#### Enabling HostPath

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, fixed

Comment on lines +223 to +239

// amir 17 feb 2025, this is here only for migration.
// even if mount method is not device, we still need to inject the deprecated agent specific device
// while we remove them one by one
isGenericDevice := *distroMetadata.RuntimeAgent.Device == k8sconsts.OdigosGenericDeviceName
if mountMethod == common.K8sVirtualDeviceMountMethod || !isGenericDevice {
deviceName := *distroMetadata.RuntimeAgent.Device
// TODO: currently devices are composed with glibc as input for dotnet.
// as devices will soon converge to a single device, I am hardcoding the logic here,
// which will eventually be removed once dotnet specific devices are removed.
if containerConfig.DistroParams != nil {
libcType, ok := containerConfig.DistroParams[common.LibcTypeDistroParameterName]
if ok {
libcPrefix := ""
if libcType == string(common.Musl) {
libcPrefix = "musl-"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is getting hard to understand and follow,
we should probably refactor this a bit in follow ups

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once we remove the devices from all remaining distros, it will be straight forward

@blumamir
Copy link
Collaborator Author

Thanks @BenElferink addressed all your comments

@blumamir blumamir enabled auto-merge (squash) February 17, 2025 14:26
@blumamir blumamir disabled auto-merge February 17, 2025 14:39
@blumamir blumamir merged commit 442f266 into odigos-io:main Feb 17, 2025
42 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants