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

Fix route name for OCP #917

Conversation

pavolloffay
Copy link
Member

Resolves https://issues.redhat.com/browse/TRACING-965

Fixes smoke test failures on OCP make e2e-tests-smoke caused probably by #891. Other related fixes #904 and #915

Signed-off-by: Pavol Loffay [email protected]

@jpkrohling
Copy link
Contributor

Like the original PR addressing the length limitation, this might indicate a problem deeper than what originally thought. Is the test failing only because it's trying to lookup a route based on a truncated name? If so, the test should use util.Truncate instead. I would actually prefer tests with big names, so that we have more coverage for such cases.

@pavolloffay
Copy link
Member Author

This is the failing log

Get https://my-jaeger-with-ingress-osdk-e2e-22fc72a5-db25-4707-876c-173229d49b42.apps.02-18-1582017833.devcluster.openshift.com/api/services: dial tcp: lookup my-jaeger-with-ingress-osdk-e2e-22fc72a5-db25-4707-876c-173229d49b42.apps.02-18-1582017833.devcluster.openshift.com: no such host

The instance name is the shortest part of the URL. Truncate should be called on the final URL but we do not have access to it.

This PR fixes the issue though.

@jpkrohling
Copy link
Contributor

The instance name is the shortest part of the URL. Truncate should be called on the final URL but we do not have access to it.

The hostname is made of name and namespace of the route, and we can control the size of the name. This is what the actual code does to prevent creating an invalid route:

var name string
if len(r.jaeger.Namespace) >= 63 {
// the route is doomed already, nothing we can do...
name = r.jaeger.Name
r.jaeger.Logger().WithField("name", name).Warn("the route's hostname will have more than 63 chars and will not be valid")
} else {
// -namespace is added to the host by OpenShift
name = util.Truncate(r.jaeger.Name, 62-len(r.jaeger.Namespace))
}

@rubenvp8510
Copy link
Collaborator

I had the same problem as I mentioned in #904

This PR seems to fix the issue.

@pavolloffay
Copy link
Member Author

To @jpkrohling note, the test code correctly gets the route and uses the hostname from the route to construct the URL for smoke test.

The problem might by that shortened URL is ending with - which is causing the failure.

@jpkrohling
Copy link
Contributor

The problem might by that shortened URL is ending with - which is causing the failure.

There's a util.DNSName() that could be applied to the name after the util.Truncate() operation.

// If the final name starts with "-", "a" is added as prefix. Similarly, if it ends with "-", "z" is added.
// Any char that is not [a-z0-9] is replaced by "-" or "a".
// Replacement character "a" is used only at the beginning or at the end of the name.
// The function does not change length of the string.
func DNSName(name string) string {
Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the behavior, the issue was that the function was changing the length of the name and in our codebase the DNSName was applied after the truncation.

There is no need to use a and z as replacement chars. The original name cannot be reconstructed.

@pavolloffay pavolloffay changed the title Shorten jaeger instance names in e2e tests Fix route name for OCP Feb 20, 2020
@@ -6,6 +6,8 @@ import (
"strings"
"testing"

"github.com/jaegertracing/jaeger-operator/pkg/util"
Copy link
Contributor

Choose a reason for hiding this comment

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

Import is out of order :/

Copy link
Member Author

Choose a reason for hiding this comment

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

goimports seems to be buggy. I will fix it manually.

@@ -6,6 +6,8 @@ import (
"strings"
"testing"

"github.com/jaegertracing/jaeger-operator/pkg/util"
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too

@pavolloffay pavolloffay force-pushed the shorten-jaeger-names-in-e2e-tests branch from f8220e7 to 3dd765d Compare February 20, 2020 15:34
@pavolloffay pavolloffay merged commit fd64f15 into jaegertracing:master Feb 20, 2020
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.

3 participants