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

[RFC] Server-side reconciliation for the v1beta2 API #426

Merged
merged 21 commits into from
Oct 7, 2021
Merged

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Sep 11, 2021

This PR introduces a new reconciler based on Kubernetes server-side apply and graduates the API to v1beta2. Part of fluxcd/flux2#1889.

💡 Motivation

  • Improve performance (CPU, memory, network, FD usage) and reduce the number of calls to Kubernetes API by replacing kubectl execs with a specialized applier written in Go.
  • Being able to validate and reconcile sources that contain both CRDs and CRs.
  • Detect and report drift between the desired state (git, s3, etc) and cluster state reliably.
  • Preview local changes to manifests without pushing to upstream (flux diff -k command TBA).
  • Being able to wait for all applied resources to become ready without requiring users to fill in the health checks list.
  • Improve the overall observably of the reconciliation process by reporting in real-time the garbage collection and health assessment actions.

⏳ Background

When we started Flux v2, we've set a goal to stop relying on 3rd party binaries for core features. While we've successfully replaced the Git CLI shell execs with Go libraries (go-git, git2go) and C libraries (libgit2, libssh2), the kustomize CLI with Go libraries (kustomize/api, kustomize/kyaml), we're still depending on the kubectl CLI for the 3-way-merge apply feature. With Kubernetes server-side apply being promoted to GA, we can finally get rid of kubectl and drive the reconciliation using exclusively the controller-runtime Go client.

💥 Breaking changes

  • Namespaced objects must contain metadata.namespace, defaulting to the default namespace is no longer supported.
  • The logs, events and alerts that report Kubernetes namespaced object changes are now using the Kind/Namespace/Name format instead of Kind/Name e.g.:
Service/flux-demo/podinfo unchanged
Deployment/flux-demo/podinfo configured
HorizontalPodAutoscaler/flux-demo/podinfo deleted
  • The minimum supported version of Kubernetes has changed to:
Kubernetes version Minimum required
v1.16 >= 1.16.11
v1.17 >= 1.17.7
v1.18 >= 1.18.4
v1.19 and later >= 1.19.0

All the versions above fix a regression in the managed fields and field type.

API changes

The kustomize.toolkit.fluxcd.io/v1beta2 API is backwards compatible with v1beta1.

Additions and deprecations:

  • .spec.validation deprecated (server-side validation is implicit)
  • .spec.patchesStrategicMerge deprecated in favour of .spec.patches
  • .spec.patchesJson6902 deprecated in favour of .spec.patches
  • .status.snapshot replaced by .status.inventory
  • .spec.wait added (when enabled the controller will wait for all the reconciled resources to become ready)

Reconciler changes

The server-side reconciler comes with the following behavioural changes:

Testing

The SSA reconciler and its controller are tested using Go stdlib, gomega and controller-runtime envtest:

 github.com/fluxcd/kustomize-controller/controllers coverage:  71.2% of statements

Due to the controller-runtime envtest limitations (no kube-controller-manager), Kubernetes Kind e2e tests where added to the GitHub Actions workflow to cover features such as waiting for deployments rollout and CRDs+CRs staged apply.

Feedback

Please comment on this PR and let us know your thoughts about this.

If you want to try out the v1beta2 API on your own test cluster:

  1. Install the latest Flux controllers
flux install
  1. Apply the CRDs from this branch
 kubectl apply -k https://github.com/fluxcd/kustomize-controller/config/crd?ref=v1beta2
  1. Deploy the kustomize-controller build of this branch
kubectl -n flux-system set image deployment/kustomize-controller \
manager=ghcr.io/fluxcd/kustomize-controller:v1beta2-50c71354

What's next?

  • Move the SSA resource manager to fluxcd/pkg/ssa
  • Use the SSA manager in Flux CLI to replace kubectl shell execs for flux bootstrap and flux install
  • Bump the minimum Kubernetes version to 1.18.8 in flux check --pre

@stefanprodan stefanprodan added area/docs Documentation related issues and pull requests enhancement New feature or request area/kustomize Kustomize related issues and pull requests area/kstatus Health checking related issues and pull requests labels Sep 11, 2021
const (
KustomizationController = "kustomize-controller"
KustomizationKind = "Kustomization"
KustomizationFinalizer = "finalizers.fluxcd.io"
Copy link
Member

Choose a reason for hiding this comment

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

We have had a discussion about this in the past, but I think it would be wise if we would make the finalizers domain specific. Reason for this is that the pattern of using various finalizers to control the garbage collection process of an object if multiple reconcilers are dealing with an object has become more common.

I think your objection in the past was that removing "all Flux reconcilers" in cases where they get stuck would become more difficult, but if we would nest all under a sub(domain)name, you could still loop over all that match that suffix. E.g. applier.finalizers.fluxcd.io, source.finalizers.fluxcd.io.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we change this now, wouldn't result in orphan resources after a Flux upgrade? I would introduce this change into a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

Follow up PR (e.g. bundled with the other refactoring efforts) is fine with me, just wanted to have it noted given it showed up again :-)

@somtochiama
Copy link
Member

somtochiama commented Sep 13, 2021

Tested this PR on kubernetes version 1.16..13, 1.17.17, 1.18.8, 1.19.1, 1.20.2 and it works💥.
It fails on 1.18.2 with the following error:

