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.

Sign up for GitHub

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

Jump to bottom

Feature/fix deprecated apis 1.16 #110

Merged
merged 9 commits into from
Oct 1, 2020
Merged

Feature/fix deprecated apis 1.16 #110

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
93682c6
ref(apis): deprecated apis in k8s 1.16
Cryptophobia Sep 20, 2019
b4928ed
ref(apis): trying to fix mock tests now
Cryptophobia Sep 20, 2019
27b8f8a
ref(charts): move from extensions/v1beta1 to apps/v1
Cryptophobia Sep 20, 2019
a6fa780
ref(mock.py): sanitize pod urls to api_v1 in the mocker
Cryptophobia Sep 21, 2019
d8e65a9
ref(apis): parse version out for replicasets
Cryptophobia Oct 21, 2019
25a7f41
ref(mock.py): reduce line length for style check
Cryptophobia Oct 21, 2019
fd3feba
docs(README): add travis build status badge
Cryptophobia Oct 21, 2019
5930851
ref(charts): add apiversion checks to helm charts
Cryptophobia Oct 21, 2019
5613bc8
fix(README): resolving a merge conflict
Cryptophobia Oct 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/controller/templates/controller-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
apiVersion: extensions/v1beta1
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?

kind: Deployment
metadata:
name: deis-controller
Expand Down
7 changes: 4 additions & 3 deletions rootfs/scheduler/mock.py
View file
Open in desktop
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,7 +391,7 @@ 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:
url = url.replace('_replicasets_', '_pods').replace('apis_extensions_v1beta1', 'api_v1') # noqa
url = url.replace('_replicasets_', '_pods').replace('apis_apps_v1', 'api_v1') # noqa
Cryptophobia marked this conversation as resolved.
Show resolved Hide resolved
else:
url = url.replace('_replicationcontrollers_', '_pods')
# make sure url only has up to "_pods"
Expand Down Expand Up @@ -450,6 +450,7 @@ def manage_replicasets(deployment, url):

# get RS url
rs_url = url.replace('_deployments_', '_replicasets_')
rs_url = rs_url.replace('apis_extensions_v1beta1_', 'apis_apps_v1_')
Cryptophobia marked this conversation as resolved.
Show resolved Hide resolved
rs_url += '_' + pod_hash
namespaced_url = rs_url[0:(rs_url.find("_replicasets") + 12)]

Expand Down Expand Up @@ -512,7 +513,7 @@ 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
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
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion rootfs/scheduler/resources/replicaset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

class ReplicaSet(Resource):
api_prefix = 'apis'
api_version = 'extensions/v1beta1'
api_version = 'apps/v1'
Cryptophobia marked this conversation as resolved.
Show resolved Hide resolved
short_name = 'rs'

def get(self, namespace, name=None, **kwargs):
Expand Down
2 changes: 1 addition & 1 deletion rootfs/scheduler/tests/test_deployments.py
View file
Open in desktop
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