Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Confirm release belongs to HR before upgrading #2123

Merged
merged 2 commits into from
Jun 13, 2019

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Jun 3, 2019

Before this change, when a user would create multiple HelmReleases
with the same release name configured, the operator would attempt
to upgrade the release indefinitely.

To overcome this issue, the operator now injects the resource ID
of the HelmRelease into the description of the release and confirms
this value equals the resource ID of the HelmRelease it is going to
run an upgrade for. If the value diverges, the status of the
HelmRelease is updated to make it visible to the user and a message
is logged.

To overcome this issue, we now check if the resources created by the
release are patched with the correct antecedent annotation, which we
create ourselves and have full control over (and survives rollbacks).

For backwards compatibility, and to be able to migrate existing
releases to a HelmRelease, we see empty descriptions as a positive.

Fixes #2101

@hiddeco hiddeco added the helm label Jun 3, 2019
@hiddeco hiddeco force-pushed the helm/2101-duplicate-release-name branch from aa99331 to 1418c30 Compare June 3, 2019 16:47
squaremo
squaremo previously approved these changes Jun 3, 2019
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Yep, a reasonable use of the mechanisms available, to fix the problem 👍

A minor suggestion, that need not hold this up: to make the description more readable, in case a human looks at it, could that be "Owned by: "+fhrResourceID perhaps.

@hiddeco
Copy link
Member Author

hiddeco commented Jun 3, 2019

A minor suggestion, that need not hold this up: to make the description more readable, in case a human looks at it, could that be "Owned by: "+fhrResourceID perhaps.

After running some tests I came to the conclusion it needs further tweaking, as in case no description is given, Helm will write its own description (<task> complete) and to make it even more fun, we will also have issues when a user performs a rollback as this will result in a Rollback to <version> description.

As these messages are simple strings in the Helm code base (and not public constants as I hoped they would be), writing (and maintaining) logic based on this does not seem preferable to me.

The other option I could think of to fix this problem is the flux.weave.works/antecedent annotation we patch all resources with after a successful install/upgrade, which seems to be a better path, this would require increasing the timeout of the context in annotateResources, as we have had user reports of this timing out for large umbrella releases.

@squaremo squaremo dismissed their stale review June 4, 2019 10:58

Hidde brought up some problems, so let's remove the green light for now

@hiddeco hiddeco force-pushed the helm/2101-duplicate-release-name branch 2 times, most recently from 5ee12bd to 6775c68 Compare June 4, 2019 12:46
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

OK, yep this works as a "who last touched it" check, which is a reasonable way to detect whether some other HelmRelease is trying to be in control.

There's a couple of situations where it won't detect what's happening -- but I don't know how it could:

  1. Some other automated process is releasing the helm chart to the same release name. In this case, it won't annotate the resources, and helm-op will treat that as indicating that it can go ahead. Arguably, it should re-release what it's been told to, since the whole idea is to use HelmReleases declare what is supposed to be there.
  2. If another HelmRelease has released a chart, but the annotation step fails. There's not much we can do about that -- the release has already happened by that point. At least that situation is likely to be transient, and should eventually just correct itself.

Before this change, when a user would create multiple HelmReleases
with the same release name configured, the operator would attempt
to upgrade the release indefinitely.

To overcome this issue, the operator now injects the resource ID
of the HelmRelease into the description of the release and confirms
this value equals the resource ID of the HelmRelease it is going to
run an upgrade for. If the value diverges, the status of the
HelmRelease is updated to make it visible to the user and a message
is logged.

For backwards compatibility, and to be able to migrate existing
releases to a HelmRelease, we see empty descriptions as a positive.
Due to Helm creating its own descriptions when the user does not supply
one, the previous approach got shot as we would be unable to determine
ownership after manual helm actions.

The new approach is to check if the resources created by the release
are patched with the correct antecedent annotation, which we create
ourselves and have full control over (and survives rollbacks).

To be able to migrate existing releases to a HelmRelease, empty
(missing) annotations are still handled as true / owned by.
@hiddeco hiddeco force-pushed the helm/2101-duplicate-release-name branch from 6775c68 to aadb430 Compare June 13, 2019 12:16
@hiddeco hiddeco merged commit bbccd7b into master Jun 13, 2019
@hiddeco hiddeco deleted the helm/2101-duplicate-release-name branch June 13, 2019 12:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm Operator will try to apply multiple times when two of more releases have the same name
2 participants