{"level":"error","ts":"2021-09-13T15:02:33.963Z","logger":"controller.kustomization","msg":"unable to update status after reconciliation","reconciler group":"kustomize.toolkit.fluxcd.io","reconciler kind":"Kustomization","name":"podinfo","namespace":"flux-system","error":"failed to update object (json PATCH for kustomize.toolkit.fluxcd.io/v1beta2, Kind=Kustomization) managed fields: failed to update ManagedFields: failed to convert new object: .status.inventory: field not declared in schema"}
{"level":"error","ts":"2021-09-13T15:02:33.964Z","logger":"controller.kustomization","msg":"Reconciler error","reconciler group":"kustomize.toolkit.fluxcd.io","reconciler kind":"Kustomization","name":"podinfo","namespace":"flux-system","error":"failed to update object (json PATCH for kustomize.toolkit.fluxcd.io/v1beta2, Kind=Kustomization) managed fields: failed to update ManagedFields: failed to convert new object: .status.inventory: field not declared in schema"}
{"level":"info","ts":"2021-09-13T15:02:34.495Z","logger":"controller.kustomization","msg":"server-side apply completed","reconciler group":"kustomize.toolkit.fluxcd.io","reconciler kind":"Kustomization","name":"podinfo","namespace":"flux-system","output":{"Deployment/flux-system/podinfo":"unchanged","HorizontalPodAutoscaler/flux-system/podinfo":"unchanged","Service/flux-system/podinfo":"unchanged"}}

This is likely due to k/k#91748

api/v1beta2/groupversion_info.go Outdated Show resolved Hide resolved
api/v1beta2/groupversion_info.go Outdated Show resolved Hide resolved
api/v1beta2/inventory_types.go Outdated Show resolved Hide resolved
api/v1beta2/kustomization_types.go Outdated Show resolved Hide resolved
api/v1beta2/kustomization_types.go Outdated Show resolved Hide resolved
api/v1beta2/reference_types.go Outdated Show resolved Hide resolved
api/v1beta2/reference_types.go Outdated Show resolved Hide resolved
api/v1beta2/reference_types.go Outdated Show resolved Hide resolved
api/v1beta2/reference_types.go Outdated Show resolved Hide resolved
internal/ssa/manager_apply_test.go Outdated Show resolved Hide resolved
@stefanprodan stefanprodan changed the title Server-side reconciliation for the v1beta2 API [RFC] Server-side reconciliation for the v1beta2 API Sep 14, 2021
@stefanprodan stefanprodan marked this pull request as ready for review September 14, 2021 10:43
@squaremo
Copy link
Member

The kustomize.toolkit.fluxcd.io/v1beta2 API is backwards compatible with v1beta1.
[...]

  • .spec.validation removed (server-side validation is implicit)

Is that field ignored or removed, and if the latter, does this mean a v1beta1 resource won't validate as v1beta2?

@stefanprodan
Copy link
Member Author

Is that field ignored or removed, and if the latter, does this mean a v1beta1 resource won't validate as v1beta2?

I haven't bumped the API version in the e2e tests to prove that the v1beta1 CRs work as expected. What happens is that the Kubernetes API drops all unknown fields before validating the CR, so validation works since .spec.validation is removed.

@squaremo
Copy link
Member

In the linked issue, it's explained that waiting on everything wouldn't work because some resource types are incompatible with kstatus (and this is the motivation for suggesting an allow-list or patterns). The change quoted above would make the controller wait on everything -- so is it the case that

  1. the issue report is mistaken, and it's fine to wait on everything
  2. the mechanism here works around incompatibility with kstatus
  3. this will not work when you're using such resources

@stefanprodan
Copy link
Member Author

stefanprodan commented Sep 16, 2021

@squaremo all Kubernetes builtin kinds are compatible with kstatus. As for custom resources, if the those have no status, or no ready condition in status, or no observed generation in status, then kstatus instead of crashing will report those resources as ready.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
darkowlzz and others added 2 commits October 6, 2021 19:15
In suite test, the context created by SetupSignalHandler() watches for
shutdown signal to cancel the context. This makes it possible to stop
the controllers by sending a kill signal that cancels the context.

This change allows controller context cancellation by creating another
context from SetupSignalHandler() context with a CancelFunc that's
called at the end of the test, instead of sending a kill signal.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
envtest: Add cancellable context to stop controllers
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Although the PR seems to be immense in LOC, most of the changes turned out to actually be tests.

All looks sensible to me @stefanprodan, nice work 🥇💯

darkowlzz and others added 3 commits October 7, 2021 16:21
testenv now supports provisioning users. Replace envtest with testenv.

Also, reorder the cleanup to stop the test environment before stopping
the file server to avoid anything in the cluster trying to connect to
the file server after it's stopped.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@dsogari
Copy link

dsogari commented Jan 11, 2022

Why can't we have the option to disable the dry-run behavior? The implicit behavior fails for me when deploying a kube-prometheus stack:

Pod/monitoring/kube-prometheus-grafana-test dry-run failed, reason: Forbidden, error: pods "kube-prometheus-grafana-test" is forbidden: error looking up service account monitoring/kube-prometheus-grafana-test: serviceaccount "kube-prometheus-grafana-test" not found

It fails because the service account mentioned is only created during the actual reconciliation.

@kingdonb
Copy link
Member

kingdonb commented Jan 14, 2022

@dsogari Please take your inquiry to a new issue, or https://github.com/fluxcd/flux2/discussions

It is not likely to get positive attention here (necro-bumped a PR that is several months old), and there is not enough information in your post to help you, nor this is not the forum.

@stefanprodan stefanprodan added the area/server-side-apply SSA related issues and pull requests label Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment