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

Global prune 2 #612

Merged
merged 4 commits into from
Nov 12, 2019
Merged

Global prune 2 #612

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb4e754
Prune global resources
dturn Oct 24, 2019
55171af
udpate tests
dturn Nov 4, 2019
afcff78
PR feedback
dturn Nov 11, 2019
5fb93cb
More code comments
dturn Nov 11, 2019
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
4 changes: 3 additions & 1 deletion lib/krane/cli/global_deploy_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class GlobalDeployCommand
desc: "Verify workloads correctly deployed" },
"selector" => { type: :string, banner: "'label=value'", required: true,
desc: "Select workloads owned by selector(s)" },
"prune" => { type: :boolean, desc: "Enable deletion of resources that do not appear in the template dir",
dturn marked this conversation as resolved.
Show resolved Hide resolved
default: true },
}

def self.from_options(context, options)
Expand All @@ -35,7 +37,7 @@ def self.from_options(context, options)

deploy.run!(
verify_result: options["verify-result"],
prune: false,
prune: options[:prune],
)
end
end
Expand Down
32 changes: 29 additions & 3 deletions lib/krane/cluster_resource_discovery.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,38 @@ def crds
end

def global_resource_kinds
@globals ||= fetch_globals.map { |g| g["kind"] }
@globals ||= fetch_resources(only_globals: true).map { |g| g["kind"] }
end

def prunable_resources
api_versions = fetch_api_versions
fetch_resources.map do |resource|
next unless resource['verbs'].include?("delete")
dturn marked this conversation as resolved.
Show resolved Hide resolved
version = api_versions.fetch(resource['apigroup'], ['v1']).last
dturn marked this conversation as resolved.
Show resolved Hide resolved
[resource['apigroup'], version, resource['kind']].compact.join("/")
end.compact
end

private

def fetch_globals
raw, _, st = kubectl.run("api-resources", "--namespaced=false", output: "wide", attempts: 5,
def fetch_api_versions
dturn marked this conversation as resolved.
Show resolved Hide resolved
raw, _, st = kubectl.run("api-versions", attempts: 5, use_namespace: false)
if st.success?
rows = raw.split("\n")
rows.each_with_object({}) do |group_version, hash|
group, version = group_version.split("/")
dturn marked this conversation as resolved.
Show resolved Hide resolved
hash[group] ||= []
hash[group] << version
end
else
{}
end
end

def fetch_resources(only_globals: false)
dturn marked this conversation as resolved.
Show resolved Hide resolved
command = %w(api-resources)
command << "--namespaced=false" if only_globals
raw, _, st = kubectl.run(*command, output: "wide", attempts: 5,
use_namespace: false)
if st.success?
rows = raw.split("\n")
Expand All @@ -34,6 +59,7 @@ def fetch_globals
fields = full_width_field_names.each_with_object({}) do |name, hash|
start = cursor
cursor = start + name.length
cursor = 0 if full_width_field_names.last == name.strip
dturn marked this conversation as resolved.
Show resolved Hide resolved
hash[name.strip] = [start, cursor - 1]
end
resources.map { |r| fields.map { |k, (s, e)| [k.strip, r[s..e].strip] }.to_h }
Expand Down
8 changes: 7 additions & 1 deletion lib/krane/global_deploy_task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ def run!(verify_result: true, prune: true)
private

def deploy!(resources, verify_result, prune)
prune_whitelist = []
resource_deployer = ResourceDeployer.new(task_config: @task_config,
prune_whitelist: prune_whitelist, max_watch_seconds: @global_timeout,
selector: @selector, statsd_tags: statsd_tags)
Expand Down Expand Up @@ -197,6 +196,13 @@ def kubeclient_builder
@kubeclient_builder ||= KubeclientBuilder.new
end

def prune_whitelist
black_list = %w(Namespace Node)
cluster_resource_discoverer.prunable_resources.select do |gvk|
dturn marked this conversation as resolved.
Show resolved Hide resolved
global_kinds.any? { |g| gvk.include?(g) } && black_list.none? { |b| gvk.include?(b) }
end
end

def check_initial_status(resources)
cache = ResourceCache.new(@task_config)
Concurrency.split_across_threads(resources) { |r| r.sync(cache) }
Expand Down
8 changes: 8 additions & 0 deletions lib/krane/kubeclient_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

build_kubeclient(
api_version: "v1beta1",
context: context,
endpoint_path: "/apis/scheduling.k8s.io"
)
end

def validate_config_files
errors = []
if @kubeconfig_files.empty?
Expand Down
7 changes: 6 additions & 1 deletion test/exe/global_deploy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ def test_deploy_parses_selector
krane_global_deploy!(flags: "--selector #{selector}")
end

def test_deploy_parses_prune
set_krane_global_deploy_expectations!(run_args: { prune: false })
krane_global_deploy!(flags: '--prune false')
end

private

def set_krane_global_deploy_expectations!(new_args: {}, run_args: {})
Expand Down Expand Up @@ -68,7 +73,7 @@ def default_options(new_args = {}, run_args = {})
}.merge(new_args),
run_args: {
verify_result: true,
prune: false,
prune: true,
}.merge(run_args),
}
end
Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/globals/priority_class.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: scheduling.k8s.io/v1beta1
description: Used for testing global deploys and pruning.
kind: PriorityClass
metadata:
name: testing-priority-class
labels:
app: krane
value: 20
12 changes: 7 additions & 5 deletions test/helpers/fixture_deploy_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def deploy_global_fixtures(set, subset: nil, **args)
fixtures = load_fixtures(set, subset)
raise "Cannot deploy empty template set" if fixtures.empty?
args[:selector] ||= "test=#{@namespace}"
namespace_globals(fixtures)
namespace_globals(fixtures, args[:selector])

