search

openshift / kube-compare

A Kubectl plugin to allow to compare a known valid reference configuration and a set of specific cluster configuration CRs.
Apache License 2.0
8 stars 8 forks source link

Merge reference info live before diff to get more relevent diffs #37

Closed nocturnalastro closed 4 days ago

nocturnalastro commented 2 weeks ago

Background

Jira: https://issues.redhat.com/browse/CNF-12172

~dependent on https://github.com/openshift/kube-compare/pull/38~

openshift-ci[bot] commented 2 weeks ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

nocturnalastro commented 1 week ago

/cc @pixelsoccupied @AlinaSecret @crwr45

nocturnalastro commented 1 week ago

/test all

pixelsoccupied commented 1 week ago

Could you link the JIRA/issues and update the PR description to explain what was done just so we understand the work in the future?

Lgtm for me otherwise....I have created the new set DU profile rules based on this build https://github.com/openshift-kni/cnf-features-deploy/pull/1940

pixelsoccupied commented 1 week ago

Might be good time to include PR template https://raw.githubusercontent.com/openshift-kni/lifecycle-agent/main/.github/pull_request_template.md

pixelsoccupied commented 1 week ago

Reached out to @imiller0 to talk about this change in behaviour and I think we can move forward as it drastically simplifies building the rules. But two things

AlinaSecret commented 1 week ago

I think its better to include this behavior in the metadata.yaml, instead of as a flag, because it an attribute of the reference the structure of the reference depends on this value. and because its in the metadata.yaml we can also enable the feature only for a part/component/CR in the reference, what do you think? is it better to have a global setting or the option to configure it by part?

AlinaSecret commented 1 week ago

Also its important to add in this pr in the docs an explanation about the merging behavior
@nocturnalastro

AlinaSecret commented 1 week ago

@pixelsoccupied not sure if i just didnt notice, but can you add the example you mentioned ( example of that in linked DU profile PR with PerformanceProfile)

imiller0 commented 1 week ago

I think we need a bit more discussion on the merging behavior. There are some other cases where always merging will hide relevant differences that should be reviewed by the user for correctness. In the current implementation the reference designer can list specific fields in the metadata.yaml for suppressing differences (which is what the merge is also doing). I like the idea that the designer is explicitly saying "these fields are OK to differ" rather than defaulting to "ignore all differences unless explicitly written into the reference". Would augmenting the metadata.yaml support for suppressing differences help with this? Perhaps the ability to flag a CR as "ok to merge this whole CR", while others would continue to show more differences (because they aren't being merged). The intent is that this tool is used to highlight risk due to differences between the user's config and the reference. The suppressing of differences is to help them focus on the important pieces by eliminating things we know are ok. If we aren't sure, or are going to make an error on what to show, we should error on the side of showing more and letting them review.

nocturnalastro commented 1 week ago

@imiller0

I've been working on some POCs for how to control this behaviour. Its very much doable, on a per template basis. I like the idea of having some config in the template via comments which allow you control if it is merged or not. Here's an example of what I have in mind

The issue (as I see it) with listing explicit fields to omit is that it becomes very costly in enumerating them and will have substantial drift over time as the controllers change, making references a nightmare to maintain. Also fieldsToOmit would have to per-template for them to be useful for this.

We could switch the behaviour to off by default instead but you will end up getting false positives with labels, annotations making it annoying to build references. Which is what @pixelsoccupied was facing when he was to building the DU profile.

While being able to save diffs and passing them in as acceptable goes some what to mitigate this I think would be annoying.

In some ways it can also make templates more explicit if you really want a field never exist you can set it to null - showing you care about it not being there.

edit: For now I'll default it off and then we can see how much we need to turn it on, it would be easy to flip the default

imiller0 commented 1 week ago

@imiller0

I've been working on some POCs for how to control this behaviour. Its very much doable, on a per template basis. I like the idea of having some config in the template via comments which allow you control if it is merged or not. Here's an example of what I have in mind

The issue (as I see it) with listing explicit fields to omit is that it becomes very costly in enumerating them and will have substantial drift over time as the controllers change, making references a nightmare to maintain. Also fieldsToOmit would have to per-template for them to be useful for this. Agreed that it would need to be per template (reference CR) to be accurate. One thing I like about your example is that the knowledge of how the CR should be treated is kept with the CR rather than a split between the CR and the metadata.yaml. This will make it easier to maintain.

The question of enumerating fields is, to me, one of the key questions. The competing interests are

Adding support for both of these will provide tools to reference designers to handle suppression of diffs. The reference designers can make the decision on what is important to highlight and what can be suppressed.

Regarding the burden this adds to the reference designer, I think this is where the high-value work exists in this tool flow. The reference designer can do the analysis/calculus once and all the users benefit from the work. Typically the reference designer is well aware of changes that will come into the CRs release-over-release because they have to account for this in the overall design anyway. Capturing it in the reference is one of the ways way they can communicate that change.

We could switch the behaviour to off by default instead but you will end up getting false positives with labels, annotations making it annoying to build references. Which is what @pixelsoccupied was facing when he was to building the DU profile.

While being able to save diffs and passing them in as acceptable goes some what to mitigate this I think would be annoying.

In some ways it can also make templates more explicit if you really want a field never exist you can set it to null - showing you care about it not being there.

edit: For now I'll default it off and then we can see how much we need to turn it on, it would be easy to flip the default

nocturnalastro commented 6 days ago

@imiller0 That makes sense, I think we have provided at least some of those tools now we can see if more are needed over time.

I think in the future it might be worth extracting extra the comments from the yaml tree to allow more refined usage but ATM I think a custom function while a little cumbersome is good enough to see if its needed.

nocturnalastro commented 6 days ago

/unhold

AlinaSecret commented 5 days ago

@imiller0 @nocturnalastro @pixelsoccupied

as Ian mentioned :

Support field-suppression annotation in the CRs (ie a comment similar to your example) where the designer can omit fields (or a tree if the field is a complex data type) based on jsonpath. As you noted this needs to be per template (CR) and, to me, is easier to maintain along with the reference CR.

also discussed here in a different worse implementation (https://github.com/openshift/kube-compare/pull/37#discussion_r1646334450)

i think we should address this in a different issue, for now , for templates that may need this feature the merge should be disabled and we can use go template solutions, i will add some clear examples, that can be added to the docs.

and we can open in issue for this change, i think after this PR we will have a good base to add it in

pixelsoccupied commented 4 days ago

Amazing stuff! Looking forward to updating the reference with this!

/lgtm /assign @AlinaSecret

AlinaSecret commented 4 days ago

/lgtm /approve Thanks @nocturnalastro know it took more time then the usual and may been exhausting , so i think its a big win to get this one merged :1st_place_medal: !!!! Appreciate your effort very much :)

openshift-ci[bot] commented 4 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlinaSecret

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: - ~~[OWNERS](https://github.com/openshift/kube-compare/blob/main/OWNERS)~~ [AlinaSecret] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 4 days ago

@nocturnalastro: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).