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

Traceflow spec update is not handled properly #5255

Closed
shi0rik0 opened this issue Jul 15, 2023 · 5 comments
Closed

Traceflow spec update is not handled properly #5255

shi0rik0 opened this issue Jul 15, 2023 · 5 comments
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@shi0rik0
Copy link
Contributor

Currently, the traceflow update event handling only considers the cases where the controller and agent update the status, but it doesn't account for the scenario where the user actively updates the spec (e.g., modifying the destination pod in the traceflow spec).

I have two possible solutions:

  1. Modify the logic of the controller and agent to handle traceflow updates. When the user updates the spec, update the OVS flows accordingly to align the behavior of traceflow with the updated spec.
  2. Prohibit spec updates.

Personally, I prefer the second solution because it is simpler and can avoid some problems. For example, if we adopt the first solution, if the traceflow is already in progress, should we preserve the collected data when changing the spec?

@luolanzone
Copy link
Contributor

I think option 2 is more reasonable considering Traceflow is an ephemeral objects. I feel it's better to involve this change after Traceflow API promotion is done in #5108.
@antoninbas @tnqn @gran-vmv welcome input.

@antoninbas
Copy link
Contributor

I guess live-traffic traceflows are not always that "ephemeral". Someone may create a live-traffic traceflow using a yaml file, realize they made a typo in one of the filters, edit the yaml file and re-apply it. In this case, preventing spec updates would lead to a failure.

However, even option 2 is better than the current behavior of ignoring user updates IMO.

@shi0rik0
Copy link
Contributor Author

I think option 2 is better because it can avoid some corner cases when implementing firstN sampling live-traffic traceflow. If the user finds a typo, they can just delete the TF and create a new one. I plan to proceed with this change after these PRs are merged: #5230, #5291.

@luolanzone @antoninbas

@shi0rik0
Copy link
Contributor Author

The behaviors of Role objects is similar to Option 2 (https://kubernetes.io/docs/reference/access-authn-authz/rbac/#clusterrolebinding-example).

After you create a binding, you cannot change the Role or ClusterRole that it refers to. If you try to change a binding's roleRef, you get a validation error. If you do want to change the roleRef for a binding, you need to remove the binding object and create a replacement.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

3 participants