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

Fix noaccess retries #12

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pophilpo
Copy link
Contributor

@pophilpo pophilpo commented Oct 27, 2023

This PR will implement breaking out of the thread loop if the exit code of the port-forward call is 1 (#11). Before breaking out it will send a ChildEvent::Exit with a new RestartPolicy::NEVER

src/kubectl.rs Outdated Show resolved Hide resolved
@sunsided sunsided added the enhancement New feature or request label Dec 6, 2023
@pophilpo
Copy link
Contributor Author

@sunsided At the moment this change will stop every process that exited with status code 1. I'm working on recoverable errors now but I think it should be another pull request

@@ -251,13 +251,19 @@ fn start_output_loop_thread(out_rx: Receiver<ChildEvent>) -> JoinHandle<()> {
"{id}: Process exited with {} - will retry in {}",
status, delay
);
} else {
} else if delay == RetryDelay::NONE {
Copy link
Owner

Choose a reason for hiding this comment

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

It seems the newly added if statement here is either unnecessary or incorrect. Unnecessary because positive delay times are already captured above, incorrect because it leaves out the hypothetical negative delay times ("less that NONE"). What was the reason for adding it?

@sunsided
Copy link
Owner

At the moment this change will stop every process that exited with status code 1.

@pophilpo The restarting is integral to the functionality. If a pod is temporarily unreachable (e.g. due to a rolling update or scale-down event), we need the connection to be retried.

@sunsided
Copy link
Owner

sunsided commented Jan 4, 2024

@pophilpo Just added crashie to help testing that. The main branch here has a kind folder now that contains a kind cluster definition with a crashie deployment that will be restarted randomly. It has an HTTP port, so testing becomes a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants