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.
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.
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.
Shouldn't this be debug level at most?
There was a problem hiding this comment.
Good catch! Updated!
piradeepk
force-pushed
the
getServerGroupFix
branch
from
f3ad4cf
to
9754f8d
Compare
piradeepk
added
the
backport-candidate
|
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.
🚀 @piradeepk @deverton you are awesome thank you for jumping on this!
|
@Mergifyio backport release-1.24.x |
(cherry picked from commit d43a0ea)
|
Command
|
fieldju
removed
the
backport-candidate
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