Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

upgrade cmd: using ICPS to compare changes between current install and target install. #671

Merged
merged 6 commits into from
Dec 12, 2019
Merged

upgrade cmd: using ICPS to compare changes between current install and target install. #671

merged 6 commits into from
Dec 12, 2019

Conversation

elfinhe
Copy link
Member

@elfinhe elfinhe commented Dec 6, 2019 •
edited
Loading

Fix: istio/istio#19135

Before this change, we used the values in Values API from the configmap of istio-sidecar-injector and the generated ICPS Values section, as the source of old and new installation configurations, which can cause some problems described in istio/istio#19135.

This PR switches the comparison from Values-based to ICPS-based.

Related: istio/istio#19451
I also get the kubectl outputs back at cmd/mesh/manifest-common.go:L85, which was removed by mistake, I think.

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Dec 6, 2019
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 6, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 6, 2019
@elfinhe elfinhe changed the title upgrade cmd switches to use ICPS to compare changes between current install and target install. upgrade cmd: using ICPS to compare changes between current install and target install. Dec 6, 2019
@elfinhe elfinhe marked this pull request as ready for review December 7, 2019 00:09
@elfinhe elfinhe requested a review from a team as a code owner December 7, 2019 00:09
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Dec 7, 2019
@nick4fake
Copy link

Relates to this one:
istio/istio#19398

BTW, is there any ETA for merging? Is there anything I can help with?

@elfinhe
Copy link
Member Author

elfinhe commented Dec 9, 2019

Relates to this one:
istio/istio#19398

BTW, is there any ETA for merging? Is there anything I can help with?

Hey Bohdan, can you elaborate why this is related to istio/istio#19398?

@nick4fake
Copy link

nick4fake commented Dec 9, 2019 •
edited
Loading

@elfinhe Because you can't use exiting IstioControlPlane API to control telemetry components (like istio) AND some parameters from Helm are not supported (I've provided values.kiali.tolerations as an example).
Setting them through unvalidatedValues also impossible as it is not used when merging manifest definitions on istioctl manifest apply.
This PR doesn't address original issue completely, but at least provides a dirty way to specify Helm values.

I might be wrong though, checking this PR right now.

@elfinhe
Copy link
Member Author

elfinhe commented Dec 9, 2019

@elfinhe Because you can't use exiting IstioControlPlane API to control telemetry components (like istio) AND some parameters from Helm are not supported (I've provided values.kiali.tolerations as an example).
Setting them through unvalidatedValues also impossible as it is not used when merging manifest definitions on istioctl manifest apply.
This PR doesn't address original issue completely, but at least provides a dirty way to specify Helm values.

I might be wrong though, checking this PR right now.

@nick4fake thanks, this PR is addressing some upgrade-specific validation issue, i.e., choosing which part in ICPS to validate for upgrade. so I think it is not related. In #19398, I think Xinnan will do something to fix the helm values issue.

@nick4fake nick4fake mentioned this pull request Dec 9, 2019
Closed
@nick4fake
Copy link

@elfinhe Understood, I've created a separate issue
istio/istio#19480

ostromart
ostromart reviewed Dec 9, 2019
View reviewed changes
cmd/mesh/upgrade.go Outdated
if diff == "" {
l.logAndPrintf("Upgrade check: Values unchanged. The target values are identical to the current values.\n")
l.logAndPrintf("Upgrade check: ICPS unchanged. The target values are identical to the current values.\n")
} else {
l.logAndPrintf("Upgrade check: Warning!!! The following values will be changed as part of upgrade. "+
"If you have not overridden these values, they will change in your cluster. Please double check they are correct:\n%s", diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

"If you have not overridden these values" doesn't seem to be applicable anymore because this is exactly what we're checking for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

cmd/mesh/upgrade.go Outdated
// It separately returns a string which represents just the overlay values in the returned ICPS.
func genOverlayICPS(filename string, force bool) (string, *v1alpha2.IstioControlPlaneSpec, error) {
// genOverlayICPS reads an ICPS from filename and returns the YAML of it
func genOverlayICPS(filename string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function still necessary? We seem to be just reading a file and returning the contents, not generating anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 11, 2019
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 11, 2019
@ostromart ostromart added the cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch label Dec 12, 2019
@istio-testing istio-testing merged commit c036885 into istio:master Dec 12, 2019
@istio-testing
Copy link

In response to a cherrypick label: #671 failed to apply on top of branch "release-1.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	cmd/mesh/manifest-common.go
M	cmd/mesh/manifest-generate_test.go
M	cmd/mesh/profile-common.go
M	cmd/mesh/upgrade.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/mesh/upgrade.go
Auto-merging cmd/mesh/profile-common.go
CONFLICT (content): Merge conflict in cmd/mesh/profile-common.go
Auto-merging cmd/mesh/manifest-generate_test.go
CONFLICT (content): Merge conflict in cmd/mesh/manifest-generate_test.go
Auto-merging cmd/mesh/manifest-common.go
CONFLICT (content): Merge conflict in cmd/mesh/manifest-common.go
Patch failed at 0001 upgrade uses ICPS to compare changes between current install and target install.

@elfinhe elfinhe mentioned this pull request Jan 13, 2020
Closed
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ostromart ostromart ostromart approved these changes

Assignees
No one assigned
Labels
cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. 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.

upgrade uses only the global portion of config for upgrade
5 participants
@elfinhe @nick4fake @istio-testing @ostromart @googlebot