Operator indefinitely reconciling CR with Deployed Helm Release due to Basic String Comparison

[muralov]

Bug Report

What did you do?
We have a chart which is dependent on other 4 helm charts. We have deployed all the charts with helm operator.

What did you expect to see?
The operator deployed our application well and it is working well. Also, reconciling is working well.

What did you see instead? Under which circumstances?
However, we have noticed that the operator is constantly updating our CR and some resources (.e.g secrets, network policies). We had huge secrets with our helm charts which lead to huge CR (1mb). Indefinite update of the CR brought our ETCD down, where ETCD duplicates resources if a new version of a resource comes.

Are you writing your operator in ansible, helm, or go?
helm

Possible Solution
It can be solved by implementiong deep equal for the line https://github.com/operator-framework/operator-sdk/blob/master/pkg/helm/release/manager.go#L139

Additional context
I have seen in the source code that basic string comparison is done between candidate and deployed release manifests strings. However, 'helm install --dry-run' is generating different texts where content is the same, but the order of some resources are changed. Here is comparison line https://github.com/operator-framework/operator-sdk/blob/master/pkg/helm/release/manager.go#L139

[joelanford]

@muralov Just checking. Do your templates use any functions that generate random data?

Are you able to show some examples of this behavior from the operator logs? I was under the impression that the Helm libraries had a deterministic sort of the resources in the manifest, but I could be wrong on that.

[muralov]

@joelanford Our templates don't use any random data.

I have tried our helm charts with helm install --dry-run and got most of the time diffs, but sometimes not. I run 'helm template' command and I got always the same string, i.e. no diffs. helm template seems to consider ordering of the resources and generate the same string always.

The logs show the diffs for secrets, network policies although they don't change. After having a look at the source code I tested with dry-run I saw it genereates different string most of the time.

[joelanford]

@muralov Thanks for the info. This is something we should be able to fix. So just to make sure I understand, is it the order of the resources that change (e.g. sometimes a secret comes before a network policy and sometimes that same network policy comes before the secret)?

[muralov]

@joelanford yes, that is correct.

[joelanford]

@muralov I started looking at the call stack and if I'm not mistaken, both helm template and the helm-operator end up using the same sorting interface implementation to sort the manifests no matter whether it's an install, update, or update dry-run.

https://github.com/helm/helm/blob/618447cbf203d147601b4b9bd7f8c37a5d39fbb4/pkg/tiller/kind_sorter.go#L122-L150

Is that non-deterministic for your chart's resources? Are you able to make a sharable example that reproduces the problem consistently?

Call stack for helm template

1. https://github.com/helm/helm/blob/v2.13.1/cmd/helm/template.go#L229
2. https://github.com/helm/helm/blob/618447cbf203d147601b4b9bd7f8c37a5d39fbb4/pkg/tiller/kind_sorter.go#L155-L156

Call stack for helm-operator (install)

1. operator-sdk/pkg/helm/release/manager.go

   Line 224 in c2b49e8

   releaseResponse, err := tiller.InstallRelease(ctx, installReq)

2. operator-sdk/vendor/k8s.io/helm/pkg/tiller/release_install.go

   Line 36 in c2b49e8

   rel, err := s.prepareRelease(req)

3. operator-sdk/vendor/k8s.io/helm/pkg/tiller/release_install.go

   Line 87 in c2b49e8

   hooks, manifestDoc, notesTxt, err := s.renderResources(req.Chart, valuesToRender, req.SubNotes, caps.APIVersions)

4. operator-sdk/vendor/k8s.io/helm/pkg/tiller/release_server.go

   Line 330 in c2b49e8

   hooks, manifests, err := sortManifests(files, vs, InstallOrder)

5. operator-sdk/vendor/k8s.io/helm/pkg/tiller/hooks.go

   Line 106 in c2b49e8

   return result.hooks, sortByKind(result.generic, sort), nil

6. operator-sdk/vendor/k8s.io/helm/pkg/tiller/kind_sorter.go

   Lines 96 to 97 in c2b49e8

   	ks := newKindSorter(manifests, ordering)
   	sort.Sort(ks)

Call stack for helm-operator (update)

1. operator-sdk/pkg/helm/release/manager.go

   Line 264 in c2b49e8

   releaseResponse, err := tiller.UpdateRelease(ctx, updateReq)

2. operator-sdk/vendor/k8s.io/helm/pkg/tiller/release_update.go

   Line 39 in c2b49e8

   currentRelease, updatedRelease, err := s.prepareUpdate(req)

3. operator-sdk/vendor/k8s.io/helm/pkg/tiller/release_update.go

   Line 116 in c2b49e8

   hooks, manifestDoc, notesTxt, err := s.renderResources(req.Chart, valuesToRender, req.SubNotes, caps.APIVersions)

And the rest is the same as the install stack from 4-6.

[muralov]

@joelanford sorry for the delay. Unfortunately, I cannot share our charts because I am not allowed to share. Today I have run 'helm install --dry-run' and 'helm template' again and I have got different strings again. As I said, with dry-run the results almost each time different, but with helm template the results are always the same.
We have fixed this diff problem by splitting the whole text with '---' and compare each k8s object manifest string independently. After applying this change, the Helm Operator indefinite reconcile is stopped. Hence, the problem is really with the basic string diff.

[joelanford]

