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.

Sign up for GitHub

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

Sort order of ports returned to fix flaky tests #1003

Merged
merged 6 commits into from
Aug 3, 2022
Merged

Sort order of ports returned to fix flaky tests #1003

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc6c3c7
Sort order of ports returned to fix flaky tests
kevinearls Jul 25, 2022
b0f3b9d
Verify type of receiver name
kevinearls Jul 25, 2022
7d22f3b
Sort ports too before returning
kevinearls Jul 27, 2022
5305fc2
Add unit test
kevinearls Aug 1, 2022
0b72f8d
respond to comments
kevinearls Aug 3, 2022
75ce40f
fix nits
kevinearls Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions pkg/collector/adapters/config_to_ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package adapters

import (
"errors"
"sort"

"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -60,28 +61,34 @@ func ConfigToReceiverPorts(logger logr.Logger, config map[interface{}]interface{
return nil, ErrReceiversNotAMap
}

sortedNames := make([]string, 0, len(receivers))
Copy link
Member

Choose a reason for hiding this comment

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

@kevinearls wouldn't it be cleaner to sort ports at the end of the function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but I'm not sure it necessary. Originally when I saw this problem it occurred because we were getting receivers in the wrong order. Sorting them alphabetically by name is fairly simple.

The ports, on the other hand, are in an array of corev1.ServicePort. That does not implement methods needed to feed it to sort.Sort(), so sorting those would be non-trivial.

Copy link
Member

@pavolloffay pavolloffay Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What is the root cause of the problem? That the ports are not sorted or receivers? We should use the least error-prone approach that will guarantee always the same result.

Take a look at sort.Slice:

	sort.Slice(ports, func(i, j int) bool {
		return ports[i].Name < ports[j].Name
	})

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem I had was caused by the receivers being in the wrong order, not the ports for a receiver.

Copy link
Member

@pavolloffay pavolloffay Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In other words, the implementation in this PR will break again if ReceiverParser.Ports() returns randomly sorted ports. In fact right now that API does not guarantee the order.

Also, there could be a unit test checking the functionality - or change the existing tests to assume the order of ports matters.

for name := range receivers {
sortedNames = append(sortedNames, name.(string))
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to check the type just for safety here?

Suggested change
sortedNames = append(sortedNames, name.(string))
v, ok := name.(string)
if !ok {
logger.Warn("receiver does not match type, want: %s, got: %T", "string", name)
continue
}
sortedNames = append(sortedNames, v)

}
sort.Strings(sortedNames)

ports := []corev1.ServicePort{}
for key, val := range receivers {
for _, name := range sortedNames {
// This check will pass only the enabled receivers,
// then only the related ports will be opened.
if !recEnabled[key] {
if !recEnabled[name] {
continue
}
receiver, ok := val.(map[interface{}]interface{})

receiver, ok := receivers[name].(map[interface{}]interface{})
if !ok {
logger.Info("receiver doesn't seem to be a map of properties", "receiver", key)
logger.Info("receiver doesn't seem to be a map of properties", "receiver", name)
receiver = map[interface{}]interface{}{}
}

rcvrName := key.(string)
rcvrParser := parser.For(logger, rcvrName, receiver)
rcvrParser := parser.For(logger, name, receiver)

rcvrPorts, err := rcvrParser.Ports()
if err != nil {
// should we break the process and return an error, or just ignore this faulty parser
// and let the other parsers add their ports to the service? right now, the best
// option seems to be to log the failures and move on, instead of failing them all
logger.Error(err, "parser for '%s' has returned an error: %w", rcvrName, err)
logger.Error(err, "parser for '%s' has returned an error: %w", name, err)
continue
}

Expand Down