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

CRAYSAT-1890: Fix sat.waiting logger #247

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

haasken-hpe
Copy link
Contributor

@haasken-hpe haasken-hpe commented Jul 31, 2024

Summary and Scope

The LOGGER was incorrectly created with __file__ as the logger name instead of __name__. As a result, it was not included under the sat package logger's hierarchy, and the expected log handlers were not applied to messages logged by this module. Fix this by specifying __name__ instead, so the logger has the name sat.waiting, which is under the sat logger.

Issues and Related PRs

Testing

Tested on:

  • rocket

Test description:

Ran sat bootsys shutdown --stage capture-state followed by sat bootsys boot --stage k8s-check --k8s-timeout 10 to force a
Waiter subclass to timeout, so I could observe the error log message.
As expected, it timed out waiting on new pods that were created by K8s
Jobs to reach a running or completed state, and it emitted the correctly
formatted log message of the form:

ERROR: Waiting for condition "..." timed out after 10 seconds

Risks and Mitigations

Very low risk logging fix.

Pull Request Checklist

  • Version number(s) incremented, if applicable
  • Copyrights updated
  • License file intact
  • Target branch correct
  • CHANGELOG.md updated
  • Testing is appropriate and complete, if applicable
  • HPC Product Announcement prepared, if applicable

Copy link
Contributor

@annapoorna-s-alt annapoorna-s-alt left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@shivaprasad-metimath shivaprasad-metimath left a comment

Choose a reason for hiding this comment

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

looks good

@haasken-hpe
Copy link
Contributor Author

Thank you both! I'll wait to merge this until after @annapoorna-s-alt has merged the feature/CRAYSAT-1740 branch to main, just so you don't have to deal with more changelog conflicts.

The `LOGGER` was incorrectly created with `__file__` as the logger name
instead of `__name__`. As a result, it was not included under the `sat`
package logger's hierarchy, and the expected log handlers were not
applied to messages logged by this module. Fix this by specifying
`__name__` instead, so the logger has the name `sat.waiting`, which is
under the `sat` logger.

Test Description:
Ran `sat bootsys shutdown --stage capture-state` followed by `sat
bootsys boot --stage k8s-check --k8s-timeout 10` to force a
`Waiter` subclass to timeout, so I could observe the error log message.
As expected, it timed out waiting on new pods that were created by K8s
Jobs to reach a running or completed state, and it emitted the correctly
formatted log message of the form:

```
ERROR: Waiting for condition "..." timed out after 10 seconds
```
@haasken-hpe haasken-hpe force-pushed the CRAYSAT-1890-fix-waiting-logger branch from 56cd776 to 4cd4178 Compare August 2, 2024 22:13
@haasken-hpe
Copy link
Contributor Author

I just rebased this on the latest main branch after merging @annapoorna-s-alt 's PR to merge the feature/CRAYSAT-1740 branch to main. As a result, I changed the version in the changelog to 3.29.1.

@haasken-hpe haasken-hpe merged commit 893c8a8 into main Aug 2, 2024
3 checks passed
@haasken-hpe haasken-hpe deleted the CRAYSAT-1890-fix-waiting-logger branch August 2, 2024 22:20
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.

3 participants