Introduce immutable transforms and consider KnativeServing CR config to be canonical #165
Conversation
There was a problem hiding this comment.
@jcrossley3: 2 warnings.
In response to this:
Fixes #138 and relates to #163
Proposed Changes
- Add an e2e test that reveals the problem described in Deleting a KnativeServing CR with changes, and then reapplying blank KS CR should use default serving yaml #138
- Pull in a pre-release of manifestival to transform manifests immutably
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
/assign @garron |
|
Thanks @jcrossley3. I also went through this exercise yesterday and put together a similar solution. Given that your review is up, let's run with it. I'm still going to share my branch so that you can take a look at some slightly different options. Namely:
I will take a more thorough look through your review to better understand some of the additional changes you made. |
| // 'metadata.resourceVersion', 'spec.clusterIP', and any existing | ||
| // entries in a ConfigMap's 'data' field. So we only overwrite fields | ||
| // set in our src resource. | ||
| // We need to preserve some target keys, specifically |
There was a problem hiding this comment.
I have a few thoughts/questions here. Maybe it makes sense to split this off into another review so that we can land the state management pieces.
-
This code is used by anyone who uses this library to apply a manifest. I don't know if there are other consumers of manifestival right now, but it feels a bit unfortunate that implementation decisions about how the operator should work result in functional changes to how this library interprets requests to apply a manifest. Are we sure others are OK with these?
-
As someone new to manifestival, I was surprised to see this logic and had assumed Apply() behaved similarly to "kubectl apply". Would it make sense to move this to the transform stage? That would make it possible for this library to provide a an opt-in implementation, but would also allow for altering that behavior should it not be right for a particular project. If you think what is here is universally desired behavior, I think it would be worth documenting what the intended behavior is.
-
While the comments mention a few specific examples where this is necessary, this implementation is generic and applies to all config. Regardless of whether we move it to a transform or not, it's hard for me to conclude from looking at this that the operator's config is going to act as the source of truth once run through this function. Would it be possible/practical to move to a hard-coded list of keys that we want to maintain? That would also serve as a means to ensure we're not breaking important key preservation over time. I'm thinking something like (rough first-pass idea):
my_manifest.Transform(
KindFilter("ConfigMap", MaintainExistingValue(my_manifest, ["spec.key1", "spec.key2"]))
).ApplyAll()There was a problem hiding this comment.
This code has evolved significantly from the PoC it originally was, as has my understanding of operators in general and kubernetes itself. 😄
You're right to assume Apply() behaves similarly to kubectl apply. It should. I would love for it to. I'm waiting for some expert to come along and tell me, "That's dumb, do it like this instead!" Are you that expert? Please!!! 😄
Some of the code in here came from the idea that we should allow external edits to the resources installed by manifestival. I never wanted the operator to overwrite a human's changes. I've come around in my thinking on this. And it's very possible that having the operator be the "source of truth" will simplify this code. That'd be swell.
Right now, to my knowledge, only the serving-operator is using the client-go branch of manifestival. I know of a few other operators based on operator-sdk/controller-runtime that use the master branch without a problem, so I'm sensitive to releasing breaking changes, but not opposed to it as long as we're clear about what changes and why.
Personally, I hate the UpdateChanged function -- it's gone through quite a few iterations and has some fairly comprehensive tests around it because of its brittleness. I'm always open to improving it, or better yet, eliminating it!
There was a problem hiding this comment.
Unfortunately, I am far from your expert on kubectl apply. From looking through more of the apply docs, I do see that it has it's own merging, so this may be closer to the kubectl apply behavior than I had realized.
Regardless, it does seem like the merging is going to pose a challenge in making the operator's spec the source of truth. We can tackle that elsewhere though. I don't think you should block your review on this feedback.
After creating the KnativeServing instance, we then update it twice, once with an entry for the config-logging ConfigMap and then again without the key, verifying that the operator correctly syncs the ConfigMap each time.
This relies on the Transform function now being immutable, returning a new Manifest instead of mutating its source resources. This is related to knative#163 as well.
Good idea, @garron -- done. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: garron, jcrossley3 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 |
This also makes the WaitForConfigMap helper more helpful
|
/lgtm |
Fixes #138 and relates to #163
Proposed Changes