@muralov Hmm. And you're not able to create a sharable chart that reproduces the issue?

A few more questions to see if I can narrow this down:

1. What version of helm are you using on the command line when you run helm install --dry-run and helm template
2. What version of the helm-operator are you using?
3. For your diff where you split and compare, what are you doing to ensure that the each list of resources is in the same order?

[muralov]

@joelanford I will try to create such a chart, please give me some time.

Here are my answers to your questions:

1. helm version is v2.12.3. Yes, I am using command line. I am also using cli diff command.
2. We are using operator-sdk version: v0.8.1-dirty, commit: 33b3bfe
3. Here is code snippet of our solution, this is our fast solution:

func (m *manager) Sync(ctx context.Context) error {
	// Get release history for this release name
	releases, err := m.storageBackend.History(m.releaseName)
	if err != nil && !notFoundErr(err) {
		return fmt.Errorf("failed to retrieve release history: %s", err)
	}

	// Cleanup non-deployed release versions. If all release versions are
	// non-deployed, this will ensure that failed installations are correctly
	// retried.
	for _, rel := range releases {
		if rel.GetInfo().GetStatus().GetCode() != rpb.Status_DEPLOYED {
			_, err := m.storageBackend.Delete(rel.GetName(), rel.GetVersion())
			if err != nil && !notFoundErr(err) {
				return fmt.Errorf("failed to delete stale release version: %s", err)
			}
		}
	}

	// Load the chart and config based on the current state of the custom resource.
	chart, config, err := m.loadChartAndConfig()
	if err != nil {
		return fmt.Errorf("failed to load chart and config: %s", err)
	}
	m.chart = chart
	m.config = config

	// Load the most recently deployed release from the storage backend.
	deployedRelease, err := m.getDeployedRelease()
	if err == ErrNotFound {
		return nil
	}
	if err != nil {
		return fmt.Errorf("failed to get deployed release: %s", err)
	}
	m.deployedRelease = deployedRelease
	m.isInstalled = true

	// Get the next candidate release to determine if an update is necessary.
	candidateRelease, err := m.getCandidateRelease(ctx, m.tiller, m.releaseName, chart, config)
	if err != nil {
		return fmt.Errorf("failed to get candidate release: %s", err)
	}

	manifestSeparator := "---"
	candidateManifests := strings.Split(candidateRelease.GetManifest(), manifestSeparator)
	deployedManifests := strings.Split(deployedRelease.GetManifest(), manifestSeparator)
	hasDifferentManifestCounts := len(candidateManifests) != len(deployedManifests)
	m.isUpdateRequired = hasDifferentManifestCounts
	if !hasDifferentManifestCounts {
		for i := 0; !m.isUpdateRequired && i < len(candidateManifests); i++ {
			indexOfMatch := indexOf(candidateManifests[i], deployedManifests)
			hasNoMatch := indexOfMatch < 0
			if hasNoMatch {
				m.isUpdateRequired = true
			}
		}
	}

	if m.isUpdateRequired {
		fmt.Printf("Update is required.")
	}

	return nil
}

func isEqualBitwise(text1 string, text2 string) bool {
	hasDifferentCharCounts := len(text1) != len(text2)
	foundDiff := hasDifferentCharCounts
	if foundDiff {
		return false
	}
	for pos := 0; !foundDiff && pos < len(text2); pos++ {
		hasDifferentValues := text2[pos] != text1[pos]
		if hasDifferentValues {
			foundDiff = true
		}
	}
	return !foundDiff
}

func indexOf(search string, texts []string) int {
	trimChars := "\n\r \t"
	for key, text := range texts {
		text = strings.Trim(text, trimChars)
		search = strings.Trim(search, trimChars)
		if isEqualBitwise(search, text) {
			return key
		}
	}
	return -1
}

[joelanford]

@muralov Just wanted to follow up and see if you've been able to reproduce this with a sharable chart?

Today I have run 'helm install --dry-run' and 'helm template' again and I have got different strings again.

I didn't catch this comment of yours last time, but if you're seeing similar differences with the Helm CLI tools, then I'd suggest opening an issue against the Helm repo since that's where the fix would ultimately need to come from.

[muralov]

@joelanford sorry, it is not ready yet. Please give me some time.

Do you mean that helm --dry-run should generate same string? I am not sure whether helm needs to fix this because helm --dry-run is just for displaying the objects for visualisation. The objects order change should not be that bad. However, I find basic text diff is not so robust. If it is okay, I can contribute our fix where we compare each resource one-by-one, not the whole text at once.

[joelanford]

What I mean is that based on my perusal of helm's code, I would expect helm --dry-run and helm template to produce identical release manifests. So if you're seeing a difference between those, it would explain why you're seeing the same behavior in the helm operator. The helm operator is using the same underlying libraries as the Helm CLI to generate the manifest.

So my stance on this issue is that:

1. If helm --dry-run and helm template are supposed to produce identical release manifests, and they aren't, that's a bug in Helm that we should work to resolve upstream.
2. If there is no guarantee that helm --dry-run and helm template will produce identical release manifests, that's a bug in helm-operator that should be resolved here.

If you're able to reproduce this with a shareable example, I think we should open an issue upstream to see what the helm maintainers think. WDYT?

[muralov]

@joelanford sorry for delay! Yes, I agree with you. I will do that soon.