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

make unicast-peer resource-name unique by adding the instance-name #230

Conversation

unki
Copy link

@unki unki commented Jan 27, 2021

Considering the following 3-node-setup:

Node IP
node1 10.0.0.1
node2 10.0.0.2
node3 10.0.0.3

There are two different VIPs bound to two separate VRRP-instances

  • instance1, VIP primarily held by node1
  • instance2, VIP primarily held by node2.

Both instances share the same unicast-peers - like in below config-snippet from node1 (10.0.0.1).

keepalived::vrrp_instance:
  instance1:
    unicast_peers:
      - 10.0.0.2
      - 10.0.0.3
  instance2:
    unicast_peers:
      - 10.0.0.2
      - 10.0.0.3

That was a working config before - but by introducing the keepalived::vrrp::unicast_peer type in #227 this now fails as duplicate keepalived::vrrp::unicast_peer resources would get created with the same resource-names.

Therefor I would like to suggest binding the resource-name to the VRRP-instance.
Either by just looping over $unicast_peer_array like I've done in this PR.
Or in any other way.

@unki unki force-pushed the fix/multiple-vrrp-instance-might-share-the-same-unicast-peers branch from 6ad57c3 to 29ae101 Compare January 27, 2021 18:03
@unki unki force-pushed the fix/multiple-vrrp-instance-might-share-the-same-unicast-peers branch from 29ae101 to 3424ec4 Compare January 27, 2021 18:12
@unki unki force-pushed the fix/multiple-vrrp-instance-might-share-the-same-unicast-peers branch from 3424ec4 to 3950c3c Compare January 27, 2021 18:16
@bastelfreak bastelfreak added the bug Something isn't working label Jan 27, 2021
Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

looks good to me. I will keep this open in case someone else want's to review this and knows more about keepalived

@fklajn
Copy link
Contributor

fklajn commented Apr 20, 2021

Any chance that this will be merged and released soon?

@kenyon
Copy link
Member

kenyon commented Apr 26, 2021

Any chance that this will be merged and released soon?

@fklajn have you tested this change, and does it work for you?

@fklajn
Copy link
Contributor

fklajn commented Apr 26, 2021

Yes, it works for me. I had to fork the repo and for now I am using my own copy with cherry-picked change. It would be nice to have this in upstream.

@unki
Copy link
Author

unki commented Apr 26, 2021

Yes, it works for me. I had to fork the repo and for now I am using my own copy with cherry-picked change. It would be nice to have this in upstream.

Can ack this too. We are using it in prod since I've opened this PR.

@bastelfreak bastelfreak merged commit 24e12a2 into voxpupuli:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants