zilliztech / milvus-helm

Apache License 2.0
54 stars 41 forks source link

K8s based podDisruptionBudget for etcd #74

Closed Rachit-Chaudhary11 closed 5 months ago

Rachit-Chaudhary11 commented 6 months ago

What this PR does / why we need it:

Higher versions of Kubernetes fail to work with etcd policy/v1beta1, hence a configurable policy based on the kubernetes version is needed.

Error Message: No matches for kind "PodDisruptionBudget" in version "policy/v1beta1".

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Rachit-Chaudhary11 commented 6 months ago

/assign @zwd1208 @haorenfsa please verify

haorenfsa commented 6 months ago

@Rachit-Chaudhary11 Lint not pass: https://github.com/zilliztech/milvus-helm/actions/runs/8214018017/job/22466048776?pr=74

Could you also post it here what changes did you do to the etcd chart? since it's a binary in the repo.

Rachit-Chaudhary11 commented 6 months ago

@Rachit-Chaudhary11 Lint not pass: https://github.com/zilliztech/milvus-helm/actions/runs/8214018017/job/22466048776?pr=74

Could you also post it here what changes did you do to the etcd chart? since it's a binary in the repo.

@haorenfsa I have made the changes to the following files (_helpers.tpl, pdm.yaml) inside etcd-6.3.3.tgz

  1. ../charts/milvus/charts/etcd-6.3.3.tgz!/etcd-6.3.3/etcd/templates/_helpers.tpl
Screenshot 2024-03-12 at 8 54 37 PM
  1. ..charts/milvus/charts/etcd-6.3.3.tgz!/etcd-6.3.3/etcd/templates/pdb.yaml Screenshot 2024-03-12 at 8 55 48 PM
Rachit-Chaudhary11 commented 6 months ago

@haorenfsa similar check is done for minio

Refer- https://github.com/zilliztech/milvus-helm/blob/master/charts/minio/templates/_helpers.tpl#L35

haorenfsa commented 6 months ago

@Rachit-Chaudhary11 looks the etcd-6.3.3.tgz binary is not gzipped:

image

How did you package it? usually we use the command like: tar -czf etcd-6.3.3.tgz .

Rachit-Chaudhary11 commented 6 months ago

@Rachit-Chaudhary11 looks the etcd-6.3.3.tgz binary is not gzipped:

image

How did you package it? usually we use the command like: tar -czf etcd-6.3.3.tgz .

@haorenfsa I believe I have done the same. May I know what is exactly the issue? Basically what I did is unarchieve it and placed it a different place, made the changes, archived it and move the archived folder again back to charts/milvus/charts which invariably replaced this existing tgz for etcd.

haorenfsa commented 6 months ago

@Rachit-Chaudhary11 it's packaged, but not archived through gzip, since we can still see the preview of its content. https://github.com/zilliztech/milvus-helm/blob/0d63040bd493efc3a136d4064591bf5d14c41a1b/charts/milvus/charts/etcd-6.3.3.tgz

The gziped content cannot be previewed, like: https://github.com/zilliztech/milvus-helm/blob/0d63040bd493efc3a136d4064591bf5d14c41a1b/charts/milvus/charts/minio-8.0.17.tgz

haorenfsa commented 6 months ago

@Rachit-Chaudhary11 please check if -z option is included in your tar command

sre-ci-robot commented 5 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Rachit-Chaudhary11 To complete the pull request process, please ask for approval from zwd1208 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/zilliztech/milvus-helm/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Rachit-Chaudhary11 commented 5 months ago

@haorenfsa Thank you for the suggestion. I have zipped the etcd directory again and checked-in. Kindly validate.

haorenfsa commented 5 months ago

Hi @Rachit-Chaudhary11 , please squash your commits into 1. and then sign it off.

For squash you can use command:

Then you can sign off with git commit --amend -s, then update your final commit msg, and save it with :x

haorenfsa commented 5 months ago

@Rachit-Chaudhary11 Oh, by the way, since we start to maintain the etcd chart. It's better to also track the source code in the repo like we did for minio. Could you please also add an etcd folder under /charts/ with the source code of the etcd chart?

image
Rachit-Chaudhary11 commented 5 months ago

@haorenfsa as suggested I have added the source code but that has invariably caused the helm and other issues above. Is it okay if you can add the source code at your end

Rachit-Chaudhary11 commented 5 months ago

@haorenfsa @zwd1208 please check for the above errors due to the addition of the suggested changes.

Rachit-Chaudhary11 commented 5 months ago

@haorenfsa @zwd1208 can you please help in getting this PR merged.

haorenfsa commented 5 months ago

@Rachit-Chaudhary11 Please make sure the checks pass. image

Rachit-Chaudhary11 commented 5 months ago

@haorenfsa Due to some rebasing issues, I am unable to proceed. Closing this PR and creating a new one having exactly the same fixes. PR #82