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

Switch namespaced deploy to pruning blacklist #616

Merged
merged 3 commits into from
Nov 15, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Nov 7, 2019

What are you trying to accomplish with this PR?
Prune all namespaced kinds by default instead of maintaining a whitelist.

How is this accomplished?
This mirrors #612. However, the latest group/versions doesn't always include all kinds, so I've added an override for the 5 kinds that need it. As far as I can tell there is no kubectl call to get all resources supported version. There is a call to get a resource's supported version making a call per resource seems extreme.

What could go wrong?
This will prune things we didn't before, full list is below..
We may have to keep the override list up-to-date. That being said the list currently contains five items, which is much shorter than the white list it's replacing.

@dturn
Copy link
Contributor Author

dturn commented Nov 7, 2019

Namespaced pruneable resources in k8s v1.16.1

"/v1/ConfigMap",
"/v1/Endpoints",
"/v1/Event",
"/v1/LimitRange",
"/v1/PersistentVolumeClaim",
"/v1/Pod",
"/v1/PodTemplate",
"/v1/ReplicationController",
"/v1/ResourceQuota",
"/v1/Secret",
"/v1/ServiceAccount",
"/v1/Service",
"apps/v1/ControllerRevision",
"apps/v1/DaemonSet",
"apps/v1/Deployment",
"apps/v1/ReplicaSet",
"apps/v1/StatefulSet",
"autoscaling/v2beta2/HorizontalPodAutoscaler",
"batch/v1beta1/CronJob",
"batch/v1/Job",
"coordination.k8s.io/v1/Lease",
"events.k8s.io/v1beta1/Event",
"extensions/v1beta1/Ingress",
"networking.k8s.io/v1beta1/Ingress",
"networking.k8s.io/v1/NetworkPolicy",
"policy/v1beta1/PodDisruptionBudget",
"rbac.authorization.k8s.io/v1/RoleBinding",
"rbac.authorization.k8s.io/v1/Role"

Newly prunable resources

"Endpoints", "Event", "LimitRange", "ReplicationController", "ControllerRevision", "Lease", "Event"

@dturn dturn force-pushed the namespaced-prune-everything branch 2 times, most recently from 86a79f2 to 8bf43b6 Compare November 7, 2019 23:36
@dturn dturn marked this pull request as ready for review November 8, 2019 01:07
@dturn dturn changed the base branch from global-prune-2 to master November 12, 2019 16:40
@dturn dturn force-pushed the namespaced-prune-everything branch 7 times, most recently from 8aeb382 to d91d9ef Compare November 12, 2019 22:07
@dturn dturn requested review from timothysmith0609 and a team November 12, 2019 22:23
@dturn dturn self-assigned this Nov 12, 2019
@@ -8,5 +8,9 @@ def initialize(**args)
raise "Use Krane::DeployGlobalTask to deploy global resources" if args[:allow_globals]
super(args.merge(allow_globals: false))
end

def prune_whitelist
Copy link
Contributor

Choose a reason for hiding this comment

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

are we adding this here (instead of updating Krane::DeprecatedDeployTask) to avoid changing KubernetesDeploy::DeployTask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@dturn
Copy link
Contributor Author

dturn commented Nov 14, 2019

This PR and the global-deploy pruning PR no longer respect the krane.shopify.io/prunable annotation. I think that's the right behavior, but we'll need to clean up the readme & migration guide, in a subsequent PR depending if this merges before 1.0.0-pre.1

@dturn dturn requested a review from dirceu November 14, 2019 16:35
@dturn dturn requested a review from a team November 14, 2019 17:37
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

A few small questions. In general I'm finding this pretty hard to read and understand, though :(

Comment on lines 88 to 97
pattern = /v(?<major>\d+)(beta)?(?<minor>\d+)?/
latest = versions.sort_by do |version|
match = version.match(pattern)
[match[:major].to_i, (match[:minor] || 999).to_i]
end.last
version_override.fetch(kind, latest)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain a bit further on exactly what's happening here? Do we only need to worry about (beta), what about alpha groups, or are they not relevant (e.g. we only care about the override group)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've also tried to fix up names and added a comment to make things clearer.

Comment on lines +36 to +37
%w(PriorityClass StorageClass).each do |expected_kind|
assert kinds.one? { |k| k.include?(expected_kind) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a sanity check or is there a reason for these 2 in particular?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a sanity check, the assert_equal(kinds.length, 13) is the check to ensure list has the right number of items.

@@ -185,8 +185,7 @@ def namespace_globals(fixtures, selector)
fixtures.each do |_, kinds_map|
kinds_map.each do |_, resources|
resources.each do |resource|
resource["metadata"]["name"] = (resource["metadata"]["name"] + @namespace)[0..63]
resource["metadata"]["name"] += "0" if resource["metadata"]["name"].end_with?("-")
resource["metadata"]["name"] = (resource["metadata"]["name"] + Digest::MD5.hexdigest(@namespace))[0..63]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to fix a bug that was causing resources to have the same name across different tests. Its triggered when the real name was long as the start of two tests were the same. We need the value appended to the name to be stable across tests hence the hashing of @namespace.

@dturn dturn force-pushed the namespaced-prune-everything branch from d91d9ef to 1460d6a Compare November 15, 2019 16:27
@dturn dturn merged commit a6b9212 into master Nov 15, 2019
@dturn dturn deleted the namespaced-prune-everything branch November 15, 2019 17:41
@dturn dturn mentioned this pull request Nov 15, 2019
@ghost ghost added the krane [ProdX-GSD] label Nov 22, 2019
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.

None yet

4 participants