-
Notifications
You must be signed in to change notification settings - Fork 1k
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(ecs): add application name filter to findCluster #5168
Conversation
@deverton would love your eyes on this 😄 |
clusterMap = findClusters(clusterMap, credentials); | ||
Moniker moniker = MonikerHelper.applicationNameToMoniker(serverGroupName); | ||
log.info("App Name is: " + moniker.getApp()); | ||
clusterMap = findClusters(clusterMap, credentials, moniker.getApp()); |
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.
There used to be a to do here
// TODO - remove the application filter
Not sure the reason it was there, but maybe worth restoring that as well?
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.
Based on what we're seeing in the current release, I'm a bit hesitant on adding that TODO back
// Can't filter by application as there's not enough information in the serverGroupName | ||
clusterMap = findClusters(clusterMap, credentials); | ||
Moniker moniker = MonikerHelper.applicationNameToMoniker(serverGroupName); | ||
log.info("App Name is: " + moniker.getApp()); |
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.
Shouldn't this be debug level at most?
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.
Good catch! Updated!
f3ad4cf
to
9754f8d
Compare
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.
The server group name format doesn't change between the naming strategies. It's always constructed as app-stack-detail-v000
so this code should be equivalent to the previous String application = StringUtils.substringBefore(serverGroupName, "-");
Note to the release manager: This is a regression as it makes Spinnaker unusable for power users with a large number of resources. |
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.
🚀 @piradeepk @deverton you are awesome thank you for jumping on this!
@Mergifyio backport release-1.24.x |
(cherry picked from commit d43a0ea)
Command
|
As a part of the Moniker PR, logic was removed that enabled filtering on the App name. This allowed the server group info to be returned quickly. In release 1.24.0, users have been seeing timeouts, and this change will look to add back the previously removed application filter.
Current 1.24.0 Release Experience: Users with large amounts of AWS resources (such as services, task definitions, and tasks) are starting to see their server group/clusters view timing out with errors.
Testing done: As this is a performance improvement, it is difficult to add unit tests (load tests would be the best to test this scenario).
Closes: spinnaker/spinnaker#6245