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

Predeploy more resources #5

Draft
wants to merge 24 commits into
base: upstream-clone
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
737574c
Allow to recognize the predeployed annotation
oct8l Nov 23, 2024
4cc4e62
Allow the mock resource to be designated as predeployed
oct8l Nov 23, 2024
a075097
Fix logic in deciding if something should be predeployed
oct8l Nov 23, 2024
8746eb1
Add functionality in the discovery to respect the predeployed annotation
oct8l Nov 23, 2024
f4e0385
Add Deployments, Services and Jobs to be deployed after CRs
oct8l Nov 23, 2024
9c539c9
Fix a couple tests to be up to speed with the new functionality
oct8l Nov 23, 2024
d70bd2d
Change test to work now that Services can be predeployed
oct8l Nov 23, 2024
ca20352
Add information to the README about resources that can be marked as '…
c-gerke Nov 25, 2024
5b45f3b
Also allow Ingresses to respect the 'predeployed' annotation
c-gerke Nov 26, 2024
52a608d
Remove duplicate 'kind' definition
c-gerke Nov 29, 2024
c598e1e
Refactor predeploy logic and enhance resource handling
c-gerke Dec 2, 2024
873c050
Explicitly set the priority resources to be predeployed
c-gerke Dec 2, 2024
876715a
Clean up (now) unused definitions
c-gerke Dec 2, 2024
2f88b93
Add more resources to the 'after_crs' section of pre-deployment
c-gerke Dec 2, 2024
3af0df3
Documentation updates
c-gerke Dec 2, 2024
f873ef1
Remove a missed definition
c-gerke Dec 2, 2024
042fc44
Revert change to force Role and RoleBinding to be predeployed at the …
c-gerke Dec 2, 2024
bd5bc25
Allow CR to be set to predeployed independent of CRDs
c-gerke Dec 2, 2024
8bbbe90
Fix line references that have changed
c-gerke Dec 2, 2024
9839b6d
Quick and dirty fix to get tests and deployments working properly
c-gerke Dec 3, 2024
bbc989f
Remove accidental newline
c-gerke Dec 3, 2024
6c4c6a9
Refactor predeployed logic for Kubernetes resources
c-gerke Dec 3, 2024
5016db4
Move pod predeploy logic with the rest of the "required to predeploy"…
c-gerke Dec 3, 2024
bfd40a7
Remove the `default_to_predeployed` function from previous iterations…
c-gerke Dec 3, 2024
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
8 changes: 4 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,11 @@ before the deployment is considered successful.
- Percent (e.g. 90%): The deploy is successful when the number of new pods that are ready is equal to `spec.replicas` * Percent.
- _Compatibility_: StatefulSet
- `full`: The deployment is successful when all pods are ready.
- `krane.shopify.io/predeployed`: Causes a Custom Resource to be deployed in the pre-deploy phase.
- _Compatibility_: Custom Resource Definition
- `krane.shopify.io/predeployed`: Causes a Custom Resource, Deployment, Service, or Job to be deployed in the pre-deploy phase.
- _Compatibility_: Custom Resource Definition, Deployment, Service, Job
Copy link
Member

Choose a reason for hiding this comment

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

This might be controversial upstream; the label originally got applied to the resource definition (class) not the concrete instance, so this is now conceptually mixed.

Copy link
Author

Choose a reason for hiding this comment

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

I had been thinking about that as well, that this implementation/change was the result of taking a feature that existed with one purpose and kind of stealing or borrowing it to apply to different objects. I don't doubt it would be controversial upstream, and I'm ready to have those conversations on if this should even be merged upstream too!

I think it's a worthwhile feature/functional change though (I do realize I'm a little close to it to see it completely objectively), and there's at least one other person at some point who was looking for this type of functionality, so I think it's worth at least proposing and discussing if this is something that would be desired in its current state.

- _Default_: `true`
- `true`: The custom resource will be deployed in the pre-deploy phase.
- All other values: The custom resource will be deployed in the main deployment phase.
- `true`: The custom resource, deployment, service, or job will be deployed in the pre-deploy phase.
- All other values: The custom resource, deployment, service, or job will be deployed in the main deployment phase.
- `krane.shopify.io/deploy-method-override`: Cause a resource to be deployed by the specified `kubectl` command, instead of the default `apply`.
- _Compatibility_: Cannot be used for `PodDisruptionBudget`, since it always uses `create/replace-force`
- _Accepted values_: `create`, `replace`, and `replace-force`
Expand Down
51 changes: 51 additions & 0 deletions lib/krane/cluster_resource_discovery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@ def crds
end
end

def services
@services ||= fetch_services.map do |svc|
Service.new(namespace: namespace, context: context, logger: logger,
definition: svc, statsd_tags: @namespace_tags)
end
end

def deployments
@deployments ||= fetch_deployments.map do |deployment|
Deployment.new(namespace: namespace, context: context, logger: logger,
definition: deployment, statsd_tags: @namespace_tags)
end
end

def jobs
@jobs ||= fetch_jobs.map do |job|
Job.new(namespace: namespace, context: context, logger: logger,
definition: job, statsd_tags: @namespace_tags)
end
end

def prunable_resources(namespaced:)
black_list = %w(Namespace Node ControllerRevision Event)
fetch_resources(namespaced: namespaced).map do |resource|
Expand Down Expand Up @@ -98,6 +119,36 @@ def fetch_crds
end
end

def fetch_services
raw_json, err, st = kubectl.run("get", "Service", output: "json", attempts: 5,
use_namespace: false)
if st.success?
MultiJson.load(raw_json)["items"]
else
raise FatalKubeAPIError, "Error retrieving Service: #{err}"
end
end

def fetch_deployments
raw_json, err, st = kubectl.run("get", "Deployment", output: "json", attempts: 5,
use_namespace: false)
if st.success?
MultiJson.load(raw_json)["items"]
else
raise FatalKubeAPIError, "Error retrieving Deployment: #{err}"
end
end

def fetch_jobs
raw_json, err, st = kubectl.run("get", "Job", output: "json", attempts: 5,
use_namespace: false)
if st.success?
MultiJson.load(raw_json)["items"]
else
raise FatalKubeAPIError, "Error retrieving Job: #{err}"
end
end

def kubectl
@kubectl ||= Kubectl.new(task_config: @task_config, log_failure_by_default: true)
end
Expand Down
3 changes: 3 additions & 0 deletions lib/krane/deploy_task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ def predeploy_sequence
).map { |r| [r, default_group] }
c-gerke marked this conversation as resolved.
Show resolved Hide resolved

after_crs = %w(
Deployment
Service
Pod
Job
).map { |r| [r, default_group] }
c-gerke marked this conversation as resolved.
Show resolved Hide resolved

crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, { group: cr.group }] }
Expand Down
5 changes: 5 additions & 0 deletions lib/krane/kubernetes_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,11 @@ def use_generated_name(instance_data)
@file = create_definition_tempfile
end

def predeployed?
predeployed = krane_annotation_value("predeployed")
predeployed.nil? || predeployed == "false"
end

class Event
EVENT_SEPARATOR = "ENDEVENT--BEGINEVENT"
FIELD_SEPARATOR = "ENDFIELD--BEGINFIELD"
Expand Down
8 changes: 8 additions & 0 deletions lib/krane/kubernetes_resource/deployment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ def validate_definition(*, **)
@validation_errors.empty?
end

def predeployed?
krane_annotation_value("predeployed") == "true"
end

def kind
@definition["kind"]
end

c-gerke marked this conversation as resolved.
Show resolved Hide resolved
private

def current_generation
Expand Down
8 changes: 8 additions & 0 deletions lib/krane/kubernetes_resource/job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ def failure_message
end
end

def predeployed?
krane_annotation_value("predeployed") == "true"
end

def kind
@definition["kind"]
end

private

def failed_status_condition
Expand Down
8 changes: 8 additions & 0 deletions lib/krane/kubernetes_resource/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ def timeout_message
"Please confirm the spec.selector is correct and the targeted workload is healthy."
end

def predeployed?
krane_annotation_value("predeployed") == "true"
end

def kind
@definition["kind"]
end

private

def fetch_related_workloads(cache)
Expand Down
3 changes: 2 additions & 1 deletion lib/krane/resource_deployer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ def predeploy_priority_resources(resource_list, predeploy_sequence)
predeploy_sequence.each do |resource_type, attributes|
matching_resources = resource_list.select do |r|
r.type == resource_type &&
(!attributes[:group] || r.group == attributes[:group])
(!attributes[:group] || r.group == attributes[:group]) &&
r.predeployed?
c-gerke marked this conversation as resolved.
Show resolved Hide resolved
end
StatsD.client.gauge('priority_resources.count', matching_resources.size, tags: statsd_tags)

Expand Down
4 changes: 4 additions & 0 deletions test/helpers/mock_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def group
"core"
end

def predeployed?
false
end

def pretty_timeout_type
end

Expand Down
3 changes: 1 addition & 2 deletions test/integration-serial/serial_deploy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,7 @@ def test_cr_success_with_service

assert_deploy_success(deploy_fixtures("crd", subset: %w(web.yml)))

refute_logs_match(/Predeploying priority resources/)
assert_logs_match_all([/Phase 3: Deploying all resources/])
assert_logs_match_all([/Phase 4: Deploying all resources/])
ensure
build_kubectl.run("delete", "-f", filepath, use_namespace: false, log_failure: false)
end
Expand Down
22 changes: 21 additions & 1 deletion test/unit/krane/resource_deployer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ def test_deploy_verify_false_no_failure_error
def test_predeploy_priority_resources_respects_pre_deploy_list
kind = "MockResource"
resource = build_mock_resource
priority_list = { kind => { group: "not-#{resource.group}" } }
resource_deployer(kubectl_times: 0).predeploy_priority_resources([resource], priority_list)
end

def test_predeploy_priority_resources_respects_pre_deploy_list_and_predeployed_true_annotation
kind = "MockResource"
resource = build_mock_resource
resource.expects(:predeployed?).returns(true)
watcher = mock("ResourceWatcher")
watcher.expects(:run).returns(true)
# ResourceDeployer only creates a ResourceWatcher if one or more resources
Expand All @@ -76,6 +84,18 @@ def test_predeploy_priority_resources_respects_pre_deploy_list
resource_deployer.predeploy_priority_resources([resource], priority_list)
end

def test_predeploy_priority_resources_respects_pre_deploy_list_and_predeployed_false_annotation
kind = "MockResource"
resource = build_mock_resource
resource.expects(:predeployed?).returns(false)
# ResourceDeployer only creates a ResourceWatcher if one or more resources
# are deployed. See test_predeploy_priority_resources_respects_empty_pre_deploy_list
# for counter example
Krane::ResourceWatcher.expects(:new).never
priority_list = { kind => { group: "core" } }
resource_deployer(kubectl_times: 0).predeploy_priority_resources([resource], priority_list)
end

def test_predeploy_priority_resources_respects_empty_pre_deploy_list
resource = build_mock_resource
priority_list = []
Expand All @@ -98,4 +118,4 @@ def resource_deployer(kubectl_times: 1, prune_allowlist: [])
def build_mock_resource(final_status: "success", hits_to_complete: 0, name: "web-pod")
MockResource.new(name, hits_to_complete, final_status)
end
end
end
Loading