Skip to content

Conversation

@EddyMM
Copy link
Contributor

@EddyMM EddyMM commented Jul 7, 2022

Description of changes

  • While reading json files such as run_instances_overrides.json, the read operation may fail due to use of non-standard characters. This is difficult to debug since at the moment we use a default value when there is an error and furthermore the error is not logged.
  • This commit logs the exception in situations where we fallback to a default value.

Tests

  • Manually tested by using a run_instances_overrides.json file with non-standard characters

Before:

2022-07-07 10:28:23,967 - [slurm_plugin.clustermgtd:_get_config] - INFO - Reading /etc/parallelcluster/slurm_plugin/parallelcluster_clustermgtd.conf
2022-07-07 10:28:23,968 - [slurm_plugin.common:read_json] - INFO - Unable to read file '/opt/slurm/etc/pcluster/run_instances_overrides.json'. Using default: {}

After:

2022-07-07 10:33:56,653 - [slurm_plugin.clustermgtd:_get_config] - INFO - Reading /etc/parallelcluster/slurm_plugin/parallelcluster_clustermgtd.conf
2022-07-07 10:33:56,654 - [slurm_plugin.common:read_json] - INFO - Unable to read file '/opt/slurm/etc/pcluster/run_instances_overrides.json' due to an exception: Expecting property name enclosed in double quotes: line 2 column 1 (char 2). Using default: {}

References

  • Link to related PRs in other packages (i.e. cookbook, node).
  • Link to documentation useful to understand the changes.

Checklist

  • Make sure you are pointing to the right branch and add a label in the PR title (i.e. 2.x vs 3.x)
  • Check all commits' messages are clear, describing what and why vs how.
  • Make sure to have added unit tests or integration tests to cover the new/modified code.
  • Check if documentation is impacted by this change.

Please review the guidelines for contributing and Pull Request Instructions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #427 (5a007dc) into develop (6aebaa8) will increase coverage by 0.34%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #427      +/-   ##
===========================================
+ Coverage    81.57%   81.91%   +0.34%     
===========================================
  Files           12       12              
  Lines         1731     1731              
===========================================
+ Hits          1412     1418       +6     
+ Misses         319      313       -6     
Impacted Files Coverage Δ
src/slurm_plugin/common.py 69.81% <0.00%> (+11.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aebaa8...5a007dc. Read the comment docs.

@EddyMM EddyMM force-pushed the wip/read_json_exceptions branch 2 times, most recently from 760c582 to 577c5da Compare July 7, 2022 11:22
While reading json files such as `run_instances_overrides.json`, the read operation may fail due to use of non-standard characters. This is difficult to debug since at the moment we use a default value when there is an error and furthermore the error is not logged.
This commit logs the exception in situations where we fallback to a default value.
@EddyMM EddyMM force-pushed the wip/read_json_exceptions branch from 577c5da to 5a007dc Compare July 7, 2022 11:30
@EddyMM EddyMM marked this pull request as ready for review July 7, 2022 13:12
@EddyMM EddyMM requested a review from a team as a code owner July 7, 2022 13:12
@EddyMM EddyMM merged commit 4aa9b10 into aws:develop Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants