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

Remove index-based pool name verification #2164

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

pjuarezd
Copy link
Member

@pjuarezd pjuarezd commented Jun 13, 2024

This PR Is the continuation of bugfix: set pool status based on name rather than index based #2161
fixes bug:

Pools names duplicated and miscreated MINIO_ARGS

Generate Minio Host names based in the spec.status leads to duplicated pool names when a pools are decomissioned

Steps to reproduce:

  1. Clone from master and build operator container and sidecar
  2. Create a Tenant, wait for it to initialize
  3. Add a second pool, name it pool-1, make sure the pool is the second in the spec.pools list
  4. Add a third pool, make sure the new pool has the name pool-2.

Notice:

  • There is 3 Pools, added in the following order:
    pool-0
    pool-1
    pool-2
This is an example tenant pools (click to expand)
pools:
  - containerSecurityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
      seccompProfile:
        type: RuntimeDefault
    name: "pool-0"
    resources:
      requests:
        cpu: "1"
        memory: 2Gi
    runtimeClassName: ""
    securityContext:
      fsGroup: 1000
      fsGroupChangePolicy: OnRootMismatch
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
    servers: 4
    volumeClaimTemplate:
      metadata:
        creationTimestamp: null
        name: data
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: "6710886400"
        storageClassName: standard
      status: {}
    volumesPerServer: 4
  - containerSecurityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
      seccompProfile:
        type: RuntimeDefault
    name: "pool-1"
    resources:
      requests:
        cpu: "1"
        memory: 2Gi
    runtimeClassName: ""
    securityContext:
      fsGroup: 1000
      fsGroupChangePolicy: OnRootMismatch
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
    servers: 4
    volumeClaimTemplate:
      metadata:
        creationTimestamp: null
        name: data
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: "6710886400"
        storageClassName: standard
      status: {}
    volumesPerServer: 4
  - containerSecurityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
      seccompProfile:
        type: RuntimeDefault
    name: "pool-2"
    resources:
      requests:
        cpu: "1"
        memory: 2Gi
    runtimeClassName: ""
    securityContext:
      fsGroup: 1000
      fsGroupChangePolicy: OnRootMismatch
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
    servers: 4
    volumeClaimTemplate:
      metadata:
        creationTimestamp: null
        name: data
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: "6710886400"
        storageClassName: standard
      status: {}
    volumesPerServer: 4
  • Notice the MINIO_ARGS env variable value is as follows (get a shell in a minio container and cat /tmp/minio/config.env):
export MINIO_ARGS="https://myminio-pool-0-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} https://myminio-pool-1-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} https://myminio-pool-2-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1}"
  1. Decomission pool pool-1
mc admin decommission start ALIAS https://myminio-pool-1-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} --insecure

Wait for decomission to finish

 mc admin decommission status ALIAS --insecure                                                                                     
┌─────┬───────────────────────────────────────────────────────────────────────────────────────┬────────────────────────┬──────────┐
│ ID  │ Pools                                                                                 │ Raw Drives Usage       │ Status   │
│ 1st │ https://myminio-pool-0-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} │ 43.5% (total: 333 GiB) │ Active   │
│ 2nd │ https://myminio-pool-1-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} │ 43.5% (total: 333 GiB) │ Complete │
│ 3rd │ https://myminio-pool-2-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} │ 43.5% (total: 333 GiB) │ Active   │
└─────┴───────────────────────────────────────────────────────────────────────────────────────┴────────────────────────┴──────────┘
  1. Remove pool-1 from the tenant yaml
example Tenant (click to expand)
apiVersion: minio.min.io/v2
kind: Tenant
metadata:
  annotations:
    prometheus.io/path: /minio/v2/metrics/cluster
    prometheus.io/port: "9000"
    prometheus.io/scrape: "true"
  labels:
    app: minio
  name: myminio
  namespace: tenant-lite
spec:
  configuration:
    name: storage-configuration
  features:
    bucketDNS: false
  image: quay.io/minio/minio:RELEASE.2024-07-16T23-46-41Z
  imagePullSecret:
    name: ""
  mountPath: /export
  podManagementPolicy: Parallel
  pools:
  - containerSecurityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
      seccompProfile:
        type: RuntimeDefault
    name: pool-0
    servers: 4
    volumeClaimTemplate:
      metadata:
        name: data
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 2Gi
    volumesPerServer: 2
  - containerSecurityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 1000
      runAsNonRoot: true
      runAsUser: 1000
      seccompProfile:
        type: RuntimeDefault
    name: pool-2
    servers: 4
    volumeClaimTemplate:
      metadata:
        name: data
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 2Gi
    volumesPerServer: 2
  priorityClassName: ""
  requestAutoCert: true
  serviceAccountName: ""
  subPath: ""
  users:
  - name: storage-user
  1. Here is the Bug: Because the list of MINIO_ARGS is built from the .spec.status, the pool pool-1 should not appear, but instead Operator names pool-2 as pool-1 when forming the MINIO_ARGS.
export MINIO_ARGS="https://myminio-pool-0-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} https://myminio-pool-1-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1}"

With this fix the Pool names are propery labeled as pool-0 and pool-2

"https://myminio-pool-0-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1} https://myminio-pool-2-{0...3}.myminio-hl.tenant-lite.svc.cluster.local/export{0...1}",

@cniackz cniackz self-requested a review July 10, 2024 18:58
@cniackz cniackz added the wip label Jul 10, 2024
@cniackz
Copy link
Contributor

cniackz commented Jul 10, 2024

Thank you Pedro for this PR, waiting for this to be ready to review further ❤️

@harshavardhana
Copy link
Member

is it still valid?

Build from MINIO_ARGS from Pool status is incomplete and leads to duplicated args names when a pool don't have name `spec.pools.*.name`.

Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
@pjuarezd pjuarezd changed the title [wip]: Remove index-based pool verification [wip]: Remove index-based pool name verification Aug 5, 2024
@pjuarezd pjuarezd marked this pull request as ready for review August 5, 2024 21:47
@pjuarezd pjuarezd changed the title [wip]: Remove index-based pool name verification Remove index-based pool name verification Aug 5, 2024
@pjuarezd
Copy link
Member Author

pjuarezd commented Aug 5, 2024

Thank you Pedro for this PR, waiting for this to be ready to review further ❤️

this is ready for review @cniackz

@pjuarezd pjuarezd added bug fix and removed wip labels Aug 5, 2024
@harshavardhana harshavardhana merged commit 86722cf into minio:master Aug 6, 2024
21 checks passed
@pjuarezd pjuarezd deleted the pool-na-no-longer-optional branch August 14, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants