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

Dont run client dummy container on ppc64le hosts #3084

Merged
merged 1 commit into from Aug 31, 2018

Conversation

andymcc
Copy link
Contributor

@andymcc andymcc commented Aug 30, 2018

The dummy client container currently wont work on ppc64le hosts.
This PR creates a filtered client group that contains only hosts
that are not ppc64le - which can then be the group to run the
dummy container against.

This is for the specific case of a containerized_deployment where
there is a mixture of non-ppc64le hosts and ppc64le hosts.
Currently ppc64le is not supported with Ceph server components.

Signed-off-by: Andy McCrae andy.mccrae@gmail.com

@tbreeds
Copy link

tbreeds commented Aug 31, 2018

RHBZ#1624216

@tbreeds
Copy link

tbreeds commented Aug 31, 2018

Works for me!

groups: _filtered_clients
with_items: "{{ groups[client_group_name] }}"
when:
- hostvars[item]['ansible_architecture'] != 'ppc64le'
Copy link

Choose a reason for hiding this comment

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

I wonder if it's worth expressing this as:

- hostvars[item]['ansible_architecture'] == 'x86_64'

that way if someone tries to do the client setup on aarch64 they don't run into the same issue.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@andymcc
Copy link
Contributor Author

andymcc commented Aug 31, 2018

Works for me, I can add in to specify only x86_64 instead.
I do wonder though, is that container required at all? E.g. for clients, why would we need a dummy container (that will be removed) to run client commands, when the clients should have been setup when ceph-common is installed?

The dummy client container currently wont work on non-x86_64 hosts.
This PR creates a filtered client group that contains only hosts
that are x86_64 - which can then be the group to run the
dummy container against.

This is for the specific case of a containerized_deployment where
there is a mixture of non-x86_64 hosts and x86_64 hosts. As such
the filtered group will contain all hosts when running with
containerized_deployment: false.

Currently ppc64le is not supported for Ceph server components.

Signed-off-by: Andy McCrae <andy.mccrae@gmail.com>
@leseb
Copy link
Member

leseb commented Aug 31, 2018

@andymcc typically in a fully containerized env, clients do not have any package manager. So to get the binary they need a container.

@andymcc
Copy link
Contributor Author

andymcc commented Aug 31, 2018

Ok fixed that up - I made one additional change, which is that if you are running iwith containerized_deployment: false - it won't try filter the list and will instead just add all hosts in to the filtered group.

That'll mean a use-case where you have ppc64le clients only - but are not running in containerized_deployment mode, it wont filter those hosts out and will run normally.

@andymcc
Copy link
Contributor Author

andymcc commented Aug 31, 2018

@leseb - ahh ok - and there isn't a container that is just the client binaries?
I'm a bit confused by the fact its a short lived container though - or rather, what does a client look like in a containerized_deployment that we would need to setup a temporary container to do client actions.

groups: _filtered_clients
with_items: "{{ groups[client_group_name] }}"
when:
- (hostvars[item]['ansible_architecture'] == 'x86_64') or (not containerized_deployment)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be?

when:
  - containerized_deployment
  - hostvars[item]['ansible_architecture'] == 'x86_64'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leseb - i think that would create a situation where on containerized_deployment: false that the filtered_clients group contains no hosts, we want the opposite which is when containerized_deployment is false or its an x86_64 host, then add it to the group.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, can't we then do something like groups.get('_filtered_clients', 'client_group_name')? So if _filtered_clients does not exist we use client_group_name?

Copy link
Contributor Author

@andymcc andymcc Aug 31, 2018

Choose a reason for hiding this comment

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

We could - but it makes line 130 difficult because we reference host [0] in the group, rather than using groups.get.

If it's not containerized_deployment groups['_filtered_clients'] would be the same as groups[client_group_name] - so im not sure its necessary.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, your approach is much easier in the end.

@leseb
Copy link
Member

leseb commented Aug 31, 2018

@andymcc no dedicated container for the binaries only, just the same one. This was a quick and dirty solution which I hope will disappear for 3.2. Actually, 2 PRs have code to remove that dependency already, I just need to resurrect them :).

@andymcc
Copy link
Contributor Author

andymcc commented Aug 31, 2018

@leseb - ahh nice! That makes sense. Thanks for the update, I figured it wasn't a permanent fix.

@mergify mergify bot merged commit 772e6b9 into ceph:master Aug 31, 2018
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

3 participants