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.

Sign up for GitHub

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

Jump to bottom

Introduce Sidecar Args #1437

Merged
merged 6 commits into from
Feb 16, 2023
Merged

Introduce Sidecar Args #1437

merged 6 commits into from
Feb 16, 2023

Conversation

dvaldivia
Copy link
Collaborator

@dvaldivia dvaldivia commented Feb 6, 2023 •
edited

Introduces an initContainer and a sidecar container for MinIO to retrieve the MINIO_ARGS for the tenant and to monitor the configurations secret and update it locally as soon as it changes.

Additional this sidecars will validate if root credentials are missing and if so, error out.

Screenshot 2023-02-07 at 2 14 43 PM

@dvaldivia dvaldivia self-assigned this Feb 6, 2023
@dvaldivia dvaldivia force-pushed the sidecar-args branch 2 times, most recently from 1fa7906 to 02f2b17 Compare February 6, 2023 23:03
harshavardhana
harshavardhana previously requested changes Feb 8, 2023
View reviewed changes
cmd/operator/app_commands.go Show resolved Hide resolved
pkg/apis/minio.min.io/v2/constants.go Outdated Show resolved Hide resolved
@dvaldivia
Sidecar MINIO_ARGS
7dcc7ff 
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@dvaldivia
Fix Config File location
cf45eff
@dvaldivia
Don't start prometheus for tests
d02127f 
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@dvaldivia
Fix Prometheus Test
3fe7711 
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@dvaldivia
fix shellcheck
5c4645b 
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@dvaldivia
variable helm rollout
b822e4e 
Signed-off-by: Daniel Valdivia <18384552+dvaldivia@users.noreply.github.com>
@cniackz
Copy link
Contributor

cniackz commented Feb 16, 2023

Hello @dvaldivia I am starting to review and test this PR, I want to take my time to review this truly, so please merge PR if needed otherwise I will place my feedback soon!

@cniackz
Copy link
Contributor

cniackz commented Feb 16, 2023 

Cesars-MacBook-Pro:operator cniackz$ oc get serviceaccount -n tenant-lite
NAME              SECRETS   AGE
builder           1         3h4m
default           1         3h4m
deployer          1         3h4m
minio-operator    1         161m
storage-lite-sa   1         18m <------------ I can see the new service account being created

And because there is new Service Account, then we need extra line for the priviledged access in OpenShift:

oc adm policy add-scc-to-user privileged -n tenant-lite -z storage-lite-sa

Or have proper constraints for the security context, anyway, the important part is that Tenant can be deployed and initialized in OpenShift in this change:

Screenshot 2023-02-16 at 1 11 39 PM

Also as a side note, used operator-ca-tls in the meantime, since we still face TLS issue here, but will be solved in #1450

My next step is to keep exploring and test in k8s as well.

@cniackz
Copy link
Contributor

cniackz commented Feb 16, 2023

Also I can see the 3 containers here and the volumes being used:

Screenshot 2023-02-16 at 1 15 10 PM

Let's see what happen if MinIO Server is terminated...

@cniackz
Copy link
Contributor

cniackz commented Feb 16, 2023

It is very quickly restored, quicker than before:

Screenshot 2023-02-16 at 1 16 19 PM

@cniackz
Copy link
Contributor

cniackz commented Feb 16, 2023

Great, I can reach MinIO Console Service as well via Route:

Screenshot 2023-02-16 at 1 18 13 PM

In general, this change is not breaking anything so far but just making things better and faster!!!!

cniackz
cniackz approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@cniackz cniackz left a comment

Choose a reason for hiding this comment

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

Great Job @dvaldivia thanks for this big Change!!! 👍

@dvaldivia dvaldivia dismissed harshavardhana’s stale review February 16, 2023 22:31

license added to files

@dvaldivia dvaldivia merged commit 299b00d into minio:master Feb 16, 2023
@dvaldivia dvaldivia deleted the sidecar-args branch February 16, 2023 22:32
@pjuarezd pjuarezd mentioned this pull request Mar 19, 2023
Merged
@djwfyi djwfyi mentioned this pull request Mar 30, 2023
Closed
8 tasks
@xoxys xoxys mentioned this pull request Apr 16, 2023
Closed
@fgleixner
Copy link

Hi,
i just tried to understand, why the minio operator switch --cluster-domain does not work any more. All defaults to cluster.local. All tenants are broken. Is it possible, that introducing sidecars and deleting the function GetEnvHandler in http_handlers.go introduced this bug? #1535 and #1544

@cniackz cniackz mentioned this pull request Nov 2, 2023
Merged
@twelho twelho mentioned this pull request Mar 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveenrajmani Praveenrajmani Praveenrajmani left review comments

@shtripat shtripat shtripat left review comments

@pjuarezd pjuarezd pjuarezd approved these changes

@cniackz cniackz cniackz approved these changes

@bexsoft bexsoft bexsoft approved these changes

@allanrogerr allanrogerr Awaiting requested review from allanrogerr

@dilverse dilverse Awaiting requested review from dilverse

@reivaj05 reivaj05 Awaiting requested review from reivaj05

@kanagarajkm kanagarajkm Awaiting requested review from kanagarajkm

@harshavardhana harshavardhana Awaiting requested review from harshavardhana

Assignees

@dvaldivia dvaldivia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dvaldivia @cniackz @fgleixner @harshavardhana @pjuarezd @Praveenrajmani @shtripat @bexsoft