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 too many project issue #176

Merged
merged 5 commits into from
Aug 23, 2018
Merged

Fix too many project issue #176

merged 5 commits into from
Aug 23, 2018

Conversation

grolu
Copy link
Contributor

@grolu grolu commented Aug 15, 2018

What this PR does / why we need it:
We face issues when a user (operator) has access to many projects and tries to fetch the all shoots list. This PR adds a special handling for users that have the operator role. These users can fetch all shoots at once (without namespace selector). This way, we do not need to make a requests for each namespace to the Kubernetes API.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Fixes an issue that leads to errors when a user (most likely with the operator role) has access to many projects and tries to fetch the all clusters list

.filter(({namespace}) => !!_.find(projectList, ['metadata.namespace', namespace]))
.map(async ({namespace, filter}) => {

if (_.get(_.head(namespaces), 'namespace') === '_all') {
Copy link
Contributor

Choose a reason for hiding this comment

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

extract to variable _.head(namespaces)


if (_.get(_.head(namespaces), 'namespace') === '_all') {
try {
const filter = _.get(_.head(namespaces), 'filter')
Copy link
Contributor

Choose a reason for hiding this comment

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

use variable

Copy link
Contributor

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

lgtm

@grolu grolu changed the title First implementation approach Fix too many project issue Aug 17, 2018
Copy link
Contributor

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

Using lodash the code could be simplified a little bit:

const batchEmitObjects = _
  .chain(batchEmitter)
  .bindKey('batchEmitObjects')
  .ary(2)
  .value() 
...
const hasIssue = item => filter !== 'issues' || shootHasIssue(item)
_
  .chain(shootList)
  .get('items')
  .filter(hasIssue)
  .groupBy('metadata.namespace')
  .forEach(batchEmitObjects)
  .commit()

Use short variant when mapping Objects:

const namespaces = _.map(projectList, (project) => _.get(project, 'metadata.namespace'))
_.map(namespacesAndFilters, ({namespace}) => namespace)

Shorter variant:

const namespaces = _.map(projectList, 'metadata.namespace')
_.map(namespacesAndFilters, 'namespace')

@grolu
Copy link
Contributor Author

grolu commented Aug 21, 2018

@holgerkoser I implemented and tested the changes proposed by you. In addition, I removed the filter statement(s) as I believe this is a leftover (before the apiserver set the unhealthy label).

Copy link
Contributor

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

lgtm

@grolu grolu merged commit 21809ff into master Aug 23, 2018
@grolu grolu deleted the too_many_requests branch August 24, 2018 14:47
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.

None yet

3 participants