Skip to content

Conversation

cyrinux
Copy link
Contributor

@cyrinux cyrinux commented Dec 5, 2024

Hi,

First thanks, I was tired of redis-sentinel tilt issue. 😸

I saw a recent commit 8cc255a about ipv6 fix but turn out it was not enought. The root issue was that the label with ipv6 ip cannot be write.

This goal of this PR is to fix IPV6 support, I did it on an ipv6 only cluster. I battle all the evening, but it's now working.

This is a breaking change for now, as I have to switch from label for the masterIP to annotations as we can't store []:, so an ipv6 in label. Ipv6 or not, store the ip as annotation looks the good way to do.

I'm not sure about the "migration" process:

  • I can then add a compatibility mode keeping and testing the masterIP label, but for the next release we will need to remove it.
  • Or we just keep the code simple and bump a major with breaking change alert.

Here a sample I use:

apiVersion: dragonflydb.io/v1alpha1
kind: Dragonfly
metadata:
  labels:
    app.kubernetes.io/created-by: dragonfly-operator
    app.kubernetes.io/instance: dragonfly-test
    app.kubernetes.io/managed-by: kustomize
    app.kubernetes.io/name: dragonfly
    app.kubernetes.io/part-of: dragonfly-operator
  name: dragonfly-test
  namespace: default
spec:
  args:
  - '--bind=::'
  - '--admin_bind=::'
  image: docker.dragonflydb.io/dragonflydb/dragonfly:v1.25.4
  replicas: 2
  resources:
    limits:
      cpu: 600m
      memory: 384Mi
    requests:
      cpu: 500m
      memory: 256Mi

I would be happy to see this merge. Is it ok for you ?

@cyrinux cyrinux marked this pull request as draft December 5, 2024 18:59
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 6 times, most recently from 716ca9a to 51e573d Compare December 5, 2024 23:03
@cyrinux cyrinux marked this pull request as ready for review December 5, 2024 23:13
@cyrinux cyrinux changed the title chore: try to workaround master label with ipv6 issue fix: ipv6 support Dec 5, 2024
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 6 times, most recently from 3840eee to 26578e3 Compare December 5, 2024 23:57
@cyrinux cyrinux changed the title fix: ipv6 support fix(operator): ipv6 support Dec 6, 2024
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 3 times, most recently from 1e6d949 to 2afc70f Compare December 6, 2024 00:08
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

Thanks for all the work!

The masterIP label is used to match if the dragonfly replica state matches with what was configured by the Operator. There should not be any issue in moving this to an annotation (and it makes sense).

The compatibility story is important for sure, My preference would be to do them both for now i.e set as label (if ipv4 only) and annotation (for both ipv4 and ipv6) for a version and remove the label part permanently in future versions. Wdyt?

Also, You should be setting/and using the masterIP here at

if masterIp != redisMasterIp && masterIp != pod.Labels[resources.MasterIp] {

@cyrinux
Copy link
Contributor Author

cyrinux commented Dec 6, 2024

Thanks for all the work!

The masterIP label is used to match if the dragonfly replica state matches with what was configured by the Operator. There should not be any issue in moving this to an annotation (and it makes sense).

The compatibility story is important for sure, My preference would be to do them both for now i.e set as label (if ipv4 only) and annotation (for both ipv4 and ipv6) for a version and remove the label part permanently in future versions. Wdyt?

Also, You should be setting/and using the masterIP here at

if masterIp != redisMasterIp && masterIp != pod.Labels[resources.MasterIp] {

This is OK for me, I will do this :-)

@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch from 6ece1a0 to cf485c9 Compare December 6, 2024 08:44
@cyrinux
Copy link
Contributor Author

cyrinux commented Dec 6, 2024

I'm ready for another review @Pothulapati 😃

@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 4 times, most recently from ab33a68 to 7516283 Compare December 6, 2024 10:10
@cyrinux
Copy link
Contributor Author

cyrinux commented Dec 6, 2024

I add another commit to bind by default on dualstack, this should not hurt.

@cyrinux cyrinux requested a review from Pothulapati December 6, 2024 10:15
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch from 7516283 to a101fd0 Compare December 7, 2024 21:52
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 2 times, most recently from c572750 to cf69781 Compare December 8, 2024 19:12
@cyrinux
Copy link
Contributor Author

cyrinux commented Dec 11, 2024

@Pothulapati should I be worried about the CI state ? Can you please re-run it for me or have idea about what happen ? Or all is good ? 😄

For info this PR is running or my ipv6 only production since few days without issues 👍🏻

@cyrinux
Copy link
Contributor Author

cyrinux commented Feb 11, 2025

Hi @Pothulapati can you help me with this PR please ? 🙏🏻

@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch 3 times, most recently from 2abe711 to 43f2362 Compare March 25, 2025 14:19
@cyrinux
Copy link
Contributor Author

cyrinux commented Mar 25, 2025

Hi, anybody can help me passing this PR ? Its running without issue in my prod since weeks.

@Ais8Ooz8 Ais8Ooz8 mentioned this pull request Apr 4, 2025
@Ais8Ooz8
Copy link

Ais8Ooz8 commented Apr 4, 2025

@Pothulapati @Abhra303 Up

@Abhra303
Copy link
Contributor

Abhra303 commented Apr 4, 2025

I will check

@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch from 6944360 to 13dddfe Compare April 5, 2025 09:24
@Ais8Ooz8
Copy link

Up

@Ais8Ooz8
Copy link

@Abhra303 @Pothulapati The operator does not work in IPv6 clusters, maybe it's time to look at this PR?

@Ais8Ooz8
Copy link

Or make it clear that the project has been abandoned

@cyrinux
Copy link
Contributor Author

cyrinux commented Jun 17, 2025

Hi here,

@Abhra303 @Pothulapati can I somehow help to get this PR merged ? Do you need something (but time) ?

cyrinux added 5 commits August 8, 2025 13:21
- Add IPv6 support with sanitizeIp function for proper address handling
- Update MasterIp from label to annotation for better metadata management
- Use net.JoinHostPort for IPv6-compatible address formatting
- Improve logging messages with proper capitalization
- Add getMasterIp function that checks annotations first
- Update Role constant references throughout codebase
both operator and dragonfly, should not hurt.
- Fix remaining references in dragonfly_controller_test.go
- Fix remaining references in dragonfly_pod_lifecycle_controller_test.go
- Ensures all tests work with the updated constant names
@cyrinux cyrinux force-pushed the fix/ipv6-labels-fun branch from d08c610 to 2ed9c2e Compare August 8, 2025 11:51
@cyrinux
Copy link
Contributor Author

cyrinux commented Aug 8, 2025

Hi,
To let you know I just finally rebase, and I use it, with dragonfly 1.32 on ipv6. All good.
Can you help me getting merged this one ?
Thank you

@Abhra303
Copy link
Contributor

Hi, sorry for not responding to the PR. Taking a look now.

@Abhra303 Abhra303 requested review from Abhra303 and removed request for Pothulapati September 10, 2025 07:19
@dhess
Copy link

dhess commented Sep 25, 2025

Hi, what's the status on this PR? It's over 9 months old now and the operator still doesn't properly support IPv6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants