-
Notifications
You must be signed in to change notification settings - Fork 115
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
Compare generations when available #325
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,8 @@ def deploy_succeeded? | |
end | ||
|
||
def deploy_failed? | ||
pods.present? && pods.any?(&:deploy_failed?) | ||
pods.present? && pods.any?(&:deploy_failed?) && | ||
observed_generation == current_generation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this not be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand it, and @KnVerey will hopefully correct me if I'm wrong, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh! Thanks for the clarification |
||
end | ||
|
||
def fetch_logs(kubectl) | ||
|
@@ -35,16 +36,6 @@ def fetch_logs(kubectl) | |
|
||
private | ||
|
||
def current_generation | ||
return -1 unless exists? # must be different default than observed_generation | ||
@instance_data["metadata"]["generation"] | ||
end | ||
|
||
def observed_generation | ||
return -2 unless exists? | ||
@instance_data["status"]["observedGeneration"] | ||
end | ||
|
||
def rollout_data | ||
return { "currentNumberScheduled" => 0 } unless exists? | ||
@instance_data["status"] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
--- | ||
apiVersion: policy/v1beta1 | ||
kind: PodDisruptionBudget | ||
metadata: | ||
name: test | ||
generation: 2 | ||
spec: | ||
minAvailable: 2 | ||
selector: | ||
matchLabels: | ||
name: web | ||
app: hello-cloud | ||
status: | ||
observedGeneration: 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
--- | ||
apiVersion: extensions/v1beta1 | ||
kind: ReplicaSet | ||
metadata: | ||
name: test | ||
generation: 2 | ||
spec: | ||
replicas: 3 | ||
selector: | ||
matchLabels: | ||
app: hello-cloud | ||
name: test | ||
template: | ||
metadata: | ||
labels: | ||
app: hello-cloud | ||
name: test | ||
spec: | ||
containers: | ||
- name: app | ||
image: busybox | ||
imagePullPolicy: IfNotPresent | ||
command: ["tail", "-f", "/dev/null"] | ||
status: | ||
observedGeneration: 2 | ||
availableReplicas: 3 | ||
readyReplicas: 3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
apiVersion: apps/v1beta1 | ||
kind: StatefulSet | ||
metadata: | ||
name: test-ss | ||
generation: 2 | ||
spec: | ||
selector: | ||
matchLabels: | ||
name: test-ss | ||
app: hello-cloud | ||
serviceName: "test-ss" | ||
updateStrategy: | ||
type: RollingUpdate | ||
replicas: 2 | ||
template: | ||
metadata: | ||
labels: | ||
app: hello-cloud | ||
name: test-ss | ||
spec: | ||
containers: | ||
- name: app | ||
image: busybox | ||
imagePullPolicy: IfNotPresent | ||
command: ["tail", "-f", "/dev/null"] | ||
status: | ||
replicas: 2 | ||
readyReplicas: 2 | ||
currentReplicas: 2 | ||
observedGeneration: 2 | ||
currentRevision: 2 | ||
updateRevision: 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# frozen_string_literal: true | ||
require 'test_helper' | ||
|
||
class PodDisruptionBudgetTest < KubernetesDeploy::TestCase | ||
def setup | ||
KubernetesDeploy::Kubectl.any_instance.expects(:run).never | ||
super | ||
end | ||
|
||
def test_deploy_succeeded_is_true_as_soon_as_controller_observes_new_version | ||
template = build_pdb_template(status: { "observedGeneration": 2 }) | ||
pdb = build_synced_pdb(template: template) | ||
assert_predicate pdb, :deploy_succeeded? | ||
end | ||
|
||
def test_deploy_succeeded_not_fooled_by_stale_status | ||
template = build_pdb_template(status: { "observedGeneration": 1 }) | ||
pdb = build_synced_pdb(template: template) | ||
refute_predicate pdb, :deploy_succeeded? | ||
end | ||
|
||
private | ||
|
||
def build_pdb_template(status: {}) | ||
pdb_fixture.dup.deep_merge("status" => status) | ||
end | ||
|
||
def build_synced_pdb(template:) | ||
pdb = KubernetesDeploy::PodDisruptionBudget.new(namespace: "test", context: "nope", | ||
logger: logger, definition: template) | ||
sync_mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) | ||
sync_mediator.kubectl.expects(:run).with("get", "PodDisruptionBudget", "test", "-a", "--output=json").returns( | ||
[template.to_json, "", SystemExit.new(0)] | ||
) | ||
pdb.sync(sync_mediator) | ||
pdb | ||
end | ||
|
||
def pdb_fixture | ||
@pdb_fixture ||= YAML.load_stream( | ||
File.read(File.join(fixture_path('for_unit_tests'), 'pod_disruption_budget_test.yml')) | ||
).find { |fixture| fixture["kind"] == "PodDisruptionBudget" } | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
# frozen_string_literal: true | ||
require 'test_helper' | ||
|
||
class ReplicaSetTest < KubernetesDeploy::TestCase | ||
def setup | ||
KubernetesDeploy::Kubectl.any_instance.expects(:run).never | ||
super | ||
end | ||
|
||
def test_deploy_succeeded_is_true_when_generation_and_replica_counts_match | ||
template = build_rs_template(status: { "observedGeneration": 2 }) | ||
rs = build_synced_rs(template: template) | ||
assert_predicate rs, :deploy_succeeded? | ||
end | ||
|
||
def test_deploy_succeeded_not_fooled_by_stale_status | ||
template = build_rs_template(status: { "observedGeneration": 1 }) | ||
rs = build_synced_rs(template: template) | ||
refute_predicate rs, :deploy_succeeded? | ||
end | ||
|
||
def test_deploy_failed_ensures_controller_has_observed_deploy | ||
template = build_rs_template(status: { "observedGeneration": 1 }) | ||
rs = build_synced_rs(template: template) | ||
rs.stubs(:pods).returns([stub(deploy_failed?: true)]) | ||
refute_predicate rs, :deploy_failed? | ||
end | ||
|
||
private | ||
|
||
def build_rs_template(status: {}) | ||
rs_fixture.dup.deep_merge("status" => status) | ||
end | ||
|
||
def build_synced_rs(template:) | ||
rs = KubernetesDeploy::ReplicaSet.new(namespace: "test", context: "nope", logger: logger, definition: template) | ||
sync_mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) | ||
sync_mediator.kubectl.expects(:run).with("get", "ReplicaSet", "test", "-a", "--output=json").returns( | ||
[template.to_json, "", SystemExit.new(0)] | ||
) | ||
sync_mediator.kubectl.expects(:run).with("get", "Pod", "-a", "--output=json", anything).returns( | ||
['{ "items": [] }', "", SystemExit.new(0)] | ||
) | ||
rs.sync(sync_mediator) | ||
rs | ||
end | ||
|
||
def rs_fixture | ||
@rs_fixture ||= YAML.load_stream( | ||
File.read(File.join(fixture_path('for_unit_tests'), 'replica_set_test.yml')) | ||
).find { |fixture| fixture["kind"] == "ReplicaSet" } | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do you have an example of a controller that doesn't populate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all objects have metadata generations, theoretically any controller could be setting observedGeneration, though this is only meaningful if the controller is writing a status. An example of a resource that has a status but no observedGeneration would be Job or CronJob. See also this k8s core issue. As a sidenote, in 1.11 (I think it made beta there?), CRs will have a reliable
metadata.generation
we can use to do this properly in our own controllers. Our generic CR success feature should make use of it when available.