-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
@@ -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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Thank you, @hiddeco!
I don't think 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/" |
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 really like the idea of adding support for hooks though, would be a great addition to the customize-controller. |
081090e
to
274d108
Compare
I don't understand this spec error:
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. |
@relu you need to configure Kind like so:
|
@stefanprodan kind is not used for these tests right now, it defaults to envtest's mock cluster since 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. |
@relu I get now what you're saying, we need to figure out how to tell envtest to use a newer Kubernetes version. |
You can configure custom binaries to be used by setting |
I think I found the issue, https://github.com/fluxcd/pkg/blob/main/actions/kubebuilder/entrypoint.sh#L5 this pulls old binaries. |
Indeed, see: https://github.com/fluxcd/kustomize-controller/pull/271/checks?check_run_id=1903336363 (Debug failure section).
|
3feb200
to
d3cdbe6
Compare
Relu I think you should use a Pod and change the label selector. We can't upgrade kubebuilder since it's alpha. |
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 |
@relu check out the pod tests that @phillebaba wrote here https://github.com/fluxcd/kustomize-controller/pull/255/files |
I tried different resources |
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. |
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. |
924a88c
to
bf20494
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @relu 🏅
@relu you need to run |
Allow passing --force to kubectl apply. Useful when dealing with immutable field changes in resources. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>
2617678
to
729dc97
Compare
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
58a2fac
to
65cfce2
Compare
Allow passing
--force
to kubectl apply. Useful when dealing withimmutable 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