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

Move all environment variables to sidecar #2221

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

dvaldivia
Copy link
Collaborator

This moves all the environment variables to the sidecar so that changes to tenant environment or secret causes the sidecar to reload the configuration locally instead of requiring a rolling restart.

Signed-off-by: Daniel Valdivia <[email protected]>
@harshavardhana
Copy link
Member

harshavardhana commented Jul 17, 2024

@jiuker with this change the PR #2140 is not needed.

@pjuarezd
Copy link
Member

pjuarezd commented Jul 17, 2024

This is great to avoid as much as possible restart pods when configuration changes, and as this PR should be merged as is, already tested it and looks great.

However a undesired secondary effect I have raised the hand is still there if three conditions are met:

  • Operator controller is down (or failing), ergo no statefulsets are being modified by the controller
  • A Pool is added or removed from the Tenant resource
  • Any of the runing minio pods is restarted or error flapping (for whathever reason)

Then that pod do not return to run state because sidecar is writing in the env variables file in /tmp/minio/config.env a MINIO_ARGS variable that includes the latest change in Pools as soon as the Tenant resource has a change, regardless if the Controller is creating or not a statefulset, or removing it.

MinIO tenant pods should be able to keep runing regardless of the controller is healty running or not in the cluster, that was the idea behind removing the Operator TLS cert and the Operator webhook to get env variables, to make Tenant pods able to keep running standalone.

Steps to reproduce:

  1. Build this branch image (or any starting v5.0.0)
  2. Create a Tenant, wait for it to be in Initialized state
  3. Add a second pool, wait the tenant to be in Initialized state
  4. Scale down the Operator deployment to 0 pods (to simulate the controller is down)
  5. Remove the second pool, <- at this point everything should be fine and no problem
  6. Delete any pods in the 2 statefulsets
  • Notice the pod do not come back online
  • Notice the pod has a MINIO_ARGS env variable in the file /tmp/minio/config.env with only one pool, instead of 2 as any of the other pods in the cluster
  • Notice the error message in MinIO is related to mismatching configuration
INFO: Following servers have mismatching configuration [https://myminio-pool-0-0.myminio-hl.tenant-lite.svc.cluster.local:9000->https://myminio-pool-0-3.myminio-hl.tenant-lite.svc.cluster.local:9000 has incorrect configuration: Expected number of endpoints 8, seen 16 https://myminio-pool-0-0.myminio-hl.tenant-lite.svc.cluster.local:9000->https://myminio-pool-0-1.myminio-hl.tenant-lite.svc.cluster.local:9000 has incorrect configuration: Expected number of endpoints 8, seen 16 https://myminio-pool-0-0.myminio-hl.tenant-lite.svc.cluster.local:9000->https://myminio-pool-0-2.myminio-hl.tenant-lite.svc.cluster.local:9000 has incorrect configuration: Expected number of endpoints 8, seen 16]

Possible solution

Somehow conscider the state of the pools in the Tenant spec.status or add a new field the Tenant spec.status to comunicate sidecar when is OK to regenerate env variables, @jiuker 's PR is an aproximately similar solution it just misses the part where sidecar waits for the "OK" to regenerate env variables.

For your consideration @harshavardhana @dvaldivia

Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

Few minor ones. Overall changes look good to me.

pkg/configuration/tenant_configuration.go Show resolved Hide resolved
sidecar/pkg/sidecar/sidecar_handlers.go Show resolved Hide resolved
sidecar/pkg/sidecar/sidecar_handlers.go Show resolved Hide resolved
@jiuker
Copy link
Contributor

jiuker commented Jul 17, 2024

@jiuker with this change the PR #2140 is not needed.

I didn't see how this pr resolved that issue. @harshavardhana
@pjuarezd Right?

@harshavardhana
Copy link
Member

Somehow conscider the state of the pools in the Tenant spec.status or add a new field the Tenant spec.status to comunicate sidecar when is OK to regenerate env variables, @jiuker 's PR is an aproximately similar solution it just misses the part where sidecar waits for the "OK" to regenerate env variables.

@jiuker ^^

@dvaldivia dvaldivia merged commit fc934fd into minio:master Jul 17, 2024
21 checks passed
@dvaldivia dvaldivia deleted the sidecar-generated-config branch July 17, 2024 15:13
@djwfyi djwfyi mentioned this pull request Jul 18, 2024
13 tasks
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.

6 participants