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

Related to #386. Wizard should bump ulimit #387

Merged
merged 2 commits into from
May 5, 2023
Merged

Conversation

smoy
Copy link
Contributor

@smoy smoy commented May 4, 2023

What changed?

  • ConfigurationWizard bumps ulimit

Rationale

  • ConfigurationWizard typically involves an import. IAMbic tries to concurrently fetch resources to bootstrap IAMbic templates from the cloud. It will use many file descriptors. (files and TCP connections). There can be argument IAMbic should still work within the available resources without crashing (but import slower). That design require more thought on how various plugin should interact with the host available resources. (file descriptors, graceful retry, configurable limit on concurrency). That will be another GitHub issue/PR.

How was it tested?

If it was manually verified, list the instructions for your reviewers to follow.

  • Unit Tests
  • Functional Tests
  • Manually Verified

I manually set ulimit -n 128, that should make it easily reproduce the issue with importing large AWS org. After this change, the wizard will bump ulimit implicitly.

@smoy smoy self-assigned this May 4, 2023
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: -16.48 ⚠️

Comparison is base (1049788) 85.33% compared to head (a2f93a2) 68.86%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #387       +/-   ##
===========================================
- Coverage   85.33%   68.86%   -16.48%     
===========================================
  Files          98       95        -3     
  Lines       10609    10290      -319     
===========================================
- Hits         9053     7086     -1967     
- Misses       1556     3204     +1648     
Flag Coverage Δ
functional_tests 67.02% <ø> (-0.18%) ⬇️
functional_tests_config_discovery 47.11% <ø> (-0.04%) ⬇️
unit_tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 62 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -351,7 +351,7 @@ def __init__(self, repo_dir: str):
self._default_region = None

asyncio.run(self.set_config_details())

check_and_update_resource_limit(self.config)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what actually makes the difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! As an alternative - what do you think about using this function in main.py under the "setup" function - this is the current pattern (we use it in import and git plan currently).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdaue2 There is a slight chicken-and-egg problem. The function takes a config but when setup is run, we may have a config to pass to the function. Wizard actually bootstrap config based on business logic in async def set_config_details(self):. That's the reasoning why I placed check_and_update_resource_limit(self.config) after self_config_details has run to ensure we can use self.config.

Copy link
Contributor

@mdaue2 mdaue2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@smoy smoy merged commit 1ec9679 into main May 5, 2023
@smoy smoy deleted the fix/macos-RLIMIT_NOFILE branch May 5, 2023 18:11
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.

2 participants