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

add inv_sig_helper pod #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

alindt
Copy link
Contributor

@alindt alindt commented Jan 21, 2025

Added sidecar container with inv_sig_helper (fixes #5)

@JuniorJPDJ JuniorJPDJ self-assigned this Jan 22, 2025
@JuniorJPDJ
Copy link
Collaborator

I'm not sure if sighelper shouldn't be separate pod. Let me think a bit ;)

invidious/Chart.yaml Outdated Show resolved Hide resolved
@unixfox
Copy link
Member

unixfox commented Jan 22, 2025

I'm not sure if sighelper shouldn't be separate pod. Let me think a bit ;)

Both are correct solution. If in the same container, you can benefit from the high speed socket file provided by inv_sig_helper.

And the other solution in a separate pod, you can have multiple invidious pods using one single inv_sig_helper. Do note that in this case it's not a good idea to have multiple inv_sig_helper and will lead to decryption issues.

I'm more leaning towards the second option because if one is using kubernetes then it's to scale "invidious" and not to keep it at just one replica.

This reverts commit d3cef4d.
@JuniorJPDJ
Copy link
Collaborator

I'm more leaning towards the second option

So you'd prefer to have multiple pods, right?

@JuniorJPDJ
Copy link
Collaborator

JuniorJPDJ commented Jan 24, 2025

So I think inv_sig_helper should be in separate StatefulSet then.
Also you should just bump minor version instead of patch - not revert the bump.

@unixfox
Copy link
Member

unixfox commented Jan 24, 2025

Why a statefulset? It doesn't story any data.

@JuniorJPDJ
Copy link
Collaborator

It does hold the state.

Do note that in this case it's not a good idea to have multiple inv_sig_helper and will lead to decryption issues.

@alindt
Copy link
Contributor Author

alindt commented Jan 25, 2025

I'm not updating the chart version anymore — that's your thing as maintainer(s) of the chart to decide whether/how/when to update it.

@alindt alindt changed the title add inv_sig_helper container add inv_sig_helper pod Jan 25, 2025
@alindt alindt requested a review from unixfox January 25, 2025 23:40
@unixfox
Copy link
Member

unixfox commented Jan 26, 2025

We appreciate people that also modify the Chart.yaml though. It's a community helm chart everyone contribute it, not only the maintainers :).

If you can set it to 2.1.0 that would be great because this is a minor change. And change the app version to the invidious changelog.

Thakks

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.

Add support for deploying inv-sig-helper
3 participants