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

muralov commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@joelanford yes, that is correct.

joelanford commented 5 years ago

@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`

Call stack for helm-operator (install)

Call stack for helm-operator (update)

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

muralov commented 5 years ago

@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 commented 5 years ago

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

What version of helm are you using on the command line when you run helm install --dry-run and helm template
What version of the helm-operator are you using?
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 commented 5 years ago

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

Here are my answers to your questions:

helm version is v2.12.3. Yes, I am using command line. I am also using cli diff command.
We are using operator-sdk version: v0.8.1-dirty, commit: 33b3bfe10176f8647f5354516fff29dea42b6342

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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:

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.
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 commented 5 years ago

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

ssalaues commented 5 years ago

@joelanford @muralov I've noticed something similar/related as well but in my case it was due to my usage of .Release.IsInstall to run a Job only at chart install time. This caused the helm operator to see a diff with the dry run which gets templated as IsUpgrade triggering a reconciliation and removing the job before it completes.

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

I would say that there is no real guarantee that both of these commands will produce identical releases at the very least because of how Helm templating can alter the manifests simply based off the types of operation. In my case for example helm template can show the same as helm install --dry-run but not helm upgrade --dry-run

Here's the chart I'm using although I had to remove the .Release.IsInstall check for compatibility with the Helm Operator. While this allows my chart to work in the interim, any changes to the job definition in the chart causes failed upgrades by the Operator since it tries to update an immutable job spec. https://github.com/scality/Zenko/blob/development/1.1/kubernetes/cosmos/templates/rclone-cronjob.yaml#L86

joelanford commented 5 years ago

@ssalaues Thanks for bringing this up. It isn't something we had considered. In the non-operator case, what would happen with your chart if it was installed, then quickly upgraded while the Job was still running (say the installer noticed a typo'd value and wanted to quickly fix it)?

Would the Job get deleted in that case?

I agree that this is a slightly different problem than the one mentioned in this issue, which has the expectation that installs and upgrades will have the same outputs given the same inputs.

What you're describing is that you expect the install and upgrade manifest to be different even when inputs are identical (based on templates that use .Release.IsInstall and .Release.IsUpgrade).

Can you think of a way to make such a scenario work with the Helm operator? It seems like those template conditionals effectively make the chart non-idempotent and therefore not really compatible with the idea of Kubernetes idempotent reconciliations.

ssalaues commented 5 years ago

In the non-operator case, what would happen with your chart if it was installed, then quickly upgraded while the Job was still running (say the installer noticed a typo'd value and wanted to quickly fix it)?

It would be the same behavior as you describe. I've tried to use helm hooks so it doesn't delete the resource which seemed more appropriate since there are post-install hooks. However Helm blocks/waits until all helm hooks are done to report a successful release. So long running post-install jobs just end up causing the helm operator to wait until a job finishes therefore blocking any other reconciliations. https://github.com/helm/helm/issues/5891

But I digress.

Can you think of a way to make such a scenario work with the Helm operator?

Yeah my solution ultimately was to switch the post-install job to simply a post-install pod definition that runs to completion. This doesn't block any other Helm operations and allows the process to run only at install time.

muralov commented 4 years ago

@joelanford I see that @ssalaues have this issue too. Do you still need sample helm chart from me?

joelanford commented 4 years ago

@muralov I think @ssalaues's issue is different because the release manifest changes depending on whether it's an install or an upgrade. That breaks the helm-operator's assumption that the chart is idempotent. If I understand correctly, @ssalaues did not have an issue with non-deterministic release manifest sorting.

If you have a reproducer, that would be super helpful.

As an aside, we're working on making the switch to Helm v3. I haven't seen anything about this specifically being fixed, but it's possible that the issue goes away with Helm v3. It might be worth checking to see if you can reproduce this with the Helm v3 CLI or with a build of the SDK from the branch in https://github.com/operator-framework/operator-sdk/pull/2080

muralov commented 4 years ago

@joelanford thanks for your reply! It was for me a bit difficult to reproduce, then somehow I gave up, however, I will try once more.

Let me try with Helm v3 CLI for helm --dry-run. I will aslo try with a build of the branch 2080.

joelanford commented 4 years ago

@muralov I'm going to close this for now since it's pretty stale and I haven't been able to reproduce it, but if you are able to provide a reproducer, please ping me and we can re-open!

Also, BTW, the Helm 3 PR has been merged to master and will be in the next release (probably this week).

inercia commented 4 years ago

I'm seeing the same problem and it seems to be a question of sorting.

When debugging the code in these lines,

    // Get the next candidate release to determine if an update is necessary.
    candidateRelease, err := m.getCandidateRelease(m.namespace, m.releaseName, m.chart, m.values)
    if err != nil {
        return fmt.Errorf("failed to get candidate release: %w", err)
    }
    if deployedRelease.Manifest != candidateRelease.Manifest {
        m.isUpdateRequired = true
    }

I attached the debugger, saved deployedRelease.Manifest and candidateRelease.Manifest to two files and found the difference:

402a403
> # Configure DevPortal
407c408
<   name: example-ambassadorinstallation-devportal-api
---
>   name: example-ambassadorinstallation-devportal
415c416
<     app.kubernetes.io/component: ambassador-devportal-api
---
>     app.kubernetes.io/component: ambassador-devportal
417,418c418,419
<   prefix: /openapi/
<   rewrite: ""
---
>   prefix: /documentation/
>   rewrite: "/docs/"
441d441
< # Configure DevPortal
446c446
<   name: example-ambassadorinstallation-devportal
---
>   name: example-ambassadorinstallation-devportal-api
454c454
<     app.kubernetes.io/component: ambassador-devportal
---
>     app.kubernetes.io/component: ambassador-devportal-api
456,457c456,457
<   prefix: /documentation/
<   rewrite: "/docs/"
---
>   prefix: /openapi/
>   rewrite: ""

The differences are just a matter of the order of the resources. I think that, instead of just comparing the manifests, we should compare 1) the list of resources created/deleted 2) the Manifest of the resource that match, one by one.

joelanford commented 4 years ago

Re-opening. We have a reproducer in #2815.

I want to investigate Helm's sorting logic to make sure it isn't inadvertently introducing the non-deterministic sorting in release manifests. If it is, we should make an upstream fix.

It is possible that a chart template could introduce the non-determinism (e.g. by ranging over a map to produce manifests or the contents of individual manifests). So a solution that compares individual manifests still won't solve this in all cases because even those can be different if the template is introducing non-determinism.

So step 1: Let's investigate Helm's sorting logic to see if there's a fix to be made there.

joelanford commented 4 years ago

It looks like there was an issue in Helm's sorting logic that was causing this, but that issue was fixed in Helm v3.1.0 via helm/helm#6842.

The latest release of Operator SDK vendors Helm v3.1.2, so this issue should be fixed starting with Operator SDK v0.17.0.

To double check the fix, I used the Kasten K10 chart from #2815 and ran helm template about 10 times each with v3.0.2 (from SDK <v0.17.0) and v3.1.2 (from SDK v0.17.0) and saw that the order was inconsistent with v3.0.2 and consistent with v3.1.2.

Re-closing since this seems to be finally fixed both upstream and in Operator SDK.

The fix for this issue is to upgrade your helm-operator base image to v0.17.0.

operator-framework / operator-sdk