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

Keep resources with ownerReference in stack prune #159

Merged
merged 7 commits into from
Sep 4, 2019
Merged

Conversation

kke
Copy link
Contributor

@kke kke commented Sep 4, 2019

Server resources with non-empty metadata.ownerReferences will be left untouched during stack prune.

(needs to be backported to v0.10)

@kke kke added the bug Something isn't working label Sep 4, 2019
Copy link
Contributor

@jakolehm jakolehm left a comment

Choose a reason for hiding this comment

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

LGTM. @jnummelin PTAL.

@jnummelin
Copy link
Contributor

any possibility to create some specs for these cases?

@jakolehm
Copy link
Contributor

jakolehm commented Sep 4, 2019

any possibility to create some specs for these cases?

Yeah, specs would be great.

Copy link
Contributor

@jnummelin jnummelin left a comment

Choose a reason for hiding this comment

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

AFAIK this will not fix the real issue at all

lib/k8s/stack.rb Outdated
@@ -101,6 +101,9 @@ def apply(client, prune: true)
if !server_resource
logger.info "Create resource #{resource.apiVersion}:#{resource.kind}/#{resource.metadata.name} in namespace #{resource.metadata.namespace} with checksum=#{resource.checksum}"
keep_resource! client.create_resource(prepare_resource(resource))
elsif server_resource.metadata&.ownerReferences && !server_resource.metadata.ownerReferences.empty?
logger.info "Server resource #{server_resource.apiVersion}:#{server_resource.apiKind}/#{server_resource.metadata.name} in namespace #{server_resource.metadata.namespace} has an ownerReference and will be kept"
keep_resource! server_resource
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I don't think this will work.

keep_resource! server_resource

will make @keep_resources map to have nil value for the "parsed" resource key. It'll later on fail at keep_resource? on return false unless keep_annotation as the keep_annotation will be nil

@keep_resources["#{resource.kind}:#{resource.metadata.name}@#{resource.metadata.namespace}"] == resource.metadata&.annotations&.dig(@checksum_annotation)
keep_annotation = @keep_resources["#{resource.kind}:#{resource.metadata.name}@#{resource.metadata.namespace}"]
return false unless keep_annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check the owner reference count here before trying to check anything else. if there's owners, keep it.

Copy link
Contributor Author

@kke kke Sep 4, 2019

Choose a reason for hiding this comment

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

the keep_resource? won't be called when pruning with keep_resources: false (stack.delete / K8s::Stack.delete) (so implemented in the prune conditional monster)

@kke
Copy link
Contributor Author

kke commented Sep 4, 2019

Yeah seems like the "fix" is now in a branch that does not even get executed for extra server resources.

The original fix in earlier commit was proper.

@kke kke merged commit 5552c97 into master Sep 4, 2019
@kke kke deleted the fix/stack_prune branch September 4, 2019 12:09
@kke kke mentioned this pull request Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants