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

HPCC-27051 Create a Sasha service to clean up post-mortem files #19579

Closed
wants to merge 0 commits into from

Conversation

streeterd
Copy link
Contributor

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@streeterd streeterd requested a review from Copilot March 3, 2025 17:08
Copy link

github-actions bot commented Mar 3, 2025

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-27051

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request adds a new configuration section for a Sasha service to clean up post-mortem files via Helm.

  • Introduces a "debug-housekeeping" section under the sasha configuration.
  • Sets default properties (plane and user) with additional options commented out for future adjustments.

Reviewed Changes

File Description
helm/hpcc/values.yaml Adds and configures "debug-housekeeping" for the Sasha post-mortem cleanup.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

@@ -513,6 +513,14 @@ sasha:
#switchMinTime: 1
#queues: "*"

debug-housekeeping: {} # NB: no properties defined, use defaults
Copy link
Preview

Copilot AI Mar 3, 2025

Choose a reason for hiding this comment

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

The inline empty map '{}' is declared for 'debug-housekeeping' and then additional properties are defined in block style. Consider removing '{}' to ensure the block values are correctly interpreted.

Suggested change
debug-housekeeping: {} # NB: no properties defined, use defaults
debug-housekeeping:

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@streeterd streeterd requested a review from Copilot March 4, 2025 09:42
@streeterd streeterd self-assigned this Mar 4, 2025

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a new debug-housekeeping configuration block for the Sasha service to enable cleanup of post-mortem files. Key changes include:

  • Adding a debug-housekeeping block with default and commented properties in helm/hpcc/values.yaml.
  • Propagating the debug-housekeeping configuration across multiple test YAML files (values-hpcc1.yaml, values-hpcc2.yaml, multicertdomains.yaml, resourced.yaml, and resourced2.yaml).

Reviewed Changes

File Description
helm/hpcc/values.yaml Added a debug-housekeeping block with a note to use defaults.
testing/helm/tests/values-hpcc2.yaml Included debug-housekeeping with disabled option.
testing/helm/tests/multicertdomains.yaml Included debug-housekeeping with disabled option.
testing/helm/tests/values-hpcc1.yaml Included debug-housekeeping with disabled option.
testing/helm/tests/resourced.yaml Added debug-housekeeping block with commented defaults and resources.
testing/helm/tests/resourced2.yaml Added debug-housekeeping block with commented defaults and resources.

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

helm/hpcc/values.yaml:516

  • The indentation of the properties under the debug-housekeeping key appears inconsistent; please verify that when uncommented, the properties are correctly nested.
  debug-housekeeping: {} # NB: no properties defined, use defaults

testing/helm/tests/resourced.yaml:514

  • [nitpick] Consider removing or clearly delineating the commented-out default properties in the debug-housekeeping block to avoid potential confusion during maintenance.
  debug-housekeeping:
@streeterd streeterd requested a review from Copilot March 4, 2025 10:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request adds a new debug‐housekeeping configuration to the Sasha service intended for cleaning up post‐mortem files. It updates the Helm chart values and test configuration files to introduce the new setting with different defaults in production versus test environments.

  • Added a debug-housekeeping block with commented default properties in helm/hpcc/values.yaml.
  • Inserted explicit disabled settings for debug-housekeeping in various test YAML files.
  • Extended debug-housekeeping in some tests to include resource configuration options.

Reviewed Changes

File Description
helm/hpcc/values.yaml Added debug-housekeeping with an empty object and commented defaults.
testing/helm/tests/multicertdomains.yaml Added debug-housekeeping with disabled set to true.
testing/helm/tests/values-hpcc1.yaml Added debug-housekeeping with disabled set to true.
testing/helm/tests/values-hpcc2.yaml Added debug-housekeeping with disabled set to true.
testing/helm/tests/resourced.yaml Introduced debug-housekeeping with commented defaults and resource settings.
testing/helm/tests/resourced2.yaml Introduced debug-housekeeping with commented defaults and resource settings.

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Comment on lines 516 to 517
debug-housekeeping: {} # NB: no properties defined, use defaults
#disabled: true
Copy link
Preview

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

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

The production values define debug-housekeeping as an empty object with commented defaults, while test configs explicitly disable it; this inconsistency may lead to unexpected behavior. Consider standardizing the default settings or explicitly documenting the differences between production and test environments.

Suggested change
debug-housekeeping: {} # NB: no properties defined, use defaults
#disabled: true
debug-housekeeping:
disabled: true

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Comment on lines 53 to 52
disabled: true

Copy link
Preview

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

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

The debug-housekeeping key is introduced with 'disabled: true' in this test file, which does not match the production configuration. Aligning these default settings can help avoid configuration drift between environments.

Suggested change
disabled: true
disabled: false

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -510,6 +510,16 @@ sasha:
resources:
cpu: "1"
memory: "4Gi"

debug-housekeeping:
Copy link
Preview

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

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

This instance of debug-housekeeping includes additional nested resource settings, differing from other configurations. Consider unifying the configuration across files to prevent confusion.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -512,6 +512,16 @@ sasha:
resources:
cpu: "1"
memory: "4Gi"

debug-housekeeping:
Copy link
Preview

Copilot AI Mar 4, 2025

Choose a reason for hiding this comment

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

The debug-housekeeping block here also contains resource settings that differ from the defaults in production. Standardize the configuration or clearly document the different behaviors between environments.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@streeterd streeterd requested a review from Copilot March 4, 2025 13:43

Choose a reason for hiding this comment

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

PR Overview

This PR implements configuration changes to support a Sasha service that cleans up post-mortem files by introducing a new debug-housekeeping configuration. The changes add debug-housekeeping settings to various Helm configuration files for both production and testing environments.

  • Added a debug-housekeeping block with placeholder parameters in helm/hpcc/values.yaml.
  • Enabled debug-housekeeping (set to disabled) in testing configuration files.
  • Added resource allocation settings under debug-housekeeping in resourced and resourced2 test files.

Reviewed Changes

File Description
helm/hpcc/values.yaml Introduces a new debug-housekeeping block with default parameters commented out.
testing/helm/tests/values-hpcc2.yaml Enables debug-housekeeping by setting disabled to true.
testing/helm/tests/values-hpcc1.yaml Enables debug-housekeeping by setting disabled to true.
testing/helm/tests/multicertdomains.yaml Enables debug-housekeeping by setting disabled to true.
testing/helm/tests/resourced.yaml Adds a debug-housekeeping block with resource allocations and commented defaults.
testing/helm/tests/resourced2.yaml Similar addition of debug-housekeeping with resource allocations and commented defaults.

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

@streeterd streeterd requested a review from Copilot March 4, 2025 14:39

Choose a reason for hiding this comment

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

PR Overview

This PR adds a new configuration option ("debug-housekeeping") to enable additional housekeeping behaviors for the Sasha service, intended to help clean up post-mortem files. Key changes include:

  • Adding a "debug-housekeeping" section with default (empty or disabled) configuration in helm/hpcc/values.yaml.
  • Introducing similar "debug-housekeeping" blocks in multiple test files (values-hpcc1.yaml, multicertdomains.yaml, values-hpcc2.yaml, resourced2.yaml, resourced.yaml) for consistency.

Reviewed Changes

File Description
helm/hpcc/values.yaml Added a "debug-housekeeping" option with commented default properties.
testing/helm/tests/values-hpcc1.yaml Added a "debug-housekeeping" block with "disabled: true" setting.
testing/helm/tests/multicertdomains.yaml Similar addition of a "debug-housekeeping" block with "disabled: true".
testing/helm/tests/values-hpcc2.yaml Similar addition of a "debug-housekeeping" block with "disabled: true".
testing/helm/tests/resourced2.yaml Added a "debug-housekeeping" block including a nested "resources" block.
testing/helm/tests/resourced.yaml Added a "debug-housekeeping" block including a nested "resources" block.

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

helm/hpcc/values.yaml:516

  • Ensure that the commented configuration lines below maintain consistent indentation if uncommented to avoid YAML parsing issues.
debug-housekeeping: {} # NB: no properties defined, use defaults

testing/helm/tests/resourced2.yaml:522

  • Confirm that 'resources' is intentionally nested under 'debug-housekeeping' in this file and that its usage is consistent with other configurations.
resources:

testing/helm/tests/resourced.yaml:520

  • Confirm that 'resources' is intentionally nested under 'debug-housekeeping' in this file and that its usage is consistent with other configurations.
resources:
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.

1 participant