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

Experimental: create a helm release from istioctl install #51696

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Experimental: create a helm release from istioctl install #51696

wants to merge 3 commits into from

Conversation

howardjohn
Copy link
Member

part of #44814

With this change, Istioctl will output a helm release. This means the install shows up in helm ls, and a user can seemless switch to helm.
Without this, helm will barf with

Error: Unable to continue with install: PodDisruptionBudget "istiod" in namespace "istio-system" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "pilot"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "istio-system"

We can make a change to istioctl to do a one-off migration, but it is not ideal IMO.

@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2024
@Stono
Copy link
Contributor

Stono commented Jun 25, 2024 •
edited
Loading

You could just set the annotations on the resources which would allow helm to "adopt" them as and when someone switches to helm?

  metadata:
    annotations:
      meta.helm.sh/release-name: istio
      meta.helm.sh/release-namespace: istio-system

@howardjohn
Copy link
Member Author

Yeah if we don't do this, we will likely create a helper like helm/helm#2730 (comment). The thought was this might have other benefits.

Largely the goal is to make istioctl install behave identically to helm install. It might not make sense to have this intermediate step, and instead wait for istio/enhancements#166 which would allow us to rip out a lot of the cruft

@bleggett
Copy link
Contributor

👍 on

It might not make sense to have this intermediate step, and instead wait for istio/enhancements#166 which would allow us to rip out a lot of the cruft

Even if it doesn't let us rip it all out, even if we still need this, it will shrink clarify the problem.

@whitneygriffith
Copy link
Contributor

This is a great path forward! And I think should be done istio/enhancements#166

Will this still be relevant when transitioning to the Helm comandeer approach when helm/helm#2730 is resolved?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 22, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@istio-policy-bot
Copy link

🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2024-06-27. It will be closed on 2024-08-11 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costinm costinm Awaiting requested review from costinm costinm will be requested when the pull request is marked ready for review costinm is a code owner

@linsun linsun Awaiting requested review from linsun linsun will be requested when the pull request is marked ready for review linsun is a code owner

@stevenctl stevenctl Awaiting requested review from stevenctl stevenctl will be requested when the pull request is marked ready for review stevenctl is a code owner

@ericvn ericvn Awaiting requested review from ericvn ericvn will be requested when the pull request is marked ready for review ericvn is a code owner

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu hzxuzhonghu will be requested when the pull request is marked ready for review hzxuzhonghu is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@howardjohn @istio-testing @Stono @bleggett @whitneygriffith @istio-policy-bot