-
Notifications
You must be signed in to change notification settings - Fork 411
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
Instrumentations support select #2778
base: main
Are you sure you want to change the base?
Instrumentations support select #2778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to see a E2E test for this behaviour.
I'll add it later. |
tests/e2e-instrumentation/instrumentation-select/01-install-app-select.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minus some unnecessary changes from E2E tests.
bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Mikołaj Świątek <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we're not following the standard semantics of v1.LabelSelector:
A label selector is a label query over a set of resources. The result of matchLabels and matchExpressions are ANDed. An empty label selector matches all objects. A null label selector matches no objects.
Whereas the way we have this implemented, null
means "everything" and there's no way to select nothing. Could we switch to the standard semantics? I don't think there's a way to get kubebuilder to set this default for us, so we'd need to do it in the defaulting webhook, but we'd get to skip the conditional instructions in the Pod mutator. What do you think?
Good suggestion. I agree with you.
If we want to implement this semantics, the current test case will need to add empty selectors to pass the e2e test. This will change the default behavior; is this necessary? |
You can add the following annotation to the field: for _, ins := range otelInsts.Items {
labelSelector, err := v1.LabelSelectorAsSelector(ins.Spec.Selector)
if err != nil {
return nil, err
}
set := labels.Set(pod.GetLabels())
if labelSelector.Matches(set) {
availableInstrument = append(availableInstrument, ins)
}
} and all the e2e tests passed. The only potential problem here, is that this default value is only reflected in the CRD itself. So if a user were to upgrade the operator without upgrading their Instrumentation CRD, the field would be |
Are there any new updates now? |
@swiatekm-sumo i think it may be okay for the instrumentations to select none by default, given that's technically the current behavior right? |
I don't think that's the case? Looking at the code being modified in this PR, it just picks the instrumentation defined in the namespace if the annotation isn't specific. Setting the default to Incidentally, we should document that this selector only applies to this situation. As is, if the selector doesn't fit the Pod, and the Pod asks for a specific Instrumentation via the annotation, it will still get it. |
# Conflicts: # bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml
I prefer this approach; maybe we can finish the current issue first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok keeping this as is, but we should document how we diverge from standard LabelSelector behavior.
Co-authored-by: Mikołaj Świątek <[email protected]>
// Unlike standard label selectors, `nil` means `everything`, and this is also the default. | ||
// This may change in a future CRD version. | ||
// +optional | ||
Selector *metav1.LabelSelector `json:"selector,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all pods from all namespaces matched?
What is the precedence order when annotation instrumentation.opentelemetry.io/
is also present on the pod/namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this feature only comes into play when the annotation is not set and we try to pick an Instrumentation from the namespace the Pod is in. The annotation has precedence. But we should document this behaviour somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this feature only comes into play when the annotation is not set and we try to pick an Instrumentation from the namespace the Pod is in
opentelemetry-operator/pkg/instrumentation/podmutator.go
Lines 368 to 377 in c65629b
func (pm *instPodMutator) getInstrumentationInstance(ctx context.Context, ns corev1.Namespace, pod corev1.Pod, instAnnotation string) (*v1alpha1.Instrumentation, error) { | |
instValue := annotationValue(ns.ObjectMeta, pod.ObjectMeta, instAnnotation) | |
if len(instValue) == 0 || strings.EqualFold(instValue, "false") { | |
return nil, nil | |
} | |
if strings.EqualFold(instValue, "true") { | |
return pm.selectInstrumentationInstanceFromNamespace(ctx, ns) | |
} |
Did I understand it wrong? I think the premise for this feature to take effect is that the instrumentation.opentelemetry.io/inject-xx
is equal to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant to say was that this mechanism only applies if the annotation doesn't specify which instrumentation should be used. Apologies for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; I understand that the documentation has been supplemented.
@swiatekm-sumo The E2E test failed, but I can pass it locally, any suggestions? |
Can you try recreating the test cluster locally to see if tests still pass? It's possible you're using an older docker image in tests. |
Thanks; I have changed the e2e test. |
# Conflicts: # apis/v1alpha1/zz_generated.deepcopy.go
@crossoverJie I think you're running into trouble merging changes in generated manifests. I suggest running |
Thanks, done. |
@crossoverJie can you rebase on main? That should get rid of the EasyCLA complaint. |
Thanks for your reminder, done. |
I don't get why EasyCLA complains about my email address. Feel free to just drop the |
kind: Pod | ||
metadata: | ||
annotations: | ||
instrumentation.opentelemetry.io/inject-java: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the use-case to have the Selector
in the CR while the inject annotation is still needed.
What is the use-case this feature solves? Why do we need to maintain two approaches to solve the same use-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case is to test that when there are multiple Instrumentation
(they all use Selector
), the Selector
feature can still work as expected.
/easycla Edit: FYI, the problem was communitybridge/easycla#4329. |
Nice PR. Is this blocked on some particular change needed? |
@jackshirazi we're discussing the design of this in the linked issue #2744 |
Description:
Support select.
Link to tracking Issue(s): #2744
Testing:
Documentation: