Skip to content

fix(soot): triggering cleanup for failed soot manager #761

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

Merged
merged 2 commits into from
Mar 31, 2025

Conversation

prometherion
Copy link
Member

@ansalamdaniel this is a draft proposal for #756, happy to review your PR too

Copy link

netlify bot commented Mar 27, 2025

Deploy Preview for kamaji-documentation canceled.

Name Link
🔨 Latest commit e36fa35
🔍 Latest deploy log https://app.netlify.com/sites/kamaji-documentation/deploys/67e6838f7cc88e00089f3fac

Signed-off-by: Dario Tranchitella <dario@tranchitella.eu>
@ansalamdaniel
Copy link

ansalamdaniel commented Mar 27, 2025

Hey @prometherion , this is better version of the PR I'm working on. One scenario I want to be sure of is that, let's say if the goroutine is unable to update the annotation of the tcp object, don't we end up in the same scenario where channel is blocking to send triggers.
Is it ok to have the triggers in select statement like this ? Let me know your thoughts.

	for _, trigger := range v.triggers {
		select {
		case trigger <- event.GenericEvent{Object: tcp}:
			continue
		default:
			log.FromContext(ctx).Info("channel maybe closed.. dropping event trigger")
		}
	}

@prometherion
Copy link
Member Author

prometherion commented Mar 27, 2025

That's definitely a good thing to add, it would avoid potential logical deadlocks: at least those would be logged and inform the human operator. 👍🏻

@ansalamdaniel
Copy link

@prometherion
Awesome. Could you please add it in this PR ??

Signed-off-by: Dario Tranchitella <dario@tranchitella.eu>
@prometherion
Copy link
Member Author

@ansalamdaniel just done!

@prometherion prometherion marked this pull request as ready for review March 31, 2025 09:56
@prometherion prometherion merged commit dd099e7 into clastix:master Mar 31, 2025
10 checks passed
@prometherion prometherion deleted the issues/756 branch March 31, 2025 16:54
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.

2 participants