yield fixtures if block_given?

Expand Down Expand Up @@ -104,7 +104,7 @@ def deploy_dirs_without_profiling(dirs, wait: true, allow_protected_ns: false, p
)
end

def global_deploy_dirs_without_profiling(dirs, verify_result: true, prune: false,
def global_deploy_dirs_without_profiling(dirs, clean_up: true, verify_result: true, prune: true,
global_timeout: 300, selector:)
deploy = Krane::GlobalDeployTask.new(
context: KubeclientHelper::TEST_CONTEXT,
Expand All @@ -118,7 +118,7 @@ def global_deploy_dirs_without_profiling(dirs, verify_result: true, prune: false
prune: prune
)
ensure
delete_globals(Array(dirs))
delete_globals(Array(dirs)) if clean_up
end

# Deploys all fixtures in the given directories via KubernetesDeploy::DeployTask
Expand Down Expand Up @@ -180,13 +180,15 @@ def build_kubectl(log_failure_by_default: true, timeout: '5s')
log_failure_by_default: log_failure_by_default, default_timeout: timeout)
end

def namespace_globals(fixtures)
def namespace_globals(fixtures, selector)
selector_key, selector_value = selector.split("=")
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"]["labels"] ||= {}
resource["metadata"]["labels"]["test"] = @namespace
resource["metadata"]["labels"][selector_key] = selector_value
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions test/helpers/kubeclient_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ def storage_v1_kubeclient
@storage_v1_kubeclient ||= kubeclient_builder.build_storage_v1_kubeclient(TEST_CONTEXT)
end

def scheduling_v1beta1_kubeclient
@scheduling_v1beta1_kubeclient ||= kubeclient_builder.build_scheduling_v1beta1_kubeclient(TEST_CONTEXT)
end

def kubeclient_builder
@kubeclient_builder ||= Krane::KubeclientBuilder.new
end
Expand Down
18 changes: 18 additions & 0 deletions test/integration-serial/serial_deploy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,24 @@ def test_global_deploy_validation_catches_namespaced_cr
wait_for_all_crd_deletion
end

def test_global_deploy_prune_black_box_success
setup_template_dir("globals") do |target_dir|
dturn marked this conversation as resolved.
Show resolved Hide resolved
flags = "-f #{target_dir} --selector app=krane"
out, err, status = krane_black_box("global-deploy", "#{KubeclientHelper::TEST_CONTEXT} #{flags}")
assert_empty(out)
assert_match("Success", err)
assert_predicate(status, :success?)

Copy link
Contributor

Choose a reason for hiding this comment

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

flush the logger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

krane_black_box uses Open3.capture3 so each call get's a unique stream

flags = "-f #{target_dir}/storage_classes.yml --selector app=krane"
out, err, status = krane_black_box("global-deploy", "#{KubeclientHelper::TEST_CONTEXT} #{flags}")
assert_empty(out)
assert_match("Pruned 1 resource and successfully deployed 1 resource", err)
assert_predicate(status, :success?)
end
ensure
build_kubectl.run("delete", "-f", fixture_path("globals"), use_namespace: false, log_failure: false)
end

private

def wait_for_all_crd_deletion
Expand Down
82 changes: 71 additions & 11 deletions test/integration/global_deploy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@ def test_global_deploy_task_success
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Not Found},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Deploying resources:",
%r{StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
%r{PriorityClass/testing-priority-class[\w-]+ \(timeout: 300s\)},
"Don't know how to monitor resources of type StorageClass.",
%r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
%r{Successfully deployed in [\d.]+s: StorageClass\/testing-storage-class},
%r{Successfully deployed in [\d.]+s: PriorityClass/testing-priority-class[\w-]+, StorageClass\/testing-storage-},
"Result: SUCCESS",
"Successfully deployed 1 resource",
"Successfully deployed 2 resource",
dturn marked this conversation as resolved.
Show resolved Hide resolved
"Successful resources",
"StorageClass/testing-storage-class",
"PriorityClass/testing-priority-class",
])
end

