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

feat: Add chart #1

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

feat: Add chart #1

wants to merge 17 commits into from

Conversation

Balsir
Copy link
Collaborator

@Balsir Balsir commented Jun 19, 2024

Description

Add keeper chart

Type of change

  • A bug fix (PR prefix fix)
  • A new feature (PR prefix feat)
  • A code change that neither fixes a bug nor adds a feature (PR prefix refactor)
  • Adding missing tests or correcting existing tests (PR prefix test)
  • Changes that do not affect the meaning of the code like white-spaces, formatting, missing semi-colons, etc. (PR prefix style)
  • Changes to our CI configuration files and scripts (PR prefix ci)
  • Documentation only changes (PR prefix docs)

How Has This Been Tested?

@Balsir Balsir requested a review from dojci June 19, 2024 10:42
@Balsir Balsir self-assigned this Jun 19, 2024
@Balsir Balsir requested a review from jaygridley July 2, 2024 09:24
@@ -0,0 +1,24 @@
apiVersion: v2
name: keeper
description: Helm chart for capacity reservation and overprovisioning
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Helm chart for capacity reservation and overprovisioning
description: Helm chart for a node capacity reservation and overprovisioning

@@ -7,9 +7,9 @@ We help companies build, run, deploy and scale software and infrastructure by em
---

## Description
<$chart-name> Helm chart.
keeper Helm chart.
Copy link
Member

Choose a reason for hiding this comment

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

I would like to describe how the "tool" can be used and what issues it solves. Basically, the same description from the main README.md would be ok. Or remove the description.

{{- else }}
priorityClassName: {{ include "keeper.fullname" $ }}-op
{{- end }}
containers:
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add security context for the Pod with "strict" policy.

app.kubernetes.io/name: {{ include "keeper.name" $ }}-rsrv-{{ $k }}
spec:
automountServiceAccountToken: false
containers:
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add security context for the Pod with "strict" policy.

@@ -0,0 +1,11 @@
apiVersion: rbac.authorization.k8s.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

As we are allowing to customize the image, some might need a different RBAC control. I would make this optional, but enabled by default.

# -- Overprovisioning priorityClass name override, will be used instead of priorityClass created in overprovisioning.priorityClass
priorityClassOverride: ""
# -- Map of overprovisioning deployments
map:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
map:
map: {}

# -- Reservation image pull policy
imagePullPolicy: Always
# -- Map of reservation deployments
map:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
map:
map: {}

- -c
args:
- kubectl scale --replicas={{ $v.schedule.up.replicas }} deployment/{{ include "keeper.fullname" $ }}-op-{{ $k }} --namespace {{ $.Release.Namespace }}
resources:
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add security context for the Pod with "strict" policy.

- -c
args:
- kubectl scale --replicas={{ $v.schedule.down.replicas }} deployment/{{ include "keeper.fullname" $ }}-rsrv-{{ $k }} --namespace {{ $.Release.Namespace }}
resources:
Copy link
Member

Choose a reason for hiding this comment

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

I would like to add security context for the Pod with "strict" policy.

# -- Overprovisioning priorityClass priority
value: "-1000000"
# -- Overprovisioning priorityClass name override, will be used instead of priorityClass created in overprovisioning.priorityClass
priorityClassOverride: ""
Copy link
Member

Choose a reason for hiding this comment

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

We could move this under priorityClass and rename to name and use if either for builtin priority class or custom one provided externally.

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.

2 participants