Skip to content

feat: Increase ulimit in unix environment#416

Merged
spangenberg merged 1 commit intomainfrom
feat/ulimit
Jan 18, 2022
Merged

feat: Increase ulimit in unix environment#416
spangenberg merged 1 commit intomainfrom
feat/ulimit

Conversation

@spangenberg
Copy link
Copy Markdown
Contributor

The ulimit in Unix environment will be increased automatically.
Currently, we fail a lot of runs due to low limits.
In case of an error, we just log and continue.

@spangenberg spangenberg requested a review from roneli January 17, 2022 16:33
@spangenberg spangenberg self-assigned this Jan 17, 2022
@spangenberg spangenberg changed the title Increase ulimit in unix environment feat: Increase ulimit in unix environment Jan 17, 2022
@github-actions github-actions bot added feat and removed enhancement labels Jan 17, 2022
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Great. I think this should work - I've one questions regarding initialization and if you were able to test it that it also sets the ulimit for child process (I think it should but would double verify it manually).

const ulimitUnix uint64 = 16384

func init() {
fileDescriptorF = checkAndSetUlimitUnix
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why we can call checkAndSetUlimitUnix directly ? This should also remove the need for this check unless Im missing some edge-case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It should fail on windows if I'm not completely mistaken. Since syscall is not available.

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

ok I see now regarding the initialization, it is because of the build tags. Can you add a small comment for that.

@yevgenypats
Copy link
Copy Markdown
Contributor

@spangenberg can you only fix that the commit will be the same as the pr title (it's when there is one commit, GitHub is using the commit instead of the PR Title and it doesn't have the feat prefix so it's "unconventional")

@spangenberg
Copy link
Copy Markdown
Contributor Author

Sure, will take care about that and the extended manual testing.

@spangenberg
Copy link
Copy Markdown
Contributor Author

The PR was manually tested on MacOS, Linux and Windows.

@spangenberg spangenberg merged commit 7440844 into main Jan 18, 2022
@spangenberg spangenberg deleted the feat/ulimit branch January 18, 2022 13:27
TinLe pushed a commit to TinLe/cloudquery that referenced this pull request Jan 19, 2022
* upstream/main:
  chore: Synced local '.github/workflows/' with remote 'workflows/common' (cloudquery#421)
  fix: Don't show telemetry notice when it's not enabled (cloudquery#418)
  feat: Increase ulimit in unix environment (cloudquery#416)
  feat: Support build-schema for all providers in config (cloudquery#414)
  fix: Fetch summary fixes (cloudquery#417)
  feat: Added store fetch summary routine (cloudquery#356)
  chore: Synced file(s) with cloudquery/.github (cloudquery#415)
  feat: Expose max_parallel_resource_fetch_limit (cloudquery#413)
  feat: Adjust log messages when .cq dir is absent (cloudquery#411)
  fix: Adjust policy describe message (cloudquery#409)
  fix: Exit with 1 on policy error (cloudquery#412)
@cq-bot cq-bot mentioned this pull request Aug 9, 2022
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
Co-authored-by: Renovate Bot <renovate@whitesourcesoftware.com>
erezrokah pushed a commit that referenced this pull request Aug 14, 2022
Add Azure Front Door CDN resource
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