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

Add support for recreating objects when immutable fields are updated #271

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

relu
Copy link
Member

@relu relu commented Feb 13, 2021

Allow passing --force to kubectl apply. Useful when dealing with
immutable field changes in resources.

I have a use case in which I'm dealing with migration kustomization that is a dependency for an application overlay. The migration kustomization is using a job and it would be really useful if the controller would support force replacements.

Note that this does not work with server-side validation.

Fix: #122

@@ -550,8 +550,8 @@ func (r *KustomizationReconciler) validate(ctx context.Context, kustomization ku
applyCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()

cmd := fmt.Sprintf("cd %s && kubectl apply -f %s.yaml --timeout=%s --dry-run=%s --cache-dir=/tmp",
dirPath, kustomization.GetUID(), kustomization.GetTimeout().String(), kustomization.Spec.Validation)
cmd := fmt.Sprintf("cd %s && kubectl apply -f %s.yaml --timeout=%s --dry-run=%s --cache-dir=/tmp --force=%t",
Copy link
Member Author

Choose a reason for hiding this comment

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

This will fail if server validation is used because --force is not supported. I've added it intentionally so that the user can get some feedback:

validation failed: error: --dry-run=server cannot be used with --force

Copy link
Member

Choose a reason for hiding this comment

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

I propose we skip validation when force is enabled. We need to update the field description with this behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I thought first as well but considering that client-side validation still works I thought it would be useful to have it enabled just for that. Do you think it's not really useful in that situation?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should downgrade the server-side dry-run to client, when force is enabled.

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, that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

This is not described on the API 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.

Added. Thank you, @hiddeco!

@stefanprodan
Copy link
Member

I don't think apply --force does what you are after, in case of a Job, it has not effect. A forced apply, recreates the object only if patching fails (e.g. immutable field error). You need replace --force to restart the job and I don't think kustomize-controller should perform such an operation.

To run jobs as part of the reconciliation, we could offer something similar to Helm hooks:

apiVersion: kustomize.toolkit.fluxcd.io/v1beta1
kind: Kustomization
metadata:
  name: app
spec:
  interval: 5m
  prune: true
  sourceRef:
    kind: GitRepository
    name: webapp
  path: "./webapp/deploy/"
  hooks:
    - type: pre-apply
      path: "./webapp/pre-apply-jobs/" 
    - type: post-apply
      path: "./webapp/post-apply-jobs/"

@relu
Copy link
Member Author

relu commented Feb 14, 2021

Actually, for my use case, this works just fine because the changes made are always going to affect immutable fields (the image tag would be updated, to be more precise). I've tested it locally and it works as intended, the job is being replaced and a new pod is started.
I think there's still value in this for situations when users don't want to manually intervene when dealing with immutable field updates (for deployments and other workload resources) but rather force the update, it can be a temporary thing in the context of a migration or reconfiguration (e.g. label selectors) that can be reverted later.

I really like the idea of adding support for hooks though, would be a great addition to the customize-controller.

@stefanprodan stefanprodan changed the title Support --force apply Add support for recreating objects when immutable fields are updated Feb 14, 2021
@relu relu force-pushed the force-apply branch 2 times, most recently from 081090e to 274d108 Compare February 15, 2021 12:35
@relu
Copy link
Member Author

relu commented Feb 15, 2021

I don't understand this spec error:

{"reconciler group": "kustomize.toolkit.fluxcd.io", "reconciler kind": "Kustomization", "name": "6re3a", "namespace": "kustomization-testti307", "revision": "v1", "error": "validation failed: error: error validating \"792d37ed-59f5-4e58-9c5d-6a47d3b2defe.yaml\": error validating data: ValidationError(ConfigMap): unknown field \"immutable\" in io.k8s.api.core.v1.ConfigMap; if you choose to ignore these errors, turn validation off with --validate=false\n"}

Immutable ConfigMaps are supported since v1.18, we're on v1.20. I first thought I had a local issue but it's reproducible here as well.

@stefanprodan
Copy link
Member

@relu you need to configure Kind like so:

      - name: Setup Kubernetes
        uses: engineerd/setup-kind@v0.5.0
        with:
          version: "v0.10.0"
          image: kindest/node:v1.20.2@sha256:8f7ea6e7642c0da54f04a7ee10431549c0257315b3a634f6ef2fecaaedb19bab

@relu
Copy link
Member Author

relu commented Feb 15, 2021

@stefanprodan kind is not used for these tests right now, it defaults to envtest's mock cluster since TEST_USE_EXISTING_CLUSTER is not set to true in the workflow.

I'll try to use kind locally and see if it's still reproducible, I am currently running exactly that version you pointed to in your comment.

@stefanprodan
Copy link
Member

@relu I get now what you're saying, we need to figure out how to tell envtest to use a newer Kubernetes version.

@hiddeco
Copy link
Member

hiddeco commented Feb 15, 2021

You can configure custom binaries to be used by setting TEST_ASSET_* (or setting Environment#BinaryAssetsDirectory), see: https://book.kubebuilder.io/reference/envtest.html

@stefanprodan
Copy link
Member

I think I found the issue, https://github.com/fluxcd/pkg/blob/main/actions/kubebuilder/entrypoint.sh#L5 this pulls old binaries.

@relu
Copy link
Member Author

relu commented Feb 15, 2021

Indeed, see: https://github.com/fluxcd/kustomize-controller/pull/271/checks?check_run_id=1903336363 (Debug failure section).

Client Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.2", GitCommit:"f5743093fd1c663cb0cbc89748f730662345d44d", GitTreeState:"clean", BuildDate:"2020-09-16T13:41:02Z", GoVersion:"go1.15", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.0", GitCommit:"70132b0f130acc0bed193d9ba59dd186f0e634cf", GitTreeState:"clean", BuildDate:"2020-01-14T00:09:19Z", GoVersion:"go1.13.4", Compiler:"gc", Platform:"linux/amd64"}

@relu relu force-pushed the force-apply branch 2 times, most recently from 3feb200 to d3cdbe6 Compare February 15, 2021 14:58
@stefanprodan
Copy link
Member

stefanprodan commented Feb 15, 2021

Relu I think you should use a Pod and change the label selector. We can't upgrade kubebuilder since it's alpha.

@relu
Copy link
Member Author

relu commented Feb 15, 2021

I don't think I can use workload resources, they will not reconcile on update and cause apply to time-out, I even tried with Service and got the same result. These work just fine for the initial apply during when they are created, but the second one will just time-out.:disappointed:
Do you happen to know any other non-workload resources that might have immutable fields?

@stefanprodan
Copy link
Member

@relu check out the pod tests that @phillebaba wrote here https://github.com/fluxcd/kustomize-controller/pull/255/files

@relu
Copy link
Member Author

relu commented Feb 18, 2021

I tried different resources ConfigMap, Deployment, Job, Service, and for all of them I get the same issue: kubectl apply gets stuck, timing-out during the second reconciliation (after the manifests are updated). This works perfectly fine if I test it on a real cluster so I think it's probably an issue with envtest itself.

@stefanprodan
Copy link
Member

stefanprodan commented Feb 18, 2021

Ok, so we can use your tests only after kubebuilder v3 gets released as it comes with a Kubernetes version that knows about immutable ConfigMap. Remove the tests from this branch, and maybe create a new WIP PR with only tests, and we'll get back to it after kubebuilder v3.

@relu
Copy link
Member Author

relu commented Feb 18, 2021

I don't think that's necessarily the case, as I said earlier I tried with all sorts of other resources that have immutable fields and in all situations the behavior was the same. There's something weird going on that has kubectl wait endlessly for the patch to apply. I will try with the master version of kubebuilder just to confirm this, it might be just that indeed this new version will work just fine.

@relu relu force-pushed the force-apply branch 4 times, most recently from 924a88c to bf20494 Compare February 18, 2021 16:49
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @relu 🏅

@stefanprodan
Copy link
Member

@relu you need to run make test and push all changes.

Allow passing --force to kubectl apply. Useful when dealing with
immutable field changes in resources.

Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@stefanprodan stefanprodan added area/kustomize Kustomize related issues and pull requests enhancement New feature or request labels Feb 23, 2021
@stefanprodan stefanprodan merged commit 2f28126 into main Feb 23, 2021
@stefanprodan stefanprodan deleted the force-apply branch February 23, 2021 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kustomize Kustomize related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option for using kubectl replace --force when fail to apply
3 participants