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

Support configuring script pod resource requirements #977

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

APErebus
Copy link
Contributor

@APErebus APErebus commented Jul 9, 2024

Fixes #979

Background

A customer appears to be having resource contention with a large number of script pods. They would like to be able to modify the resource requirements of script pods.

Results

Adds support for a new environment variable OCTOPUS__K8STENTACLE__PODRESOURCEJSON which contains the serialized resource requirements provided by the helm chart in OctopusDeploy/helm-charts#225

We then deserialize this to a V1ResourceRequirements and use it

Shortcut story: [sc-82857]

How to review this PR

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

@APErebus APErebus marked this pull request as ready for review July 9, 2024 23:26
@APErebus APErebus requested a review from a team as a code owner July 9, 2024 23:26
Copy link
Contributor

@kevjt kevjt 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! One minor comment.

Copy link
Contributor

@liam-mackie liam-mackie left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the 🍐 on the review.
For posterity - we talked about validating the resource requests and limits, but there was no easy way to do this in the C# k8s client. In addition, the error returned when this happens is verbose and points to the issue fairly well.

Copy link
Contributor

@liam-mackie liam-mackie left a comment

Choose a reason for hiding this comment

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

Ignore this comment - accidentally approved the same PR instead of the helm-chart PR.

APErebus and others added 2 commits July 10, 2024 11:01
Co-authored-by: Kevin Tchang <151479559+kevjt@users.noreply.github.com>
//if we can't parse the JSON, fall back to the defaults below and warn the user
log.WarnFormat(e, message);
//write a verbose message to the script log.
tentacleScriptLog.Verbose(message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor addition, I now write a verbose message back to the script log

@APErebus APErebus merged commit 4a46ece into main Jul 10, 2024
53 checks passed
@APErebus APErebus deleted the ap/script-pod-resources branch July 10, 2024 02:21
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.

CVE-2024-38095: .NET Information Disclosure Vulnerability
3 participants