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

Collector instance not created if name + namespace exceeds 63 due to how the labels are built #596

Closed
mimatache opened this issue Dec 2, 2021 · 8 comments · Fixed by #641

Comments

@mimatache
Copy link

When applying a CR where the name + namespace length is over 63 characters the operator fails to create the instances dues to a failure when setting the labels:

{"level":"error","ts":1638457753.904282,"logger":"controllers.OpenTelemetryCollector","msg":"failed to reconcile config maps","error":"failed to reconcile the expected configmaps: failed to create: ConfigMap \"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-collector\" is invalid: metadata.labels: Invalid value: \"nsxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\": must be no more than 63 characters","stacktrace":"github.com/open-telemetry/opentelemetry-operator/controllers.(*OpenTelemetryCollectorReconciler).Reconcile\n\t/workspace/controllers/opentelemetrycollector_controller.go:147\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.6/pkg/internal/controller/controller.go:298\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.6/pkg/internal/controller/controller.go:253\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.6/pkg/internal/controller/controller.go:214"}

As the Kubernetes standards state, a label can be no more than 63 characters. The same goes for instance names.

Since the names of items (CR & namespace) are allowed to be up to 63 characters, having the label set automatically to name + namespace seems like an issue with the operator.

As solutions I have 2 propositions:

  1. Split that label into 2 labels: 1 for name and 1 for namespace
  2. If the label size exceeds 63 characters create some kind of hash that is limited to 63 characters and replace the label with that.
@mmatache
Copy link
Contributor

mmatache commented Dec 3, 2021

I would like to fix this issue. Are there any preferences on the approach? Are the labels used in the operator anywhere for lookups?
I am partial to the second approach since it would remain backwards compatible to anybody that is currently using the operator.

@jpkrohling
Copy link
Member

To be clear, this is about the label app.kubernetes.io/instance, but we would have the same problem elsewhere if the name is too big. We have a routine for the Jaeger Operator to deal with this, perhaps we could have the same here:

https://github.com/jaegertracing/jaeger-operator/blob/91e3b69ee5c8761bbda9d3cf431400a73fc1112a/pkg/service/query.go#L69

@mmatache
Copy link
Contributor

mmatache commented Dec 7, 2021

@jpkrohling thank you for the information!
Is it ok to use the same functions Jaeger is using?
If yes, should we make a copy of the functions, or add a dependency to Jaeger and use them directly?
If no, is there any specific functionality you are looking for?

@jpkrohling
Copy link
Member

Yes, I think it's OK to use the same, but copy them. There's no need to add a dependency just because of that.

@mmatache
Copy link
Contributor

mmatache commented Dec 7, 2021

My thoughts exactly, but since this is not my project, I still wanted to confirm with you.

@mmatache
Copy link
Contributor

@jpkrohling I have found an issue in the jaeger trimming code which was not covered by any tests.
I have created #641 to handle this case. You should probably also look into whether this should be fixed in the Jaeger code as well.

@jpkrohling
Copy link
Member

Would you be open to creating an issue there as well? Bonus points if you can send in a PR, which should be similar to this one here.

@mmatache
Copy link
Contributor

@jpkrohling One hand scratches another 😄 : jaegertracing/jaeger-operator#1678

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 a pull request may close this issue.

3 participants