feat(kubernetes): Add support for excluding event loading on manifest…

[mtweten]

…s retrieval

This change allows optionally skipping kubernetes event retrieval when retrieving manifests.

When live manifest mode is enabled, scenarios that result in a lot of manifests being loaded will put a lot of extra pressure on the kubernetes API server due to event loading.

Specifically, we ran into this scenario when we had a lot of deployment pipelines running concurrently, and each deployment involved a decent number of manifests (~30). In Orca, the WaitForManifestStableTask (when live manifest mode is enabled) iterates through the manifests involved in the deployment and calls the retrieve manifest API for each one, which causes a lot of full event loads to occur, which tanks (in our case) the kubernetes API server. However, WaitForManifestStableTask does not actually need any event information to do its job, so in that particular case we can skip loading events entirely.

See spinnaker/orca#2997 for the corresponding Orca change.

[ezimanyi]

This is a great change, thanks for this! I added a few inline comments---the biggest being that I think this will be simpler if we just support a single version of each call on the interface:

• getManifest which requires passing in inlcudeEvents (as you've done, so just removing the old signature and updating the one place it's used)
• getClusterAndSortAscending which never returns events

[mtweten]

Thanks for the review Eric!

Addressed your comments, I agree with simplifying the number of APIs - I was just initially a little worried and wanted to do it in a super passive way so I didn't inadvertently break anything.

Only thing I wanted to be sure of was the change in getDynamicManifestFromCluster - essentially we're just saying the contract for that API call is that events aren't returned, and if that functionality is eventually needed it can be added in the future?

[ezimanyi]

Looks good!

Yes, that's exactly what I meant for getDynamicManifestFromCluster. I'm not sure I see a use case for us ever needing events from that function, so it seems easier to just remove them entirely and if such a use case does arise whoever needs them can add a boolean to that function too.