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

[Part 1 of #758] Remove pod-culler remnants #890

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

consideRatio
Copy link
Member

As of JupyterHub 0.9, we ended up with some dead code no longer in use.

@consideRatio consideRatio added this to the 0.8 milestone Aug 30, 2018
Copy link
Member Author

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Well sure I made the PR, but it LGTM after self-review a long time since I did it initially.

Note that this is also a bugfix that may not have caused any issue. The bugfix is that we ran two cullers side if max-age was non-zero.

@@ -18,5 +18,3 @@ charts:
valuesPath: singleuser.image
buildArgs:
JUPYTERHUB_VERSION: 0.9.2
pod-culler:
valuesPath: cull.podCuller.image
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer building the image, should be very safe.

{{ include "jupyterhub.matchLabels" $_ | replace ": " "=" | replace "\n" "," | quote }}
{{- end }}


Copy link
Member Author

Choose a reason for hiding this comment

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

This helper was only used by the deleted pod-culler deployment

Copy link
Member Author

Choose a reason for hiding this comment

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

The service running in the hub pod will now take care of the culling, and the hub pod has the credentials required do to so already.

cull_max_age = get_config('cull.max-age')
if cull_max_age and V(jupyterhub.__version__) >= V('0.9'):
if cull_max_age:
Copy link
Member Author

Choose a reason for hiding this comment

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

This will have the same truth table no matter what, if we assume jupyterhub.version to always be >= 0.9, which we do.

name: pod-culler
labels:
{{- include "jupyterhub.labels" . | nindent 4 }}
---
Copy link
Member Author

Choose a reason for hiding this comment

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

This serviceAccount, and associated role bound to it, was only used by the pod-culler deployment.

@@ -1,39 +0,0 @@
{{- if and .Values.cull.enabled .Values.cull.maxAge -}}
Copy link
Member Author

Choose a reason for hiding this comment

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

OH! Note that this culler ran alongside the jupyterhub in-hub-pod service culler since JH 0.9, we had two of them before! I wonder if that caused some issues?

ping @minrk @betatim - in case you have had some unexpected culling behaviors.

podCuller:
image:
name: jupyterhub/k8s-pod-culler
tag: generated-by-chartpress
Copy link
Member Author

Choose a reason for hiding this comment

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

These values were only consumed by the deleted deployment.

@consideRatio consideRatio requested a review from minrk September 4, 2018 13:44
@minrk minrk merged commit 22190bf into jupyterhub:master Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants