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

Replace ip_version check with ansible test #7418

Merged
merged 1 commit into from Apr 24, 2023
Merged

Conversation

xbezdick
Copy link
Contributor

Instead of checking ip_version variable we should check the input address for ip version and select code path based on that.

This solves ceph adoption with mixed ipv6 and ipv4 networks.

Resolves: rhbz#2186226

@fultonj
Copy link
Contributor

fultonj commented Apr 19, 2023

LGTM

fmount
fmount previously approved these changes Apr 19, 2023
Copy link
Contributor

@fmount fmount left a comment

Choose a reason for hiding this comment

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

Thanks

@fmount
Copy link
Contributor

fmount commented Apr 19, 2023

@guits can you review this patch when you have the chance?

@asm0deuz
Copy link
Collaborator

@xbezdick why not set ip_version to ivp4? cephadm_mgmt_network shouldn't be used in the condition to chose between ipv4 or ipv6

@asm0deuz
Copy link
Collaborator

jenkins test centos-non_container-update

@asm0deuz
Copy link
Collaborator

jenkins test centos-non_container-all_daemons

@asm0deuz
Copy link
Collaborator

jenkins test centos-non_container-switch_to_containers

@asm0deuz
Copy link
Collaborator

jenkins test centos-non_container-purge

guits
guits previously approved these changes Apr 20, 2023
Copy link
Collaborator

@guits guits left a comment

Choose a reason for hiding this comment

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

that looks good to me, I don't see any issue with this patch.
I let @asm0deuz take a final look

@asm0deuz
Copy link
Collaborator

jenkins test centos-non_container-all_daemons

@asm0deuz
Copy link
Collaborator

@xbezdick LGTM as well but could you sign-off your commit? Thx

@asm0deuz asm0deuz changed the title Replace ip_version check with ansible test [skip ci] Replace ip_version check with ansible test Apr 24, 2023
Instead of checking ip_version variable we should check the input
address for ip version and select code path based on that.

This solves ceph adoption with mixed ipv6 and ipv4 networks.

Resolves: rhbz#2186226
Signed-off-by: Lukas Bezdicka <lbezdick@redhat.com>
@xbezdick xbezdick dismissed stale reviews from guits and fmount via d065067 April 24, 2023 11:55
@asm0deuz asm0deuz changed the title [skip ci] Replace ip_version check with ansible test Replace ip_version check with ansible test Apr 24, 2023
@ktdreyer
Copy link
Member

As we found in rhbz#2207872, now that we use ansible-core from RHEL 8, we should track each new Ansible community module dependency in ceph-ansible.spec.in.

@ktdreyer
Copy link
Member

It's actually even more complicated. ceph-ansible still uses Ansible 2.9 downstream. Did you develop this and test it with Ansible 2.14?

@ktdreyer
Copy link
Member

It's actually even more complicated. ceph-ansible still uses Ansible 2.9 downstream. Did you develop this and test it with Ansible 2.14?

I see now, when I install the ansible-collection-ansible-utils RPM, it works. So we need to add this to ceph-ansible.spec.in.

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

Successfully merging this pull request may close these issues.

None yet

6 participants