Expand All @@ -37,9 +40,9 @@ def test_global_deploy_task_success_timeout
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Not Found},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Deploying resources:",
"Result: TIMED OUT",
"Timed out waiting for 1 resource to deploy",
"Timed out waiting for 2 resources to deploy",
%r{StorageClass\/testing-storage-class[\w-]+: GLOBAL WATCH TIMEOUT \(0 seconds\)},
"If you expected it to take longer than 0 seconds for your deploy to roll out, increase --max-watch-seconds.",
])
Expand All @@ -54,12 +57,16 @@ def test_global_deploy_task_success_verify_false
"All required parameters and files are present",
"Discovering resources:",
" - StorageClass/testing-storage-class",
" - PriorityClass/testing-priority-class",
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Not Found},
%r{PriorityClass/testing-priority-class[\w-]+\s+Not Found},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Deploying resources:",
%r{StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
%r{PriorityClass/testing-priority-class[\w-]+ \(timeout: 300s\)},
"Result: SUCCESS",
"Deployed 1 resource",
"Deployed 2 resource",
dturn marked this conversation as resolved.
Show resolved Hide resolved
"Deploy result verification is disabled for this deploy.",
"This means the desired changes were communicated to Kubernetes, but the"\
" deploy did not make sure they actually succeeded.",
Expand All @@ -77,7 +84,7 @@ def test_global_deploy_task_empty_selector_validation_failure
end

def test_global_deploy_task_success_selector
selector = "app=krane"
selector = "app=krane2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? Should we be using both this and a generated selector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we always generated a selector, this PR allows us to pass the selector (needed for pruning reasons). But if the selector matches a different test's selector the pruning process conflicts.

assert_deploy_success(deploy_global_fixtures('globals', selector: selector))

assert_logs_match_all([
Expand All @@ -89,14 +96,17 @@ def test_global_deploy_task_success_selector
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Not Found},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Deploying resources:",
%r{PriorityClass/testing-priority-class[\w-]+ \(timeout: 300s\)},
%r{StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Don't know how to monitor resources of type StorageClass.",
%r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
%r{Successfully deployed in [\d.]+s: StorageClass\/testing-storage-class},
/Successfully deployed in [\d.]+s/,
"Result: SUCCESS",
"Successfully deployed 1 resource",
"Successfully deployed 2 resource",
dturn marked this conversation as resolved.
Show resolved Hide resolved
"Successful resources",
"StorageClass/testing-storage-class",
"PriorityClass/testing-priority-class",
])
end

Expand All @@ -116,4 +126,54 @@ def test_global_deploy_task_failure
"Template validation failed",
])
end

def test_global_deploy_prune_black_box_success
dturn marked this conversation as resolved.
Show resolved Hide resolved
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?

reset_logger
assert_deploy_success(deploy_global_fixtures('globals', subset: 'storage_classes.yml', selector: 'test=prune1'))
assert_logs_match_all([
"Phase 1: Initializing deploy",
"Using resource selector test=prune1",
"All required parameters and files are present",
"Discovering resources:",
" - StorageClass/testing-storage-class",
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Exists},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Don't know how to monitor resources of type StorageClass.",
dturn marked this conversation as resolved.
Show resolved Hide resolved
%r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
%r{Successfully deployed in [\d.]+s: StorageClass\/testing-storage-class},
"Result: SUCCESS",
"Pruned 1 resource and successfully deployed 1 resource",
dturn marked this conversation as resolved.
Show resolved Hide resolved
"Successful resources",
"StorageClass/testing-storage-class",
])
end

def test_no_prune_global_deploy_black_box_success
assert_deploy_success(deploy_global_fixtures('globals', clean_up: false, selector: 'test=prune2'))
reset_logger
assert_deploy_success(deploy_global_fixtures('globals', subset: 'storage_classes.yml',
selector: "test=prune2", prune: false))
assert_logs_match_all([
"Phase 1: Initializing deploy",
"Using resource selector test=prune2",
"All required parameters and files are present",
"Discovering resources:",
" - StorageClass/testing-storage-class",
"Phase 2: Checking initial resource statuses",
%r{StorageClass\/testing-storage-class[\w-]+\s+Exists},
"Phase 3: Deploying all resources",
%r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
"Don't know how to monitor resources of type StorageClass.",
dturn marked this conversation as resolved.
Show resolved Hide resolved
%r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
%r{Successfully deployed in [\d.]+s: StorageClass\/testing-storage-class},
"Result: SUCCESS",
"Successfully deployed 1 resource",
"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'.

end
end
9 changes: 9 additions & 0 deletions test/unit/cluster_resource_discovery_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ def test_global_resource_kinds_success
end
end

def test_prunable_resources
dturn marked this conversation as resolved.
Show resolved Hide resolved
crd = mocked_cluster_resource_discovery(full_response)
kinds = crd.prunable_resources
assert_equal(kinds.length, 15)
%w(scheduling.k8s.io/v1/PriorityClass storage.k8s.io/v1/StorageClass).each do |kind|
assert_includes(kinds, kind)
dturn marked this conversation as resolved.
Show resolved Hide resolved
end
end

private

def mocked_cluster_resource_discovery(response, success: true)
Expand Down