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.

Sign up for GitHub

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

Jump to bottom

add commonLabels value #3438

Merged
merged 1 commit into from
Nov 2, 2022
Merged

add commonLabels value #3438

merged 1 commit into from
Nov 2, 2022

Conversation

bastienbosser
Copy link

Signed-off-by: BOSSER, Bastien bastien.bosser@atos.net

Describe what this PR does

Add a commonLabels variable in the values.yml file that allows to add additional labels on all helm chart resources.

As you know, labels can be used to organize and to select subsets of objects. In my case of use it allows me to group the applications by category and to check by a simple command the status of it.

As examples, for all security applications, I put a label "my-cluster-name/application_group: security". Then I just have to make the command "kubectl get all -A -l my-cluster-name/application_group=security" to know the status of all applications that provide security.

Related issues

Fixes: #3437

@nixpanic nixpanic added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Oct 17, 2022
nixpanic
nixpanic requested changes Oct 17, 2022
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks for the PR!

You will need to force-push a correction though. The subject of the commit should have a topic, and that is missing at the moment. I recommend using

deploy: add commonLabels value

or similar. The commit message itself can also have a few more details, basically copy/paste from the PR description.

Madhu-1
Madhu-1 reviewed Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

please add some test labels here https://github.com/ceph/ceph-csi/blob/devel/scripts/install-helm.sh#L172-L182 so that this will be get tested in CI

@bastienbosser bastienbosser force-pushed the feature-possibility-to-add-additional-labels branch from f88a3f1 to cdeae99 Compare October 26, 2022 14:28
@bastienbosser
Copy link
Author

@nixpanic I changed the commit message.
@Madhu-1 What type of test would you like me to develop? Should I just add the customLabels value in the helm deploy?

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Oct 26, 2022

@nixpanic I changed the commit message. @Madhu-1 What type of test would you like me to develop? Should I just add the customLabels value in the helm deploy?

Just add some sample custom labels to the above script

@bastienbosser
Copy link
Author

@Madhu-1 Are the tests suitable for you?

