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

[stable/redis] Add chart and release labels in pods #6909

Merged
merged 1 commit into from
Jul 30, 2018
Merged

[stable/redis] Add chart and release labels in pods #6909

merged 1 commit into from
Jul 30, 2018

Conversation

javsalgar
Copy link
Collaborator

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2018
@carrodher
Copy link
Collaborator

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carrodher, javsalgar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2018
@k8s-ci-robot k8s-ci-robot merged commit f784384 into helm:master Jul 30, 2018
@desaintmartin
Copy link
Collaborator

desaintmartin commented Aug 30, 2018

Hello guys,
Doesn't all of these PRs break k8s/helm upgrades (helm/helm#2494) ?

marekaf pushed a commit to marekaf/charts that referenced this pull request Sep 4, 2018
Signed-off-by: Marek Bartik <mab@revolgy.com>
@paultiplady
Copy link

Confirmed, these changes appear to have broken backwards-compatibility with existing deployments, and should therefore require a major version bump, rather than including in a point release.

@desaintmartin
Copy link
Collaborator

desaintmartin commented Sep 7, 2018

What I mean is that with those changes, EVERY version (even patch version) upgrade will break, not only this one.

@paultiplady
Copy link

Is the problem you're raising that there is no satisfactory migration path (even if it requires modifying the deployment outside of helm)?

@desaintmartin
Copy link
Collaborator

Well, the (temporary) satisfactory path is to revert those changes so that there is no mutable labels and we can actually use those Charts in production.
There should be a consensus about this problem for all charts of this repository, clear understanding of the problem (helm vs k8s and its implications) and it should be written on the guidelines of this repo.

@paultiplady
Copy link

That seems correct to me. Helm requires its charts use SemVer, under which It's a bug to break upgrades on a patch release.

As long as there's a documented upgrade procedure for a back-incompatible change on major release that would comply with the requirements of SemVer, though depending on how painful that migration is you could also deem it unacceptable, or hide it behind a feature-flag so that existing users don't need to take the upgrade.

@desaintmartin
Copy link
Collaborator

desaintmartin commented Sep 7, 2018

Well, I think it is not a matter of semver.
Let's break down the problem.
1/ This patch is applied. It breaks upgrades. Whatever fix is taken in order to make it upgradable (even if the "fix" is "upgrading major release to mark it incompatible")
2/ Everybody is happy
3/ A minor patch change is made to this chart. Something very tiny like "fix typo in readme". It will again break upgrade of an existing Release of this Chart, because spec.template.metatdata.labels now contains a "chart" entry which itself contains the version. spec.template.metatdata.labels being immutable for some k8s reason (in fact, the selector.matchLabels generated from this is immutable), k8s is not happy and prevents any upgrade.

This is causing a few confusions, like here:
#7441
Where the root of the problem is not understood.

This is at least my understanding, but I'm actually happy if I'm wrong (because this problem is blocking for me).

You are talking about point 1, which does not contain the whole story.

@paultiplady
Copy link

Thanks for the clarification, my understanding of the issue was incomplete. You're right, it sounds like this change simply isn't compatible with the way that k8s metadata is managed.

Providing a concrete link from the API docs, https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#selector.

aba182 pushed a commit to aba182/charts that referenced this pull request Sep 7, 2018
Signed-off-by: aba182 <ajwilhel@gmail.com>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
gsemet pushed a commit to gsemet/charts that referenced this pull request Nov 13, 2018
Signed-off-by: Gaetan Semet <gaetan@xeberon.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants