Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helm: don't specify default topology domainlabels in rbd chart #4776

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

Infinoid
Copy link
Contributor

Remove the bad default, add commented-out standard labels as a suggestion.

Describe what this PR does

This fixes a problem I noticed while digging into #4775.

It brings the default topology.domainLabels value in line with what the README.md claims (default={}), and in line with what the installer manifest does (leaves the command-line parameter commented out).

Is there anything that requires special attention

I believe this change is beneficial enough to stand on its own. That said, these label definitions are 4 years old, and I don't know what's different between the 3.11.0 and 3.12.0 releases to cause #4775 to occur now.

I had to work around the problem with helm --set topology.domainLabels=[], which worked for me. This PR updates the default to reflect that, and updates the comments to suggest the standard topology labels.

There are two places in the helm chart which reference this value, both are guarded with:

{{- if .Values.topology.domainLabels }}

So I think it doesn't need to be an explicitly empty array ([]), leaving the whole thing commented out is fine.

Related issues

Fixes: #4775

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

@mergify mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Aug 17, 2024
@Infinoid
Copy link
Contributor Author

Updated to resolve lint failure:

./charts/ceph-csi-rbd/values.yaml
  303:3     warning  comment not indented like content  (comments-indentation)

@Infinoid
Copy link
Contributor Author

Updated again; my fix for yamllint made helm lint unhappy. This version tries to make both of them happy.

Remove the bad default, add commented-out standard labels as a suggestion.

Fixes: #4775
Signed-off-by: Mark Glines <mark@glines.org>
@Infinoid
Copy link
Contributor Author

Update: minimize diff, preserve the original indentation.

Copy link
Contributor

@iPraveenParihar iPraveenParihar left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks!

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Aug 19, 2024

@Mergifyio queue

@Madhu-1 Madhu-1 added the backport-to-release-v3.12 Label to backport from devel to release-v3.12 branch label Aug 19, 2024
Copy link
Contributor

mergify bot commented Aug 19, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Aug 19, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 19, 2024
@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Aug 19, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@nixpanic
Copy link
Member

@Mergifyio requeue

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 19, 2024
Copy link
Contributor

mergify bot commented Aug 19, 2024

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-release-v3.12 Label to backport from devel to release-v3.12 branch component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSI NodePlugin with v.3.12.0 driver fails when missing domain labels
5 participants