Madhu-1
Madhu-1 reviewed Oct 26, 2022
View reviewed changes
scripts/install-helm.sh Outdated
@@ -179,7 +179,7 @@ install_cephcsi_helm_charts() {
kubectl_retry delete cm ceph-config --namespace ${NAMESPACE}

# shellcheck disable=SC2086
"${HELM}" install --namespace ${NAMESPACE} --set provisioner.fullnameOverride=csi-rbdplugin-provisioner --set nodeplugin.fullnameOverride=csi-rbdplugin --set configMapName=ceph-csi-config --set provisioner.replicaCount=1 ${SET_SC_TEMPLATE_VALUES} ${RBD_SECRET_TEMPLATE_VALUES} ${RBD_CHART_NAME} "${SCRIPT_DIR}"/../charts/ceph-csi-rbd --set topology.enabled=true --set topology.domainLabels="{${NODE_LABEL_REGION},${NODE_LABEL_ZONE}}" --set provisioner.maxSnapshotsOnImage=3 --set provisioner.minSnapshotsOnImage=2
"${HELM}" install --namespace ${NAMESPACE} --set provisioner.fullnameOverride=csi-rbdplugin-provisioner --set nodeplugin.fullnameOverride=csi-rbdplugin --set configMapName=ceph-csi-config --set provisioner.replicaCount=1 --set-json='commonLabels={"test": "test", "un": "deux"}' ${SET_SC_TEMPLATE_VALUES} ${RBD_SECRET_TEMPLATE_VALUES} ${RBD_CHART_NAME} "${SCRIPT_DIR}"/../charts/ceph-csi-rbd --set topology.enabled=true --set topology.domainLabels="{${NODE_LABEL_REGION},${NODE_LABEL_ZONE}}" --set provisioner.maxSnapshotsOnImage=3 --set provisioner.minSnapshotsOnImage=2
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good, can you please add some meaningful names (choose whatever you like something specific to cephcsi)

--set-json='commonLabels={"component": "ceph", "sub-component": "rbd"}'

@bastienbosser bastienbosser force-pushed the feature-possibility-to-add-additional-labels branch from 6322356 to 097f875 Compare October 26, 2022 15:57
@bastienbosser bastienbosser requested review from nixpanic and Madhu-1 and removed request for nixpanic and Madhu-1 October 26, 2022 16:06
Madhu-1
Madhu-1 reviewed Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

changes LGTM, please squash the commits.

humblec
humblec previously requested changes Oct 27, 2022
View reviewed changes
scripts/install-helm.sh Outdated
@@ -179,7 +179,7 @@ install_cephcsi_helm_charts() {
kubectl_retry delete cm ceph-config --namespace ${NAMESPACE}

# shellcheck disable=SC2086
"${HELM}" install --namespace ${NAMESPACE} --set provisioner.fullnameOverride=csi-rbdplugin-provisioner --set nodeplugin.fullnameOverride=csi-rbdplugin --set configMapName=ceph-csi-config --set provisioner.replicaCount=1 ${SET_SC_TEMPLATE_VALUES} ${RBD_SECRET_TEMPLATE_VALUES} ${RBD_CHART_NAME} "${SCRIPT_DIR}"/../charts/ceph-csi-rbd --set topology.enabled=true --set topology.domainLabels="{${NODE_LABEL_REGION},${NODE_LABEL_ZONE}}" --set provisioner.maxSnapshotsOnImage=3 --set provisioner.minSnapshotsOnImage=2
"${HELM}" install --namespace ${NAMESPACE} --set provisioner.fullnameOverride=csi-rbdplugin-provisioner --set nodeplugin.fullnameOverride=csi-rbdplugin --set configMapName=ceph-csi-config --set provisioner.replicaCount=1 --set-json='commonLabels={"app.kubernetes.io/name": "ceph-csi-rdb", "app.kubernetes.io/managed-by": "helm"}' ${SET_SC_TEMPLATE_VALUES} ${RBD_SECRET_TEMPLATE_VALUES} ${RBD_CHART_NAME} "${SCRIPT_DIR}"/../charts/ceph-csi-rbd --set topology.enabled=true --set topology.domainLabels="{${NODE_LABEL_REGION},${NODE_LABEL_ZONE}}" --set provisioner.maxSnapshotsOnImage=3 --set provisioner.minSnapshotsOnImage=2
Copy link
Collaborator

Choose a reason for hiding this comment

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

change below to "app.kubernetes.io/name": "ceph-csi-rdb" to "app.kubernetes.io/name": "ceph-csi-rbd"

Copy link
Author

Choose a reason for hiding this comment

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

Done

@bastienbosser
Copy link
Author

changes LGTM, please squash the commits.

Normally you can do this directly during the merge @Madhu-1

@Rakshith-R
Copy link
Contributor

changes LGTM, please squash the commits.

Normally you can do this directly during the merge @Madhu-1

@bastienbosser
This is currently automated in cephcsi repo.
merigfyio bot will be merging this pr with rebase and merge logic after approvals and successful ci runs.
It'll be cleaner to have one commit for this pr.

Thanks !

@bastienbosser bastienbosser force-pushed the feature-possibility-to-add-additional-labels branch from 4f0da28 to 8a9d6a9 Compare October 27, 2022 14:15
@bastienbosser
Copy link
Author

changes LGTM, please squash the commits.

Normally you can do this directly during the merge @Madhu-1

@bastienbosser This is currently automated in cephcsi repo. merigfyio bot will be merging this pr with rebase and merge logic after approvals and successful ci runs. It'll be cleaner to have one commit for this pr.

Thanks !

Done

@Madhu-1 Madhu-1 requested a review from nixpanic October 27, 2022 14:56
Madhu-1
Madhu-1 previously approved these changes Oct 27, 2022
View reviewed changes
nixpanic
nixpanic previously approved these changes Oct 28, 2022
View reviewed changes
@nixpanic
Copy link
Member

After other PRs are done with their CI runs, this can be rebased by Mergify and de ok-to-test label can be added

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Oct 28, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Oct 28, 2022

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the feature-possibility-to-add-additional-labels branch from 8a9d6a9 to ca00435 Compare October 28, 2022 19:50
@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Oct 28, 2022
@nixpanic nixpanic removed the ok-to-test Label to trigger E2E tests label Nov 1, 2022
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 1, 2022

It seems the deploying through Helm is not working. All CI jobs fail in a similar way:

Snippet:

Fri Oct 28 20:20:37 UTC 2022: 'kubectl_retry label node/minikube test.failure-domain/zone=testzone' done (ret=0)
node/minikube labeled
Error: unknown flag: --set-json
...

Maybe --set-json is something of a newer Helm version? You can change the Helm version in the build.env file.

Yes, because of the missing flag, deployment was not successful and csi pods are not running as expected. Can you please use some other existing flag to set it or we need to update the helm version in the build.env?

@bastienbosser bastienbosser force-pushed the feature-possibility-to-add-additional-labels branch from ca00435 to 7fb0387 Compare November 2, 2022 07:07
@mergify mergify bot dismissed stale reviews from Madhu-1 and nixpanic November 2, 2022 07:07

Pull request has been modified.

@bastienbosser
Copy link
Author

I changed the helm version in the build.env file to make the tests work.

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 2, 2022

I changed the helm version in the build.env file to make the tests work.

How this value can be set if someone is using an older helm version? Do you have the sample command?

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 2, 2022

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

1 similar comment
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 2, 2022

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

@nixpanic
Copy link
Member

nixpanic commented Nov 2, 2022

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

Seems to run well now!

@nixpanic
Copy link
Member

nixpanic commented Nov 2, 2022

@Mergifyio rebase

@mergify
deploy: add commonLabels value
e690c63 
Signed-off-by: BOSSER, Bastien <bastien.bosser@atos.net>
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the feature-possibility-to-add-additional-labels branch from 7fb0387 to e690c63 Compare November 2, 2022 09:49
@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Nov 2, 2022
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

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

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/upgrade-tests-rbd

@mergify mergify bot merged commit dea07aa into ceph:devel Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@humblec humblec humblec approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

Assignees
No one assigned
Labels
component/deployment Helm chart, kubernetes templates and configuration Issues/PRs ok-to-test Label to trigger E2E tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possibility to add additional labels on all helm chart resources
5 participants
@bastienbosser @Madhu-1 @Rakshith-R @nixpanic @humblec