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

Require k8s 1.17+ to reduce complexity #2005

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jan 21, 2021

By relying on k8s 1.17+, we can cut away a hard to maintain
user-scheduler implementation for k8s 1.16 and earlier that is different
from the implementation we use for k8s 1.17+. See #1995.

Action points

By relying on k8s 1.17+, we can cut away a hard to maintain
user-scheduler implementation for k8s 1.16 and earlier that is different
from the implementation we use for k8s 1.17+.
@manics
Copy link
Member

manics commented Jan 23, 2021

The failing test error

Run . ./ci/common
  . ./ci/common
  UPGRADE_FROM_VERSION=$(curl -sS https://jupyterhub.github.io/helm-chart/info.json | jq -er '.jupyterhub.dev')
  echo "UPGRADE_FROM_VERSION=$UPGRADE_FROM_VERSION" >> $GITHUB_ENV
  
  echo ""
  echo "Installing already released jupyterhub version $UPGRADE_FROM_VERSION"
  
  # FIXME: We change the directory so jupyterhub the chart name won't be
  #        misunderstood as the local folder name.
  #
  #        https://github.com/helm/helm/issues/9244
  cd ci
  helm install jupyterhub --repo https://jupyterhub.github.io/helm-chart/ jupyterhub --values ../dev-config.yaml --version=$UPGRADE_FROM_VERSION
  
  echo ""
  echo "Installing Helm diff plugin while k8s resources are initializing"
  helm plugin install https://github.com/databus23/helm-diff
  shell: /usr/bin/bash -e {0}
  env:
    KUBECONFIG: /home/runner/.kube/config
    pythonLocation: /opt/hostedtoolcache/Python/3.8.6/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.8.6/x64/lib

Error: chart "jupyterhub" version "0.11.1-n058.h9dcf8f80" not found in https://jupyterhub.github.io/helm-chart/ repository
Installing already released jupyterhub version 0.11.1-n058.h9dcf8f80
Error: Process completed with exit code 1.

sounds suspiciously similar to jupyterhub/mybinder.org-deploy#1706

@consideRatio
Copy link
Member Author

consideRatio commented Jan 23, 2021

I think of two reasons for this happening:

  1. We run helm repo update at one point in time, and then later we check the available versions which may now contain something newer. In this repo, we don't use helm repo add + helm repo update, but use the --repo flag during upgrades.
  2. We run with --repo two times nad helm is using a cache in the second lookup. I doubt this though.
  3. GitHub pages serving our Helm chart repo is using a cache of some kind. In z2jh we first check the version against a template generated .json file and then later use helm upgrade --repo ... against the index.yaml file. In case the index.yaml file is cached, we can get this issue.

I suspect this is caused by step 3. Googling my way onwards I conclude that they have a 10 minute cache control. See: https://webapps.stackexchange.com/a/119294

@yuvipanda
Copy link
Collaborator

Thanks for keeping this separate, @consideRatio! I'd like us to consider also making a point release before we bump to k8s 1.17. That would make life easier for folks, since they wouldn't have to move to helm 3 and a k8s version bump at the same time.

@consideRatio
Copy link
Member Author

@yuvipanda 1.17 is now the default in GKE and users of 1.16 are now being automatically upgraded to 1.17.

After considering this for a while I'd like to just get rid of old logic and reduce complexity sooner rather than later.

The key point is that it would give me peace of mind when users report issues with the user-scheduler to know that if they use a specific version a specific user-scheduler implementation rather than one depending on the k8s version. There have been some issues with it lately as reported in #2025 for example and some before it also.

Okay to merge this?

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

$ git grep 1\\.16

CHANGELOG.md:than 1.16.
CHANGELOG.md:- Fixed a compatibility issue with Kubernetes 1.16+
CHANGELOG.md:- Kubernetes 1.16 compatibility achieved
jupyterhub/templates/scheduling/user-scheduler/deployment.yaml:          image: {{ .Values.scheduling.userScheduler.image.name }}:v1.16.15
jupyterhub/templates/scheduling/user-scheduler/rbac.yaml:  #       1.16 and in 1.17, but unchanged in 1.18 and 1.19.

image: {{ .Values.scheduling.userScheduler.image.name }}:v1.16.15

is removed in #1995

Does the comment in

# NOTE: These rules have been unchanged between 1.12 and 1.15, then changed in
# 1.16 and in 1.17, but unchanged in 1.18 and 1.19.
#
# ref: https://github.com/kubernetes/kubernetes/blob/v1.19.0/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml#L696-L829

need to be updated or removed? As it is I don't understand the it's significance.

Regardless, 👍 for dropping 1.16

@yuvipanda
Copy link
Collaborator

Thanks for patiently waiting, @consideRatio! This is good to go now!

@yuvipanda yuvipanda merged commit 07cd588 into jupyterhub:master Feb 11, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Feb 11, 2021
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.

3 participants