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

Fixes the metric output file becoming corrupted #393

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Conversation

ribalba
Copy link
Member

@ribalba ribalba commented Jul 20, 2023

No description provided.

@ribalba ribalba requested a review from ArneTR July 20, 2023 09:49
time.sleep(1)
count += 1
if count >= 5:
raise RuntimeError('powermetrics is not stopping. Please kill with `kill -9 ...`.')
Copy link
Member

Choose a reason for hiding this comment

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

If it needs to be anyway killed with kill -9, why don't we do it?

This would then not only leave the output file corrupted, but also the state of the GMT, as you might have two instances of powermetrics running. correct?

Copy link
Member

Choose a reason for hiding this comment

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

Has been stale for a week now. If you do not have any further input I would vouch for changing the code to be less manual and more automatic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally. Is on my todo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I now remember why I did it like this. kill -9 needs to be called as root and I was hesitant to add another line to sudoers. No real reason why to be honest. Added it.

@ArneTR ArneTR changed the base branch from dev to main July 25, 2023 10:24
@ribalba
Copy link
Member Author

ribalba commented Aug 2, 2023

@ArneTR ready to merge IMHO

@ArneTR
Copy link
Member

ArneTR commented Aug 3, 2023

LGTM. I will merge it in and report back. Since the error happens only transiently I cannot really check it on the spot. Will report if I notice furhter errors.

@ArneTR ArneTR merged commit 4dd5897 into main Aug 3, 2023
@ArneTR ArneTR deleted the fixes-powermetrics-bug branch August 3, 2023 14:20
ArneTR added a commit that referenced this pull request Aug 3, 2023
* main:
  jobs.py now appends date
  Fixes the error on mac on which /tmp is a symlink to /private/tmp (#410)
  Fixes the metric output file becoming corrupted (#393)
ArneTR added a commit that referenced this pull request Aug 4, 2023
* main:
  Ignore filesystem paths
  Bugfix: Phases ordering was wrong way round
  eco-ci will now post the energy reading in the PR conversation (#397)
  SCI metric (#412)
  Fixes the github security errors (#411)
  API does not block returning machines, but will now return if they are available or not
  Enables empty services in the usage_scenario (#409)
  wrong import
  jobs.py now appends date
  Fixes the error on mac on which /tmp is a symlink to /private/tmp (#410)
  Fixes the metric output file becoming corrupted (#393)
  Only available machines may be listed
  Index.js now can filter by repo and filename (#408)
  Python requirements are now freshly updated with every install
  Gunicorn container now on python3-slim (Debian) instead of Ubuntu 22.04
ArneTR added a commit that referenced this pull request Aug 8, 2023
* main: (42 commits)
  Added exception handling to client.py
  Fix and better display of SCI values in badged and dashboard
  Increased the waiting time for powermetrics to shut down
  Bugfix for non-string types replacement
  Docker prune is now the default for jobs.py
  Bump psycopg[binary] from 3.1.9 to 3.1.10 (#413)
  Bump fastapi from 0.100.1 to 0.101.0 (#414)
  Bump orjson from 3.9.2 to 3.9.3 (#415)
  Ignore filesystem paths
  Bugfix: Phases ordering was wrong way round
  eco-ci will now post the energy reading in the PR conversation (#397)
  SCI metric (#412)
  Fixes the github security errors (#411)
  API does not block returning machines, but will now return if they are available or not
  Enables empty services in the usage_scenario (#409)
  wrong import
  jobs.py now appends date
  Fixes the error on mac on which /tmp is a symlink to /private/tmp (#410)
  Fixes the metric output file becoming corrupted (#393)
  Only available machines may be listed
  ...
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