Skip to content

fixed issue #35591, remove Istio operator CR with specified revision … #35720

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
Merged
merged 3 commits into from
Oct 26, 2021
Merged

fixed issue #35591, remove Istio operator CR with specified revision … #35720

merged 3 commits into from
Oct 26, 2021

Conversation

@zhlsunshine
Copy link
Contributor

fixed issue #35591, remove Istio operator CR with specified revision if it is uninstalled

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

Please check any characteristics that apply to this pull request.

  • Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

@zhlsunshine zhlsunshine requested a review from a team as a code owner October 25, 2021 07:07
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 25, 2021
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 25, 2021
@zhlsunshine zhlsunshine added release-notes-none Indicates a PR that does not require release notes. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 25, 2021
@zhlsunshine
Copy link
Contributor Author

$ istioctl install --set profile=demo -y
$ stioctl install --set profile=demo -y --set revision=canary
$ istioctl x revision list

REVISION TAG      ISTIO-OPERATOR-CR                   PROFILE REQD-COMPONENTS
default  default  istio-system/installed-state        demo    base
                                                              istiod
                                                              ingress:istio-ingressgateway
                                                              egress:istio-egressgateway
canary   <no-tag> istio-system/installed-state-canary demo    base
                                                              istiod
                                                              ingress:istio-ingressgateway
                                                              egress:istio-egressgateway

$ istioctl x uninstall --revision canary
$ istioctl x revision list

REVISION TAG     ISTIO-OPERATOR-CR            PROFILE REQD-COMPONENTS
default  default istio-system/installed-state demo    base
                                                      istiod
                                                      ingress:istio-ingressgateway
                                                      egress:istio-egressgateway

@zhlsunshine
Copy link
Contributor Author

/test integ-security-k8s-tests_istio

morvencao
morvencao reviewed Oct 25, 2021
View reviewed changes
operator/pkg/helmreconciler/common.go Outdated
return nil
}

// getIstioOperatorCRDName get the Istio operator crd name based on specified revision
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/getIstioOperatorCRDName/getIstioOperatorCRName/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morvencao okay, done!

operator/pkg/helmreconciler/prune.go Outdated
Comment on lines 213 to 227
} else {
// Remove Istio operator CR with specified revision if it is uninstalled
ioplist := h.getIstioOperatorCR()
if ioplist.Items != nil {
for _, iop := range ioplist.Items {
revisionIop := getIstioOperatorCRDName(revision)
if iop.GetName() == revisionIop {
if iopToList, err := iop.ToList(); err == nil {
iopToList.Items = []unstructured.Unstructured{iop}
usList = append(usList, iopToList)
break
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

not sure if it's proper to add getting IOP CR logic here, GetPrunedResources is used to get resources for specified component, for IOP instance with multiple enabled components, the IOP CR will be added to usList multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @morvencao, thanks for your comment, I have checked that there are only 2 places to call GetPrunedResources: one is uninstall and the other one is to get resources for specified component for some enabled components. I only need to handle the former, so I added an assertion for this. Thanks again!

@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 26, 2021
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@zhlsunshine
Copy link
Contributor Author

/test integ-pilot-multicluster-tests_istio

@istio-testing istio-testing merged commit 42ab59a into istio:master Oct 26, 2021
@zhlsunshine zhlsunshine deleted the 35591 branch October 26, 2021 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@morvencao morvencao morvencao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zhlsunshine @istio-policy-bot @morvencao @istio-testing