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
One-pager design for ignore changes on updates / granular management policies #3822
Conversation
f80f9e2
to
327f656
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.
Thanks for tackling this design @lsviben!
59c27f5
to
83e1188
Compare
I'm currently impacted by the issues we are trying to solve, shared here TL,DR; 3 different fields on the NodePool manifest of provider-gcp are generating issues with GKE's autoUpgrade and autoScaling functionalities. These 3 fields have something in common. None of them are on my initial manifest and neither is a mandatory field. So, I'm thinking that another approach to solving this is to avoid updating the original manifest This way, CrossPlane will ensure that only the values set by the user are sustained over time while allowing all other values to flow freely. Additionally, providers and other plugins will be able to get the full info on the resource from the |
Thanks for the input! Basically your idea would be to turn off Ill add your use case and alternative solution suggestion to the proposal |
Your interpretation sounds about right, but I'm a bit new to CrossPlane and I'm not fully familiar with what I'm getting some inspiration on ArgoCD, that keeps 2 manifests versions: the one that the user uploads (desired manifest), and the one that is actually running on the cluster (current manifest). Then, on every cycle, we would make sure that the current manifests includes all the values in the desired manifest, and reconcile only if there is a deviation between these two. It's expected that the current manifest will have more values that the desired manifest. And this is OK. Regarding how this overlaps with Observe-Only goals, I believe this proposal will accomplishes both of them. |
This is along the lines of what I was thinking after our discussion a few weeks back. We can't turn off late-init by default because that would be a breaking API change. We could add a knob to disable it though - for example the new At that point we'd have:
If you wanted to allow the external system (e.g. a GKE node pool autoscaler) to control a particular field you would:
One downside of this approach is that you have to opt-out of late init entirely (not just for the fields you care about). I'm not sure how much of an issue that would actually be in practice. |
The default management policy is Stating this, I like the idea of using the managementPolicy to control the late-init behavior since disabling late-init would mean XP managing the resource but only the provided fields. We can find a suitable policy name considering this, just to throw out some ideas: |
Throwing out another potential policy name: |
I like this approach of using the And in the start we don't need to support Just not sure if this should be optional:
IMO the fields that are in For the naming (hardest part as always) I narrowed down the suggestions to: I'm personally leaning towards |
for our use-case, we're creating an azuread group (https://marketplace.upbound.io/providers/upbound/provider-azuread/v0.5.0/resources/groups.azuread.upbound.io/Group/v1beta1), and we want to set |
35a9c2d
to
d944b43
Compare
Thanks for the input, I included it in the use cases :) |
Updated the proposal with the solution we discussed above (#3822 (comment)), as it seems like we are converging to a common view. Now the suggested solution is to add a new Management Policy(name still up for discussion) that would act in all ways as
In addition, as there are use cases which need a way to initialize resources with some parameters, but not use them later, the Although, while writing this solution update, it seems to me like these can be divided into 2 features. Not sure if we should split these features? P.S Its also worth to note that the previous suggested solution with |
Wondering why we are forcing the Following up on the GCP nodepool example having issues with autoUpgrade and autoScaler, I would want to avoid managing certain fields (eg. nodeCount, version) and still eliminate the Nodepool on delete. While |
Agree that by default we should But anyway, a valid point and I think we should keep this question open. |
d944b43
to
620adfd
Compare
Yes, this was something that I realized independently and came here to comment on.
Using the new Considering all of these, I am wondering whether we should keep the |
I agree with this - keeping DeletionPolicy separate is a cleaner interface that is easier to understand. |
From my perspective If we want to observe only, our manifest will include only the resource name (eg. as we do now when importing existing resources from a cloud provider) If we add any other value to that manifest, then it behaves as partial control, ensuring informed values are always in sync. It could be confusing to have values in an ObserveOnly manifest that are not kept in sync, so i assume that name only is the way to go for this case. |
The main problem here is what to do regarding the creation of resources. For example, what should the XP controller do after noticing there is no such bucket with the below manifest? Should it create it or error out assuming the user only wants to observe it? apiVersion: s3.aws.upbound.io/v1beta1
kind: Bucket
metadata:
annotations:
crossplane.io/external-name: example-bucket
name: example-bucket
spec:
managementPolicy: PartialControl
forProvider:
region: us-west-1 We already had some similar discussions in the OO design PR, please check: #3531 (comment) |
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.
Some thoughts on the deletionPolicy
Ok I see that most people are aligned with separating the ManagementPolicy from the DeletionPolicy. I on board with that as well, makes it cleaner and more explicit IMO. We wont need the |
Correct.
Yeah, this is something I am not happy with. However, we can document something like "
I don't think setting a conditional default is technically possible, at least with the way we are currently using, i.e. API level defaulting. |
Yeah this is tricky. I would also prefer to avoid cases where external ObserveOnly resources would accidentally be deleted. We could always also go back to just ignoring the I would still like to leave an option for users to configure a resource they want to just Observe and explicitly set if they want to delete it. Maybe it would be easier to digest the fact that an ObserveOnly reasource can be deleted if the name of the
Some thoughts behind this was to decouple the late initialization from the management policies, and to put the deletion decision back under management policies. If we "map" the management policies to the steps of the external client we could have the policies be easily understandable. BTW we were struggling to find a good name for a field that for now would control if late initialization is on or off and that can possibly be extended later with similar options. Maybe the simplest solution would be to just have a |
This seems like a good compromise to me. I worry that if we overthink this too much we'll end up with something that is potentially safer, but harder to understand. Perhaps it would be sufficient to add a strong warning to the |
I think my main concern is with this design is that we are changing what the Now, we are moving to some other direction:
This has two caveats:
This is why I am proposing to leave managementPolicy as is, but control disabling late-init behavior with another configuration. |
Thinking out loud, I think we want to affect the following things:
If I try to frame these as "what is Crossplane allowed to do" (not what can't it do), I think it's something like:
One option would be to frame this as a set of enums, similar to the If we were to frame this as a set of enum values representing what Crossplane should do for a particular managed resource, I think the default behavior would look like this: spec:
managementPolicy: [Observe, Create, Initialize, Update, Delete] Maybe for this case, like RBAC, you could support a shorthand: spec:
managementPolicy: ["*"] An observe-only resource would look like this: spec:
managementPolicy: [Observe] A resource that you didn't want to late-initialize would look like this: spec:
managementPolicy: [Observe, Create, Update, Delete] And a resource that you didn't want to delete (the equivalent to spec:
managementPolicy: [Observe, Create, Initialize, Update] Some further thoughts:
|
For me this feels like it could solve a lot of confusion about what exactly the different management policies do. I just wonder if we should just have the options So IMO we would still need a separate field that controls the actions on the spec:
managementPolicy: "*"
localManagement: "*" Disable late init: spec:
managementPolicy: "*"
localManagement: [Initialize] ObserveOnly: spec:
managementPolicy: [Observe]
localManagement: [] On the other hand, for sure this is more complex then just setting for example a |
We could avoid this by calling it spec:
managementPolicy: [Observe, Create, LateInitialize, Update, Delete]
I don't think I agree. I would argue that Worth noting that there are even more slightly oddball combinations this would open up, like:
I would interpret this as:
In practice we'd still be observing the ER in order to know whether we should create or update it, we just wouldn't be recording our observations. I also don't really know why anyone would use this particular policy, but it seems like we if we took this "array of enums" path we should support it for consistency. |
This might actually be useful for migration scenarios where the resource already exists and Crossplane needs to take ownership of it. I think using I'd prefer to have the ability to turn on/off specific external interface functionality, and late initialization, at my discretion. |
spec:
managementPolicy: [Observe, Create, LateInitialize, Update, Delete] Let me try write documentation for this, as I imagine it would appear in the API docs:
|
For me this seems understandable and flexible. If there are no other concerns, I will update the proposal to reflect the proposed changes to the management policy. |
I like the flexibility and explicitness of this API. My only concern is its verbosity, especially for some common use cases. For example:
becomes
But I don't think it is a significant problem and fine with moving forward with it. |
Updated the proposal, please take another look. |
I am not sure if this document should still be a proposal for Maybe more fitting would be something like : "Crossplane management policy" or "Crossplane managed resource management policy" - but that has too many manage in it. WDYT? I would change the background and goals a bit to fit more with the proposed solution. |
I'm admittedly a bit late to this party, but I'm wondering if it might be useful to include a policy option along the lines of I think this could be done with the current spec by:
While this is functional, it requires quite a few touches to do so. |
I believe this will also prevent applying updates to restore the existing state if it gets out of sync, in addition to blocking changes to I think controlled apply/approval is a much larger discussion that should be dealt with in a separate issue/design doc. |
Actually, at least in some cases, that might well be desired. Perhaps some out-of-band change was made in a break-glass kind of scenario, and we don't want that to automatically be reverted.
That's fair, just wanted to bring it up as a thing to consider. I can open a separate issue for it after I read through the relevant contribution guidelines etc. |
This is one of the scenarios behind the "pause reconciliation" feature that uses annotation I agree with @negz that |
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.
Looking great, just left a couple of comments that are nonblocking!
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.
Looking great, just left a couple of comments that are nonblocking!
6043e8c
to
95ceffc
Compare
Agreed on this @lsviben - do you want to go ahead and update the PR title to capture this broader scope too? |
Done! Kept the ignore changes part for historic and intention reasons and added granular management policies |
95ceffc
to
d33c0df
Compare
Signed-off-by: lsviben <sviben.lovro@gmail.com>
d33c0df
to
b8b3c85
Compare
Description of your changes
Extended the initial proposal with an one-pager design doc.
Iterating over the solution we came to a more generalized approach that introduces granular management policies through which features like ObserveOnly and ignore changes can be realized.
Fixes #3516
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
N/A