Skip to content
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

feat: feature flag to not clone secondary resource on access #2364

Merged
merged 6 commits into from
May 2, 2024

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Apr 24, 2024

No description provided.

Signed-off-by: Attila Mészáros <csviri@gmail.com>
@csviri csviri self-assigned this Apr 24, 2024
@csviri csviri changed the title feat: feature flag to clone secondary resource on access feat: feature flag to not clone secondary resource on access Apr 24, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
* longer reflect the state on the server in that case.
*/
default boolean cloneSecondaryResourcesWhenGettingFromCache() {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be false by default since we're using SSA by default now, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was, thinking about that, and there are pros/cons to that. It quite risky when somebody is migrating to the new version. On the other hand yes, that would be more optimal to have by default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new version will have significant changes… actually, I'm thinking that the change to SSA will require significant rewrite of operators if you need to only send what changes. Right now, most operators send the whole changed resource, not simply what has changed.
For that matter, that's actually kind of what we're pushing towards with the desired state: people compute the whole resource and that's what gets used. It might not be a big deal because these resources' fields are probably managed by the JOSDK reconciler already but I'm wondering how prevalent that actually is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, most operators send the whole changed resource, not simply what has changed.

It's not only what changed, but the whole desired state with SSA.

Yes this might need some changes when there is no SSA used already, I mean the changes are made on an existing object. But this should be the way to do it. Seconday resource is read for comparison (matching) and when it is needed as an input for other resources.

I don't think it is such a big deal to migrate, but I can imagine some cases that might not be trivial.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider, though, is that people might not upgrade if they perceive the upgrade requires too many changes without bringing much benefit (i.e. if someone's operator already works correctly, they probably won't upgrade, especially if they need to change things a lot to do so).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, however, that is why we add feature flags.

csviri and others added 4 commits April 25, 2024 16:57
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
@csviri csviri merged commit 6897669 into next May 2, 2024
20 checks passed
@csviri csviri deleted the cloning-feature-flags branch May 2, 2024 06:02
csviri added a commit that referenced this pull request May 17, 2024

Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request May 17, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request May 21, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Jun 17, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm pushed a commit that referenced this pull request Jul 8, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm pushed a commit that referenced this pull request Jul 9, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm pushed a commit that referenced this pull request Jul 12, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Aug 8, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Aug 15, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm pushed a commit that referenced this pull request Aug 29, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Sep 20, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm pushed a commit that referenced this pull request Oct 10, 2024
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloning of secondary resource when getting it from InformerEventSource cache
2 participants