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

Support strategic merge patch for custom resources #58414

Closed
nikhita opened this issue Jan 17, 2018 · 10 comments
Closed

Support strategic merge patch for custom resources #58414

nikhita opened this issue Jan 17, 2018 · 10 comments
Assignees
@liggitt
Labels
area/custom-resources kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@nikhita
Copy link
Member

nikhita commented Jan 17, 2018

We should support strategic merge patch (smp) for custom resources if we plan to move CRDs to GA [1].

Currently, we face the following issues with regards to not supporting strategic merge patch:

/cc @sttts @deads2k @enisoc @mbohlool @liggitt @lavalamp @caesarxuchao @jennybuckley

[1] https://groups.google.com/forum/#!topic/kubernetes-sig-api-machinery/07JKqCzQKsc
@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jan 17, 2018
@nikhita
Copy link
Member Author

nikhita commented Jan 17, 2018

/area custom-resources
/sig api-machinery
/kind feature

@k8s-ci-robot k8s-ci-robot added area/custom-resources sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/feature Categorizes issue or PR as related to a new feature. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 17, 2018
@liggitt
Copy link
Member

liggitt commented Jan 17, 2018

We should support strategic merge patch (smp) for custom resources if we plan to move CRDs to GA

I'm not sure I agree. Strategic merge patch has proved problematic both client-side and server-side. Designing a major addition to it seems like an investment in a direction we don't want to go.

@sttts
Copy link
Contributor

sttts commented Jan 19, 2018

I'm not sure I agree.

Let's clarify: we need PATCH conflict retries for CRDs. If that is through SMP or something else does not matter. Or we officially accept that the PATCH behaviour is different for CRDs. Not sure we want this kind compromises.

@nikhita nikhita mentioned this issue Jan 23, 2018
Closed
54 tasks
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2018
@nikhita
Copy link
Member Author

nikhita commented Apr 20, 2018

I'm not sure if we are going to support this but removing the stale label until it is decided what to do.

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 20, 2018
@lavalamp
Copy link
Member

lavalamp commented Apr 20, 2018 via email

@nikhita
Copy link
Member Author

nikhita commented Apr 20, 2018

JSONPatch and JSON merge patch should work today, I think?

Yes, they do. 👍

@liggitt
Copy link
Member

liggitt commented Apr 25, 2018

SMP requirement for patch is removed by #63146

GC reliance on SMP should be tracked/resolved in #56348

agree with @lavalamp that we don't want to extend SMP usage to CRD

/close

@liggitt liggitt mentioned this issue May 3, 2018
Closed
@nikhita nikhita mentioned this issue May 13, 2018
Merged
@roycaihw roycaihw mentioned this issue May 26, 2018
Merged
@tillig tillig mentioned this issue Oct 3, 2018
Closed
@caesarxuchao caesarxuchao mentioned this issue Oct 9, 2018
Merged
@cben
Copy link

cben commented Oct 23, 2018

https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#advanced-features-and-flexibility says for strategic-merge-patch / CRDs column: "No, but similar functionality planned". Should this be updated to "No" or "Not planned"?

@cben cben mentioned this issue Oct 23, 2018
Closed
@nikhita nikhita mentioned this issue Nov 14, 2018
Merged
@nikhita
Copy link
Member Author

nikhita commented Nov 14, 2018

Should this be updated to "No" or "Not planned"?

@cben Thanks for pointing it out. Created kubernetes/website#10994 to say "No".

This was referenced Aug 1, 2019
Closed
Closed
@krishnadurai krishnadurai mentioned this issue Sep 24, 2019
Closed
@thatsmydoing thatsmydoing mentioned this issue Nov 14, 2019
Closed
3 tasks
@Lucaber Lucaber mentioned this issue Apr 15, 2020
Closed
@cben cben mentioned this issue Jun 14, 2020
Closed
@clux clux mentioned this issue Nov 25, 2020
Closed
@stevenctl stevenctl mentioned this issue Dec 10, 2020
Closed
@rgolangh rgolangh mentioned this issue Feb 18, 2021
Merged
@hasheddan hasheddan mentioned this issue Feb 23, 2021
Merged
2 tasks
@monopole monopole mentioned this issue Mar 14, 2021
Closed
@katrogan katrogan mentioned this issue Nov 22, 2021
Merged
8 tasks
@dgkanatsios dgkanatsios mentioned this issue Jul 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@liggitt liggitt

Labels
area/custom-resources kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@cben @lavalamp @sttts @liggitt @nikhita @k8s-ci-robot @fejta-bot