Skip to content

Conversation

@kkourt
Copy link
Contributor

@kkourt kkourt commented Feb 26, 2025

In certain situations (e.g., development) it's useful to have a tetra client constantly running as you are restarting the tetragon agent. Add a --reconnect option in the tetra getevents command to support this.

tetra: add a --reconnect option to getevents command

@kkourt kkourt requested a review from a team as a code owner February 26, 2025 09:00
@kkourt kkourt requested a review from jrfastab February 26, 2025 09:00
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Could we make this the default, I think it would benefit everybody? Also it might be confusing with the gRPC retry already a flag.

@kkourt
Copy link
Contributor Author

kkourt commented Feb 26, 2025

Could we make this the default, I think it would benefit everybody? Also it might be confusing with the gRPC retry already a flag.

I wouldn't make this the default because it breaks compatibility.
What does the gRPC retry flag do?

@mtardy
Copy link
Member

mtardy commented Feb 26, 2025

Could we make this the default, I think it would benefit everybody? Also it might be confusing with the gRPC retry already a flag.

I wouldn't make this the default because it breaks compatibility. What does the gRPC retry flag do?

It retries the attempt to connect to the gRPC server ebae77f. It wouldn't retry after a successful attempt as you noticed, it's for the initial connection to the gRPC server.

@kkourt
Copy link
Contributor Author

kkourt commented Feb 26, 2025

How about I change the --retry option to --reconnect (and --retry-timeout to --reconnect-timeout). Does that help?

@mtardy
Copy link
Member

mtardy commented Feb 26, 2025

      --retries int             Connection retries with exponential backoff (default 1)

maybe --reconnect with a bool input that tries to restart the connection indefinitely? I find having --retry and --retries confusing

@kkourt kkourt added the release-note/misc This PR makes changes that have no direct user impact. label Feb 26, 2025
@mtardy
Copy link
Member

mtardy commented Feb 26, 2025

How about I change the --retry option to --reconnect (and --retry-timeout to --reconnect-timeout). Does that help?

EDIT: Yes agree same idea

I would keep --retries to explicitly show it's retrying when trying to establish a new connection (this can be done on every connection) and add new --reconnect that tries to restart the complet connection (with the retries) once it's been closed (what you want to add).

@kkourt kkourt force-pushed the pr/kkourt/tetra-getevents-retry branch from 2a045c7 to 281dce9 Compare February 26, 2025 10:31
@kkourt
Copy link
Contributor Author

kkourt commented Feb 26, 2025

How about I change the --retry option to --reconnect (and --retry-timeout to --reconnect-timeout). Does that help?

EDIT: Yes agree same idea

I would keep --retries to explicitly show it's retrying when trying to establish a new connection (this can be done on every connection) and add new --reconnect that tries to restart the complet connection (with the retries) once it's been closed (what you want to add).

👍🏼

Pushed a new version.

@kkourt kkourt changed the title tetra/getevents: add a --retry option tetra/getevents: add a --reconnect option Feb 26, 2025
@mtardy mtardy self-requested a review February 27, 2025 09:26
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks looks good, just a question on the name "timeout"

In certain situations (e.g., development) it's useful to have a tetra
client constantly running as you are restarting the tetragon agent. Add
a --reconnect option in the tetra getevents command to support this.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt kkourt force-pushed the pr/kkourt/tetra-getevents-retry branch from 281dce9 to a982517 Compare March 5, 2025 10:43
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

nice, looks useful

@kkourt kkourt merged commit 874b28c into main Mar 5, 2025
41 checks passed
@kkourt kkourt deleted the pr/kkourt/tetra-getevents-retry branch March 5, 2025 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants