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

Make the k8s store properly patch annotations #740

Closed

Conversation

sandhose
Copy link
Contributor

@sandhose sandhose commented Dec 7, 2019

Hi,

While trying out Stolon on Kubernetes, I ran into a weird issue where Stolon was unable to change the annotations on the sentinel and keeper pods. The api-server said it was trying to change some fields that were immutable. This was due to Stolon trying to replace the whole pod object instead of patching it.

This PRs changes the k8s store to properly use jsonpatch to set the annotations. This is heavily inspired by the way kubectl annotate does patch the annotations: https://github.com/kubernetes/kubectl/blob/b909fcb4a071a1a9669a9fe1f48482c848823124/pkg/cmd/annotate/annotate.go#L277-L311

@dineshba
Copy link
Contributor

dineshba commented Dec 9, 2019

Code looks good to me. @sandhose How to test this feature?

@sgotti
Copy link
Member

sgotti commented Dec 9, 2019

@sandhose Thanks for the PR. Issue #678 is probably related. I'll review it in the next days.

@ccmehk
Copy link

ccmehk commented Dec 10, 2019

This look good, i want a quick patch on this

internal/store/k8s.go Outdated Show resolved Hide resolved
Previously it tried to replace the whole object, which might cause
issues with newer versions of Kubernetes.
@sandhose sandhose force-pushed the fix-kubernetes-annotations branch from 15cccd7 to 5e2486f Compare December 10, 2019 17:14
@sgotti
Copy link
Member

sgotti commented Dec 11, 2019

@sandhose let me know when you're ready for another review (I saw you pushed an updated but it didn't fixed my comments).

If you're interested this is what I had in mind to simplify this PR: sgotti@de8eddd

Please also update the commit message to follow our conventions and better describe what this patch does:

k8s store: patch pod annotations

Use k8s client mpatch method to update pod annotations (in some versions
replacing the whole pod object will return an error)

@Josua-SR
Copy link

Josua-SR commented Jan 15, 2020

@sandhose I came across a similar issue when I was trying to deploy stolon, and this PR definitely solved it on my end - thank you very much!

(tried on v0.15.0)

@sgotti
Copy link
Member

sgotti commented Jan 17, 2020

@sandhose Since there was no activity and I'd like to merge this soon I created a new PR #751 with the proposed changes.

@sgotti sgotti closed this Jan 17, 2020
@sandhose
Copy link
Contributor Author

@sgotti Thanks for picking that up, I really did not had time to finish this those past weeks

@sgotti
Copy link
Member

sgotti commented Jan 27, 2020

@sandhose Thanks to you! I left you as the author of the commit in the new PR.

@sandhose sandhose deleted the fix-kubernetes-annotations branch January 27, 2020 13:42
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.

5 participants