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

Feature/fix deprecated apis 1.16 #110

Merged
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@

# Deis Controller

[![Build Status](https://travis-ci.org/teamhephy/controller.svg?branch=master)](https://travis-ci.org/teamhephy/controller)
[![codecov.io](https://codecov.io/github/deis/controller/coverage.svg?branch=master)](https://codecov.io/github/deis/controller?branch=master)
[![Docker Repository on Quay](https://quay.io/repository/deisci/controller/status "Docker Repository on Quay")](https://quay.io/repository/deisci/controller)

Expand Down
7 changes: 3 additions & 4 deletions charts/controller/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
name: controller
home: https://github.com/deisthree/controller
home: https://github.com/teamhephy/controller
version: <Will be populated by the ci before publishing the chart>
description: Deis Workflow Controller (API).
description: Hephy Workflow Controller (API).
maintainers:
- name: Deis Team
email: engineering@deis.com
- email: team@teamhephy.com
6 changes: 6 additions & 0 deletions charts/controller/templates/controller-deployment.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
{{- if .Capabilities.APIVersions.Has "apps/v1" }}
apiVersion: apps/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be something like this to keep backwars compatibility with older clusters:

Suggested change
apiVersion: apps/v1
{{- if .Capabilities.APIVersions.Has "apps/v1" }}
apiVersion: apps/v1
{{- else }}
apiVersion: extensions/v1beta1
{{- 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.

Going to try the above and see if it works for us. 👍 Sometime before we have had problems using . Capabilities.APIVersions.Has.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs mention that checking for resources (apps/v1/Deployment) requires at least Helm v2.15, but just using apps/v1 (without a resource) should be fine, if that's specific enough.

Copy link
Member

Choose a reason for hiding this comment

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

You can perhaps just check for "v1" – I think you might not find specific API groups, but you can count on support for apps/v1 if APIVersions has "v1"

There was an issue I found on the Prometheus helm chart repo that covered this, I will look to see what they settled on to solve the backwards compatibility question, and if it differs

Copy link
Member Author

Choose a reason for hiding this comment

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

Following this suggestion in 5930851 . 👍 I found this similar example in a Datadog agent chart that looks very similar to exactly what we are doing here:

https://github.com/helm/charts/blob/2215d0f752370ae75fa54a9f62b057ed14f8e744/stable/datadog/templates/agent-clusterchecks-deployment.yaml#L2-L9

I think they are adding the last else in the end to make sure it tries to get deployed using the default apiVersion as apps/v1 even if the deployer agent (Helm or whatever...) does not have permission to view .Capabilities.APIVersions?

{{- else if .Capabilities.APIVersions.Has "extensions/v1beta1" }}
apiVersion: extensions/v1beta1
{{- else }}
apiVersion: apps/v1
{{- end }}
kind: Deployment
metadata:
name: deis-controller
Expand Down
8 changes: 6 additions & 2 deletions rootfs/scheduler/mock.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ def delete_pods(pods, current, desired):


def create_pods(url, labels, base, new_pods):
# Start by fetching available pods in the Namespace that fit the profile
# Start by fetching available pods in the Namespace that fit the procfile
# and prune down if needed, Otherwise go into the addition logic here
for _ in range(new_pods):
data = base.copy()
Expand Down Expand Up @@ -391,6 +391,8 @@ def upsert_pods(controller, url):
# turn RC / RS (which a Deployment creates) url into pods one
url = url.replace(cache_key(controller['metadata']['name']), '')
if '_replicasets_' in url:
# Try replacing both just in case one or the other exists (api backwards compatibility)
url = url.replace('_replicasets_', '_pods').replace('apis_apps_v1', 'api_v1') # noqa
Cryptophobia marked this conversation as resolved.
Show resolved Hide resolved
url = url.replace('_replicasets_', '_pods').replace('apis_extensions_v1beta1', 'api_v1') # noqa
else:
url = url.replace('_replicationcontrollers_', '_pods')
Expand Down Expand Up @@ -512,7 +514,9 @@ def manage_replicasets(deployment, url):

def update_deployment_status(namespaced_url, url, deployment, rs):
# Fill out deployment.status for success as pods transition to running state
pod_url = namespaced_url.replace('_replicasets', '_pods').replace('apis_extensions_v1beta1', 'api_v1') # noqa
pod_url = namespaced_url.replace('_replicasets', '_pods').replace('apis_apps_v1', 'api_v1') # noqa
# Try replacing both just in case one or the other exists (api backwards compatibility)
pod_url = pod_url.replace('_replicasets', '_pods').replace('apis_extensions_v1beta1', 'api_v1') # noqa
while True:
# The below needs to be done to emulate Deployment handling things
# always cleanup pods
Expand Down
4 changes: 3 additions & 1 deletion rootfs/scheduler/resources/deployment.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ class Deployment(Resource):

@property
def api_version(self):
# API locations have changed since 1.9 and deprecated in 1.16
# https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/
if self.version() >= parse("1.9.0"):
return 'extensions/v1beta1'
return 'apps/v1'

return 'extensions/v1beta1'

Expand Down
2 changes: 2 additions & 0 deletions rootfs/scheduler/resources/ingress.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ def get(self, name=None, **kwargs):
return response

def create(self, ingress, namespace, hostname):
# Ingress API will be deprecated in 1.20 to use networking.k8s.io/v1beta1
# https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/
url = "/apis/extensions/v1beta1/namespaces/%s/ingresses" % namespace
Cryptophobia marked this conversation as resolved.
Show resolved Hide resolved

data = {
Expand Down
12 changes: 11 additions & 1 deletion rootfs/scheduler/resources/replicaset.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
from scheduler.exceptions import KubeHTTPException
from scheduler.resources import Resource

from packaging.version import parse


class ReplicaSet(Resource):
api_prefix = 'apis'
api_version = 'extensions/v1beta1'
short_name = 'rs'

@property
def api_version(self):
# API locations have changed since 1.9 and deprecated in 1.16
# https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/
if self.version() >= parse("1.9.0"):
return 'apps/v1'

return 'extensions/v1beta1'

def get(self, namespace, name=None, **kwargs):
"""
Fetch a single ReplicaSet or a list
Expand Down
2 changes: 1 addition & 1 deletion rootfs/scheduler/tests/test_deployments.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def test_deployment_api_version_1_9_and_up(self):

deployment = copy.copy(self.scheduler.deployment)

expected = 'extensions/v1beta1'
expected = 'apps/v1'

for canonical in cases:
deployment.version = mock.MagicMock(return_value=parse(canonical))
Expand Down