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

K8s based podDisruptionBudget for etcd #74

Closed
wants to merge 15 commits into from

Conversation

Rachit-Chaudhary11
Copy link
Contributor

@Rachit-Chaudhary11 Rachit-Chaudhary11 commented Mar 9, 2024

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.]

  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [mychartname])
  • PR only contains changes for one chart

Signed-off-by: Rachit Chaudhary - r0c0axe <[email protected]>
@Rachit-Chaudhary11
Copy link
Contributor Author

/assign @zwd1208
@haorenfsa please verify

@haorenfsa
Copy link
Collaborator

haorenfsa commented Mar 12, 2024

@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
Copy link
Contributor Author

Rachit-Chaudhary11 commented Mar 12, 2024

@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
Copy link
Contributor Author

@haorenfsa
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@haorenfsa
Copy link
Collaborator

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

Signed-off-by: Rachit Chaudhary - r0c0axe <[email protected]>
@sre-ci-robot
Copy link
Collaborator

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Rachit Chaudhary - r0c0axe <[email protected]>
@mergify mergify bot added the ci-passed label Mar 19, 2024
@Rachit-Chaudhary11
Copy link
Contributor Author

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

@haorenfsa
Copy link
Collaborator

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

For squash you can use command:

  • git rebase -i HEAD~11 (you have 11 commits in all)
  • then, in the interactive vim: change all the pick to squach, then save it with :x
image

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

@haorenfsa
Copy link
Collaborator

@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

Signed-off-by: Rachit Chaudhary - r0c0axe <[email protected]>
@Rachit-Chaudhary11
Copy link
Contributor Author

@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 Rachit-Chaudhary11 changed the title Bug Fix - podDisruptionBudget for etcd is hardcoded to policy/v1beta1 podDisruptionBudget for etcd d Mar 27, 2024
@Rachit-Chaudhary11 Rachit-Chaudhary11 changed the title podDisruptionBudget for etcd d K8s based podDisruptionBudget for etcd Mar 27, 2024
@Rachit-Chaudhary11
Copy link
Contributor Author

Rachit-Chaudhary11 commented Mar 29, 2024

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

@Rachit-Chaudhary11
Copy link
Contributor Author

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

@haorenfsa
Copy link
Collaborator

haorenfsa commented Apr 1, 2024

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

Signed-off-by: Rachit-Chaudhary11 <[email protected]>
@Rachit-Chaudhary11
Copy link
Contributor Author

Rachit-Chaudhary11 commented Apr 1, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants