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

fix cleanup of resources #81

Merged
merged 3 commits into from
Jan 3, 2024
Merged

fix cleanup of resources #81

merged 3 commits into from
Jan 3, 2024

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jan 2, 2024

Type

bug_fix, enhancement


Description

  • The cleanup interval for resources is now configurable through an environment variable. If the environment variable is not set or cannot be parsed, a default value of 24 hours is used.
  • The cleanup handler's logging has been improved to include the cleanup interval when a cleanup task starts and to log deletions at the debug level.
  • The metadata loading function has been updated to also load labels from the metadata file.
  • The function to delete resources by template hash or wlid has been updated to look for the template hash in the labels instead of the annotations.
  • The list of workloads to cleanup has been reduced to only include cronjob, daemonset, deployment, job, pod, replicaset, and statefulset.

PR changes walkthrough

Relevant files                                                                                                                                 
Configuration changes
1 files
main.go                                                                                                         
    main.go

    The cleanup interval for resources is now configurable
    through an environment variable. If the environment variable
    is not set or cannot be parsed, a default value of 24 hours
    is used. The cleanup handler is updated to use the new
    configurable interval.

+7/-1
Bug_fix
1 files
cleanup.go                                                                                                   
    pkg/cleanup/cleanup.go

    The cleanup handler's logging has been improved to include
    the cleanup interval when a cleanup task starts and to log
    deletions at the debug level. The metadata loading function
    has been updated to also load labels from the metadata file.
    The function to delete resources by template hash or wlid
    has been updated to look for the template hash in the labels
    instead of the annotations. Some unused code has been
    removed.

+9/-6
Enhancement
1 files
discovery.go                                                                                               
    pkg/cleanup/discovery.go

    The list of workloads to cleanup has been reduced to only
    include cronjob, daemonset, deployment, job, pod,
    replicaset, and statefulset. The functions to fetch wlids
    from running workloads and to fetch instance IDs and image
    IDs and replicas from running pods have been updated to
    improve logging and remove unused code.

+5/-20

Signed-off-by: Matthias Bertschy <[email protected]>
@matthyx matthyx requested a review from dwertent January 2, 2024 10:30
Copy link

github-actions bot commented Jan 2, 2024

Summary:

  • License scan: failure
  • Credentials scan: success
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

@matthyx matthyx marked this pull request as ready for review January 2, 2024 11:11
@codiumai-pr-agent-free codiumai-pr-agent-free bot added enhancement New feature or request bug_fix labels Jan 2, 2024
Copy link

PR Description updated to latest commit (d2d9042)

Copy link

PR Analysis

  • 🎯 Main theme: Enhancing the cleanup of resources
  • 📝 PR summary: This PR introduces several enhancements to the cleanup of resources. It makes the cleanup interval configurable through an environment variable, improves the cleanup handler's logging, updates the metadata loading function to also load labels from the metadata file, and updates the function to delete resources by template hash or wlid to look for the template hash in the labels instead of the annotations. It also reduces the list of workloads to cleanup.
  • 📌 Type of PR: Bug fix
    Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR involves changes in multiple files and the logic of cleanup tasks which requires a good understanding of the codebase.
  • 🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: It would be beneficial to add unit tests for the new functionalities introduced in this PR. This will help ensure the correctness of the code and prevent potential regressions in the future. Also, consider using constants for string literals that are used multiple times in the code, such as "CLEANUP_INTERVAL".

🤖 Code feedback:
relevant filemain.go
suggestion      

Consider handling the error when parsing the cleanup interval from the environment variable. If the parsing fails, it might be due to an invalid format provided by the user. In such a case, it would be helpful to inform the user about the expected format. [important]

relevant lineintervalDuration, err := time.ParseDuration(interval)

relevant filepkg/cleanup/cleanup.go
suggestion      

It's good to see that the logging level for deletions has been changed to debug. However, it might be useful to also log the total number of deletions at the info level after the cleanup task is completed. This will provide a quick overview of the cleanup task's result without flooding the log with details. [medium]

relevant linelogger.L().Debug("deleting", helpers.String("kind", resourceKind), helpers.String("namespace", metadata.Namespace), helpers.String("name", metadata.Name))

relevant filepkg/cleanup/cleanup.go
suggestion      

It seems that the function deleteByTemplateHashOrWlid now looks for the template hash in the labels instead of the annotations. It would be helpful to update the function's comment to reflect this change. [medium]

relevant linewlReplica, wlReplicaFound := metadata.Labels[instanceidhandler.TemplateHashKey] // replica

relevant filepkg/cleanup/discovery.go
suggestion      

The list of workloads to cleanup has been hardcoded in the code. Consider moving this list to a configuration file or environment variables. This will make it easier to update the list without changing the code. [medium]

relevant lineWorkloads = sets.NewSet[string

✨ Usage tips:

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

Copy link

github-actions bot commented Jan 2, 2024

Summary:

  • License scan: failure
  • Credentials scan: success
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

"job",
"lease",
"namespace",
Copy link

Choose a reason for hiding this comment

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

Are you sure about this?

Copy link

github-actions bot commented Jan 2, 2024

Summary:

  • License scan: failure
  • Credentials scan: success
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

Signed-off-by: Matthias Bertschy <[email protected]>
@matthyx matthyx force-pushed the cleanup branch 2 times, most recently from ad5e438 to f5e4e15 Compare January 2, 2024 16:12
Signed-off-by: Matthias Bertschy <[email protected]>
Copy link

github-actions bot commented Jan 2, 2024

Summary:

  • License scan: failure
  • Credentials scan: success
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

@matthyx matthyx added the release label Jan 3, 2024
@matthyx matthyx merged commit 7599c7e into main Jan 3, 2024
6 checks passed
@matthyx matthyx deleted the cleanup branch January 3, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug_fix enhancement New feature or request release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants