[bitnami/redis] Ensure that Redis resources are less than 63 chars #34803
Conversation
mboutet
changed the title
|
Testing on my end, but encountering few issues with my changes, so I'm setting the PR as draft for now. EDIT: I think it's good now. |
carrodher
added
verify
|
~Actually, I think there's still an issue with one label having a too long value, but this label comes from the common bitnami chart, so I would have to fix it there :/ ~ It was the value of the |
mboutet
force-pushed
the
truncate-long-redis-name
branch
from
72849ad to
b3ceebb
Compare
mboutet
force-pushed
the
truncate-long-redis-name
branch
from
b3ceebb to
8623aff
Compare
Signed-off-by: Maxence Boutet <[email protected]>
| {{/* | ||
| Return the proper Redis master fullname. | ||
| The 0000000000 is to take into account the 10-charaters hash that the StatefulSet | ||
| appends to the name of the StatefulSet in the pod's controller-revision-hash label. | ||
| A label value can only have 63 characters. | ||
| */}} | ||
| {{- define "redis.master.fullname" -}} | ||
| {{- printf "%s-%s" ((include "common.names.fullname" .) | trunc ((sub 63 ("-master-0000000000" | len)) | int) | trimSuffix "-") "master" -}} | ||
| {{- end -}} | ||
| {{/* | ||
| Return the proper Redis replicas fullname | ||
| */}} | ||
| {{- define "redis.replicas.fullname" -}} | ||
| {{- printf "%s-%s" ((include "common.names.fullname" .) | trunc ((sub 63 ("-replicas-0000000000" | len)) | int) | trimSuffix "-") "replicas" -}} | ||
| {{- end -}} | ||
| {{/* | ||
| Return the proper Redis sentinel fullname | ||
| */}} | ||
| {{- define "redis.sentinel.fullname" -}} | ||
| {{- printf "%s-%s" ((include "common.names.fullname" .) | trunc ((sub 63 ("-node-0000000000" | len)) | int) | trimSuffix "-") "node" -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
Since the Service also uses this helper, it could result in it being recreated for some users. Truncating the equivalent of 0000000000 is only strictly needed for the Statefulset in reality.
|
Hi @mboutet, I just updated the thread with more information. Let me know if it's clear now. |
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
Signed-off-by: Maxence Boutet <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
Signed-off-by: Maxence Boutet <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
Signed-off-by: Maxence Boutet <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
Signed-off-by: Bitnami Bot <[email protected]>
|
Sorry @mboutet, I lost track of this ticket. Let me review it again and provide feedback |
jotamartos
left a comment
jotamartos
left a comment
There was a problem hiding this comment.
Thanks for your contribution but your changes return errors when deploying the solution
➜ redis git:(truncate-long-redis-name) helm install redis
Error: INSTALLATION FAILED: parse error at (redis/templates/scripts-configmap.yaml:690): unexpected <template> in command
| A label value can only have 63 characters. | ||
| */}} | ||
| {{- define "redis.master.fullname" -}} | ||
| {{- printf "%s-%s" ((include "common.names.fullname" .) | trunc ((sub 63 ("-master-0000000000" | len)) | int) | trimSuffix "-") "master" -}} |
There was a problem hiding this comment.
Current fullnames do not include the "0000000000" string, so it's not necessary to remove that
➜ redis git:(main) k get all
NAME READY STATUS RESTARTS AGE
pod/redis-master-0 1/1 Running 0 2m29s
pod/redis-replicas-0 1/1 Running 0 2m28s
pod/redis-replicas-1 1/1 Running 0 91s
pod/redis-replicas-2 1/1 Running 0 51s
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE
service/redis-headless ClusterIP None <none> 6379/TCP 2m29s
service/redis-master ClusterIP 10.0.92.51 <none> 6379/TCP 2m29s
service/redis-replicas ClusterIP 10.0.203.103 <none> 6379/TCP 2m29s
NAME READY AGE
statefulset.apps/redis-master 1/1 2m30s
statefulset.apps/redis-replicas 3/3 2m30s
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
|
Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary. |
bitnami-bot
added
stale
4 participants
Description of the change
Make sure to truncate the Statefulsets and Services' name to 63 characters.
Benefits
This guarantees that all the resources are created successfully.
Possible drawback
None, as I made sure to truncate the original
fullnamewhile keeping the differentiating suffix as-is (to prevent resource name clash in case thefullnameis really long).Applicable issues
Additional information
I bumped the major version as it could technically cause resources to be recreated due a change in the
metadata.nameof some Statefulsets and Services. In practice, however, it should not happen as the truncation only happens if really needed, so users already using the chart without issues won't get any truncation. On the other hand, the major bump is also warranted due to the complexity of the chart and the regression risk of this PR even though I tried to not miss anything.Checklist
Chart.yamlaccording to semver. This is not necessary when the changes only affect README.md files.README.mdusing readme-generator-for-helm