Add user-specified instrumentation volume

[jnarezo]

Description:

Adds the ability for user to specify a custom volume for instrumentation / auto-injection to Instrumentation.spec.<lang>.volumeSizeLimit.

Adds validation to prevent Volume and VolumeSizeLimit being defined simultaneously to prevent overlapping behavior (ex. Do we use Volume.Size or VolumeSizeLimit?)

Link to tracking Issue(s):

• Resolves: Allow references to other volumes (eg. PVCs) for instrumention auto-inject #3267

Testing:

Unit tests for validation with different combinations of Volume/VolumeSizeLimit being defined and not defined

Documentation:

Updated API docs with new spec.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jnarezo / name: Joshua Narezo (9bed7cb, 0b64efe, 61785b6, bf3d1e8, 388b098, b88b3d0, 6811710, 29812e0, 1b029b9, 0fbd80d, 1f4d428, bd0ecb0, 717834e, e408e6a, f6be0cc, f404799, daae32a, 1b4a046, 5948165, 401a28e, 8a87082, b5ab3a7, 53cf4c6)

[swiatekm]

These changes look good to me overall. Some things missing before we can merge this PR:

• You need to add a changelog entry. Run make chlog-new and fill in the template.
• Run make precommit and fix any issues it brings up. Right now, the CI is failing because the bundle needs to be updated after changes to CRDs.
• We should check this volume mechanism in E2E tests. It's fine to modify an existing test.

[jnarezo]

These changes look good to me overall. Some things missing before we can merge this PR:

• You need to add a changelog entry. Run make chlog-new and fill in the template.
• Run make precommit and fix any issues it brings up. Right now, the CI is failing because the bundle needs to be updated after changes to CRDs.
• We should check this volume mechanism in E2E tests. It's fine to modify an existing test.

Thanks, got it working locally! Was using golang v1.23.x and pipeline was broken on me, so I downgraded.

[swiatekm]

The PR looks great, thanks for the contribution!

There's one big problem with it, and to be clear, this is really Kubernetes' fault. As it turns out, the OpenAPI spec for corev1.Volume is actually quite large, and we're including a copy of it for each language instrumentation. As a result, we're making the Instrumentation CRD much larger. There are consequences to this - for example, if the CRD is too big to fit in a K8s annotation (the limit is ~256kb), then it can't be applied using kubectl apply anymore. Your change pushes the Instrumentation CRD above that limit.

I'm not sure what we can do to alleviate this. We could make the Volume attribute common to all instrumentations. It would make it less flexible, but maybe that's ok? @jaronoff97 @pavolloffay WDYT?

[jnarezo]

Hey @swiatekm, sorry I dropped this for a bit, but I saw that there was no response on your question.

I redesigned the feature to accept a volumeClaimTemplate which is restricted to a generic ephemeral volume to match the solution proposed in the issue.

Although more restrictive that originally planned, I think this still provides about the functionality as an emptyDir and will still allow users with emptyDir restrictions to use a different type of volume, supported by storage drivers.

I think this should work for now. An alternative could be using an existing PVC, but there may be issues with files already existing from previous autoinstrumentation so I did not pursue it further.

What do you think?

[jnarezo]

Also, restricting the volume allowed me to get the manifest down to ~80KB.

[swiatekm]

@jnarezo this looks like a reasonable solution to me. Pending any more fundamental changes to the CRD, where we perhaps move all of these common values to the top level, this looks like a good compromise. WDYT @jaronoff97 @pavolloffay ?

Looks like you have some conflicts and failing e2e tests, could you have a look at that?

[jnarezo]

@jnarezo this looks like a reasonable solution to me. Pending any more fundamental changes to the CRD, where we perhaps move all of these common values to the top level, this looks like a good compromise. WDYT @jaronoff97 @pavolloffay ?

Looks like you have some conflicts and failing e2e tests, could you have a look at that?

Yeah! Sorry, looks like my e2e differed a bit locally. Thanks for working with me on this.

[jaronoff97]

just one thought, otherwise this looks good. thank you 🙇

[jaronoff97]

should we just deprecate this in favor of the volume claim template?

[swiatekm]

Let's open a separate issue for that, I don't think it should be within the scope of this PR.

[swiatekm]

#3382