-
Notifications
You must be signed in to change notification settings - Fork 10
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 composite readiness based on expected resource count #55
base: main
Are you sure you want to change the base?
Conversation
…d resource count Signed-off-by: Bob Haddleton <[email protected]>
Signed-off-by: Bob Haddleton <[email protected]>
59e0522
to
1482cd8
Compare
Random fly-by comment: I wonder how people are expected to maintain an accurate count of resources in a changing world. Seems to me that this just becomes another vector for bugs where someone adds conditional resources, forgets to update the count and the XR becomes ready too early. Or worse, removes resources, doesn't update the count, and the XR never becomes ready. It seems to me that a different way to approach this problem is for conditional modules to create the expected resources when it is time for them to do so. But if is not yet time to do it, they instead create a "sentinel, placeholder, noop resource" that never becomes ready preventing the XR from becoming ready. |
@gotwarlost I don't disagree but I'm already seeing the first scenario daily and I encounter the second in normal development, so I don't see that this makes things any worse. I'm not sure that the overhead of creating placeholder resources and then updating them later is worth the extra effort, compared to just telling the composite reconciler whether the resource is ready or not. |
I completely agree with this statement. I guess my disagreement is in the manner we convey this information. If it means that someone has to manually keep a counter up to date to do so, I feel like that is a losing proposition. I notice that there is already a |
@bobh66 I wonder if a list of named resources that are expected to exists might be more robust and easier to maintain. It would avoid any magic numbers in the code. It would also preserve the functionality if supplementary non-critical resources are added. |
@chlunde That would work too, though in some cases we use generated resource names so they would have to be passed through the context. I wonder if it makes sense to support both the count for simple cases where you just want to make sure that N things exist, and a list of resources for more complicated cases which may be more dynamic across deployments. |
Description of your changes
Added an optional Input to the function for ExpectedResourceCount to enable control of Composite Resource readiness
based on whether the expected number of resources have been created.
If the ExpectedResourceCount is not specified (the default) there is no change in functionality.
If the ExpectedResourceCount is specified and the number of desired composed resources that are Ready is greater than or equal to the expected count, the Composite will be marked as
Ready = True
If the ExpectedResourceCount is specified and is less than the number of desired composed resources that are Ready the Composite will be marked as
Ready = False
Fixes #54
I have:
Comments are welcome. I'm a better Python programmer than go programmer and it usually shows.