-
Notifications
You must be signed in to change notification settings - Fork 214
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
generalization of the update matcher #2014
Conversation
I should mention that both the current logic and this logic have problems with matching stuff in the desired state that ends up being pruned. This pr does make that problem worse - if the desired state for example includes the status subresource that will be seen as a difference is it doesn't match exactly with the actual. But I don't think users should generally be doing this. |
69d0048
to
97da6cf
Compare
97da6cf
to
243b320
Compare
@metacosm @csviri there are two parts here
|
0e5b9cf
to
2fbd8a0
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.
Looks, good just some comments, and missing unit tests :)
...vaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java
Outdated
Show resolved
Hide resolved
...k/operator/processing/dependent/kubernetes/updatermatcher/GenericResourceUpdaterMatcher.java
Show resolved
Hide resolved
// https://github.com/fabric8io/kubernetes-client/issues/3816 | ||
var specDiffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, SPEC); | ||
boolean specMatch = match(equality, wholeDiffJsonPatch, ignoreList, SPEC); | ||
if (!specMatch) { |
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.
Please add unit tests at least for some of the special cases we were handling before?
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.
Same applies here GenericKubernetesResourceMatcherTest covers all of the modifications.
595f8d2
to
7bd9674
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
@@ -24,6 +23,9 @@ public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends H | |||
private static final String OP = "op"; | |||
public static final String METADATA_LABELS = "/metadata/labels"; | |||
public static final String METADATA_ANNOTATIONS = "/metadata/annotations"; | |||
// without knowing the CRD we cannot ignore status as it may not be a subresource, so if it's | |||
// included we expect it to match | |||
private static Set<String> NOT_OTHER_FIELDS = Set.of(SPEC, "/metadata", "/apiVersion", "/kind"); |
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.
Why SPEC is here? we want to match SPEC, or? I'm reading it wrong
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.
also again would add here status, since that does not need to match I think even if it is not a subresource
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.
Why SPEC is here? we want to match SPEC, or? I'm reading it wrong
spec and metadata have their own handling, so that's not covered by the "other fields" matching.
also again would add here status, since that does not need to match I think even if it is not a subresource
You would only want to do that if you suspect that users are including non-matching status that is not meaningful on their dependent resources. My guess would be that they are not including status, and if they are it would be because it's coming from the actual state (and would match), or that it's not a subresource and needs to be considered in matching. But if you feel strongly about it, it can be excluded.
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.
spec and metadata have their own handling, so that's not covered by the "other fields" matching.
sorry totally missed 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.
Yet another tweak would be to change the logic to just walk through the diffs once and do prefix matching to determine if the change is allowable that may make things even clearer.
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.
You would only want to do that if you suspect that users are including non-matching status that is not meaningful on their dependent resources. My guess would be that they are not including status, and if they are it would be because it's coming from the actual state (and would match), or that it's not a subresource and needs to be considered in matching. But if you feel strongly about it, it can be excluded.
So these are resources which usually created, but not managed by the controller, so in normal case we apply the desired state using SSA, that is usually the spec. We don't care about the status at all at this phase, we might case in a postCondition, or when we are setting the status for the primary resource.
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.
We don't care about the status at all at this phase,
If you assume it's a subresource. If not, then you do.
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.
yep, true, but again that should not be a feature flag? we are comparing the desired with the actual, the actual would contain the status even if it is a subresource, but the desired won't, wouldn't this always fail to match in that case?
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.
we are comparing the desired with the actual, the actual would contain the status even if it is a subresource, but the desired won't, wouldn't this always fail to match in that case?
Ah, the issue you are raising is with the true parameter with https://github.com/operator-framework/java-operator-sdk/pull/2014/files#diff-3425845ceca0cbb05292b87cdceec9960b4342cefac82c18ab929a5ef22e5288R211 - if someone leaves the status unpopulated in the desired (which is expected) the exact match here will cause a problem. I must have been implicitly assuming that all are add ops would apply here, but that would obviously require a separate case. Let me make another revision and add a test.
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.
Further refactored to clarify the matching algorithm and added status as excluded.
54a65a6
to
1d53df0
Compare
One last thought is whether you want something for the loss of the protected method updateClonedActual - a release note? Bring back a similar method, or just let people override updateResource after calling the super method? |
I think overriding |
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
also adding a test that removes a field not present in the desired
1d53df0
to
0357ac3
Compare
var clonedActual = context.getControllerConfiguration().getConfigurationService() | ||
.getResourceCloner().clone(actual); | ||
KubernetesSerialization kubernetesSerialization = | ||
context.getClient().getKubernetesSerialization(); |
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'm wondering if this is opening potential issues where the cloning occurs in a different manner than here if, for some reason, the configured resource cloner has a different serialization set up… Should we remove the cloner altogether?
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.
If this was the only instance of the cloner being used, it could be removed or deprecated. There is also a clone method on KubernetesSerialization.
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 apart from one comment.
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (operator-framework#2001) * feat: leader election callbacks (operator-framework#2015) * discriminator improvements (operator-framework#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]> Signed-off-by: Steve Hawkins <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
* feat: create resource only if not exists (#2001) * feat: leader election callbacks (#2015) * discriminator improvements (#2013) * generalization of the update matcher * consolidating the diffs also adding a test that removes a field not present in the desired --------- Co-authored-by: Attila Mészáros <[email protected]> Signed-off-by: csviri <[email protected]> Signed-off-by: Attila Mészáros <[email protected]>
This generalizes the update and match logic for fields other than spec and metadata. This should make the usage of specific subclasses unnecessary.