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

Global prune 2 #612

Merged
merged 4 commits into from
Nov 12, 2019
Merged

Global prune 2 #612

merged 4 commits into from
Nov 12, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Nov 4, 2019

What are you trying to accomplish with this PR?
Add pruning by default to global deploy

How is this accomplished?
Fetch a list of global resources that are pruneable (everything that supports delete) exclude node and namespace since I can't image we'd actually want to ever prune these and then pass them as the list of resources to prune.

What could go wrong?
Worst case is that we are including something in the prune list we shouldn't

@KnVerey
Copy link
Contributor

KnVerey commented Nov 6, 2019

Worst case is that we are including something in the prune list we shouldn't

For sanity checking purposes, can you please post what this list actually is in the oldest and newest k8s versions we currently support?

@dturn
Copy link
Contributor Author

dturn commented Nov 7, 2019

We intentionally black list Node and Namespace. I can't really see a reason where pruning a Node is a good idea. Pruning a Namespace is more reasonable but really dangerous.

1.11.10

"/v1/PersistentVolume",
"admissionregistration.k8s.io/v1beta1/MutatingWebhookConfiguration",
"admissionregistration.k8s.io/v1beta1/ValidatingWebhookConfiguration",
"apiextensions.k8s.io/v1beta1/CustomResourceDefinition",
"apiregistration.k8s.io/v1/APIService",
"certificates.k8s.io/v1beta1/CertificateSigningRequest",
"extensions/v1beta1/PodSecurityPolicy",
"policy/v1beta1/PodSecurityPolicy",
"rbac.authorization.k8s.io/v1/ClusterRoleBinding",
"rbac.authorization.k8s.io/v1/ClusterRole",
"scheduling.k8s.io/v1beta1/PriorityClass",
"storage.k8s.io/v1/StorageClass",
"storage.k8s.io/v1beta1/VolumeAttachment"

1.16.1

"/v1/PersistentVolume",
"admissionregistration.k8s.io/v1/MutatingWebhookConfiguration",
"admissionregistration.k8s.io/v1/ValidatingWebhookConfiguration",
"apiextensions.k8s.io/v1/CustomResourceDefinition",
"apiregistration.k8s.io/v1/APIService",
"certificates.k8s.io/v1beta1/CertificateSigningRequest",
"node.k8s.io/v1beta1/RuntimeClass", # not in 1.11
"policy/v1beta1/PodSecurityPolicy",
"rbac.authorization.k8s.io/v1/ClusterRoleBinding",
"rbac.authorization.k8s.io/v1/ClusterRole",
"scheduling.k8s.io/v1/PriorityClass",
"storage.k8s.io/v1/CSIDriver", # not in 1.11
"storage.k8s.io/v1/StorageClass",
"storage.k8s.io/v1beta1/VolumeAttachment"

@dturn dturn force-pushed the global-deploy-2 branch 2 times, most recently from 7b616f1 to fa17169 Compare November 7, 2019 00:57
@dturn dturn force-pushed the global-prune-2 branch 2 times, most recently from d4244b8 to 7c85a9f Compare November 7, 2019 17:51
@dturn dturn changed the base branch from global-deploy-2 to master November 7, 2019 18:16
@dturn dturn marked this pull request as ready for review November 7, 2019 18:21
@dturn dturn requested review from timothysmith0609 and a team November 7, 2019 18:21
Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

One question but I think this looks pretty good to me.

test/unit/cluster_resource_discovery_test.rb Show resolved Hide resolved
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

I'd also like to see a test that shows pruning behaves correctly with custom resources, when both a namespaced and a global CR exist.

lib/krane/cli/global_deploy_command.rb Outdated Show resolved Hide resolved
lib/krane/cluster_resource_discovery.rb Show resolved Hide resolved
lib/krane/global_deploy_task.rb Outdated Show resolved Hide resolved
test/unit/cluster_resource_discovery_test.rb Show resolved Hide resolved
lib/krane/cluster_resource_discovery.rb Outdated Show resolved Hide resolved
@@ -116,4 +126,54 @@ def test_global_deploy_task_failure
"Template validation failed",
])
end

def test_global_deploy_prune_black_box_success
assert_deploy_success(deploy_global_fixtures('globals', clean_up: false, selector: 'test=prune1'))
Copy link
Contributor

Choose a reason for hiding this comment

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

same question, should we be adding this selector to the generated one so we don't have to hope future contributors won't reuse this manual selector?

test/integration/global_deploy_test.rb Outdated Show resolved Hide resolved
test/integration/global_deploy_test.rb Show resolved Hide resolved
test/integration/global_deploy_test.rb Outdated Show resolved Hide resolved
"Successful resources",
"StorageClass/testing-storage-class",
])
assert_deploy_success(deploy_global_fixtures('globals', selector: 'test=prune2'))
Copy link
Contributor

Choose a reason for hiding this comment

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

we should refute that the logs include "[pP]rune[d]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the test name to modify the fixture name, so if we refute 'prune' we can't use 'prune' in the test name. Instead I'm refuting 'pruned'.

@dturn dturn requested a review from dirceu November 11, 2019 23:30
Copy link
Contributor

@dirceu dirceu left a comment

Choose a reason for hiding this comment

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

I added a small question, but this makes sense to me. 👍

@@ -103,6 +103,14 @@ def build_storage_v1_kubeclient(context)
)
end

def build_scheduling_v1beta1_kubeclient(context)
Copy link
Contributor

@dirceu dirceu Nov 12, 2019

Choose a reason for hiding this comment

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

Is this actually necessary? I tried removing this and running tests but they keep passing; I tested with the tests modified/added in this PR:

$ alias rt="bundle exec ruby -I test"
$ rt test/integration-serial/serial_deploy_test.rb --name test_global_deploy_prune_black_box_success
$ rt test/integration/global_deploy_test.rb
$ rt test/unit/cluster_resource_discovery_test.rb

Am I missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build_scheduling_v1beta1_kubeclient are mostly used in the tests to allow for manual interaction with resources, usually to delete them in an ensure block. They aren't being used but seemed less than ideal to add a new kind to the fixtures but not add its helper methods.

@dturn dturn merged commit 15c4723 into master Nov 12, 2019
@dturn dturn deleted the global-prune-2 branch November 12, 2019 16:41
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants