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

Blog post for Server Side Apply to GA #28964

Merged
merged 3 commits into from
Aug 4, 2021
Merged

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Jul 15, 2021

This adds the blog post for Server Side Apply moving to GA in 1.22

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2021
@k8s-ci-robot k8s-ci-robot requested review from kbarnard10 and sftim July 15, 2021 15:11
@k8s-ci-robot k8s-ci-robot added area/blog Issues or PRs related to the Kubernetes Blog subproject language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 15, 2021
@netlify
Copy link

netlify bot commented Jul 15, 2021

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: 437f38b

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/610ab04948e06d0007bbd88c

😎 Browse the preview: https://deploy-preview-28964--kubernetes-io-main-staging.netlify.app

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Hi. Here's some early feedback. I hope it's useful.

content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
@kbhawkey
Copy link
Contributor

👀

Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

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

Thanks a lot, just a few small changes. Well written!

content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-07-15-server-side-apply-ga.md Outdated Show resolved Hide resolved

The new client-go support makes it much easier to use Server-side Apply in controllers.

When authoring new controllers to use Server-side Apply, a good approach is to have the controller recreate the apply configuration for an object each time it reconciles that object. This ensures that the controller fully reconciles all the fields that it is responsible for. Controllers typically should unconditionally set all the fields they own by setting `Force: true` in the `ApplyOptions`. Controllers must also provide a `FieldManager` name that is unique to the reconciliation loop that apply is called from.

Choose a reason for hiding this comment

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

I'm guessing this replaces deepEquals logic? Wouldn't this add additional load on the api server? But agree that this definitely simplifies the reconciler.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on how this replaces deepEquals?

Copy link

@karunasagark karunasagark Jul 27, 2021

Choose a reason for hiding this comment

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

@apelisse , sorry about the confusion. I was asking if this phrase ('recreate the apply configuration for an object each time it reconciles') meant SSA is a replacement for performing deepEquals before sending create/update request to k8s api server to avoid infinite reconcile loop?

Say, controller is reconciling a CRD and creating Statefulset. Such a controller will watch both CRD and Statefulset. When it performs create/update on the Statefulset, it is trigger the watch event to the controller triggering another reconcile loop. This loop would have performed a deepEquals or something else to prevent repeated invocation of reconcile loop. SSA seems to avoid this, by not bumping up the resourceVersion. But the deepEquals logic is now replaced with a SSA call that is over the network. Hence was wondering if this might add load to the api server. Please correct me if I've got something wrong.

Copy link
Member

@apelisse apelisse Jul 27, 2021

Choose a reason for hiding this comment

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

Your assumptions are all reasonable and great. This should, in general, replace the deepEqual since controllers shouldn't even have to download the object in the first place. We've spent a lot of time benchmarking and optimizating the apiserver so that this doesn't become a problem, thank you!

Choose a reason for hiding this comment

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

Thank you!

@sftim
Copy link
Contributor

sftim commented Jul 27, 2021

Hi. If this is ready for review, please retitle accordingly. The release blog team can assign a publication date.

@chrisnegus
Copy link
Contributor

This blog is currently scheduled for publication on August 6th.

Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

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

This reads really well! I had one nit and suggested a few changes to how URLs were formed. I'm not sure if links to blogs can use the same form, so I didn't suggest changes there.

content/en/blog/_posts/2021-08-13-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-08-13-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-08-13-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-08-13-server-side-apply-ga.md Outdated Show resolved Hide resolved
content/en/blog/_posts/2021-08-13-server-side-apply-ga.md Outdated Show resolved Hide resolved
@jlbutler
Copy link
Contributor

@Jefftree since this is in review, can you remove WIP from the PR title? Thanks!

@sftim
Copy link
Contributor

sftim commented Jul 28, 2021

I'll get Prow to help
/retitle Blog post for Server Side Apply to GA

@k8s-ci-robot k8s-ci-robot changed the title [WIP] blog post for Server Side Apply to GA Blog post for Server Side Apply to GA Jul 28, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2021
@sftim
Copy link
Contributor

sftim commented Aug 1, 2021

Todos:

Adjust publish date
A couple of comments need to be addressed

To help reviewers, @Jefftree, would you be willing to update the PR description so that it is current?

@sftim
Copy link
Contributor

sftim commented Aug 1, 2021

/wg api-expression

@k8s-ci-robot k8s-ci-robot added the wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. label Aug 1, 2021
@sftim sftim requested a review from apelisse August 3, 2021 19:59
@apelisse
Copy link
Member

apelisse commented Aug 4, 2021

Thanks, LGTM for me!

@sftim
Copy link
Contributor

sftim commented Aug 4, 2021

@Jefftree one thing you might want to do is bump your repository's main branch to something more current (it's currently stale by 4763 commits).

Anyway, I tested a merge locally and it still looks fine.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 38fe2a9245503f4eeac7ff53729d7af7d38c381f

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2021
@Jefftree
Copy link
Member Author

Jefftree commented Aug 4, 2021

@Jefftree one thing you might want to do is bump your repository's main branch to something more current (it's currently stale by 4763 commits).

Anyway, I tested a merge locally and it still looks fine.
/lgtm

Rebased on the latest main branch. Thanks

@annajung
Copy link
Contributor

annajung commented Aug 4, 2021

This looks really good!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 380ddcd2087ce6b1d9c02dc0f85fb6904ec5bb5e

@sftim
Copy link
Contributor

sftim commented Aug 4, 2021

/approve
/hold

Can unhold once Kubernetes v1.22 release is finished.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2021
@sftim
Copy link
Contributor

sftim commented Aug 4, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9c4b023 into kubernetes:main Aug 4, 2021
Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

Thanks for the great doc! I have some feedback. Not sure if it's the right place to send.


When authoring new controllers to use Server-side Apply, a good approach is to have the controller recreate the apply configuration for an object each time it reconciles that object. This ensures that the controller fully reconciles all the fields that it is responsible for. Controllers typically should unconditionally set all the fields they own by setting `Force: true` in the `ApplyOptions`. Controllers must also provide a `FieldManager` name that is unique to the reconciliation loop that apply is called from.

When upgrading existing controllers to use Server-side Apply the same approach often works well--migrate the controllers to recreate the apply configuration each time it reconciles any object. Unfortunately, the controller might have multiple code paths that update different parts of an object depending on various conditions. Migrating a controller like this to Server-side Apply can be risky because if the controller forgets to include any fields in an apply configuration that is included in a previous apply request, a field can be accidently deleted. To ease this type of migration, client-go apply support provides a way to replace any controller reconciliation code that performs a "read/modify-in-place/update" (or patch) workflow with a "extract/modify-in-place/apply" workflow. Here's an example of the new workflow:
Copy link
Member

Choose a reason for hiding this comment

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

if the controller forgets to include any fields in an apply configuration that is included in a previous apply request, a field can be accidently deleted

@Jefftree Could you elaborate why this could happen? Is it because the field previously had its field manager set to the controller-- and if the controller omits the field in a future apply, the server will think the controller wants to delete this field?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. Since the field is owned by the controller, it is assumed that the controller cares about the field. Other apply requests to update this field will cause a conflict. Omitting a field from an apply request where the applier is the owner will cause the field to be deleted (assuming there are no other owners of the field). When a controller wants to update an object via apply, it should send all fields that it owns if there is no intent to delete any field.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Thanks for the clarification!


It is strongly recommended that all [Custom Resource Definitions](/docs/concepts/extend-kubernetes/api-extension/custom-resources/) (CRDs) have a schema. CRDs without a schema are treated as unstructured data by Server-side Apply. Keys are treated as fields in a struct and lists are assumed to be atomic.

CRDs that specify a schema are able to specify additional annotations in the schema. Please refer to the documentation on the full list of available annotations.
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to the documentation on the full list of available annotations

Is the link to the documentation missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/blog Issues or PRs related to the Kubernetes Blog subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.
Projects
None yet
Development

Successfully merging this pull request may close these issues.