Fix rbd pools trash purge scheduling #2357
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks for the PR! ❤️ |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c8ba8105a4a94ca8954091b9cd82167d ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 46m 40s |
This patch ensures to use the right conditions and parameters while trash purge scheduling on the rbd pools. Jira: https://issues.redhat.com/browse/OSPRH-8504
katarimanojk
force-pushed
the
purge_scheduler_task
branch
from
cc42afd
to
d8cee74
Compare
| - cifmw_cephadm_enable_trash_scheduler | default(false) | ||
| - cifmw_cephadm_pools is defined | ||
| - cifmw_cephadm_pools | length > 0 |
There was a problem hiding this comment.
do we need a | default([]) here ?
There was a problem hiding this comment.
I think these checks make | default([]) unnecessary. I think we specifically want to avoid this block if there are no cifmw_cephadm_pools so I think the update of the when is fine. I assume you were just proposing some alternative method for the when and not pointing out a logic problem. If there's a logic problem, then please elaborate.
| changed_when: false | ||
| become: true | ||
| loop: "{{ [ cinder_pool.name | default('volumes') ] + cinder_pool.cinder_extra_pools | default([]) }}" | ||
| loop: "{{ cifmw_cephadm_pools | default([]) }}" |
There was a problem hiding this comment.
Is the goal to set the rbd trash purge scheduler to all the ceph pools w/ an rbd application set? Or the goal was to enabled it only for cinder pools?
There was a problem hiding this comment.
you are right, i think goal here is set it only for cinder pools
in tripleo, cinder_pool was defined in ceph_pools like this
ceph_pools:
cinder_pool:
name: 'volumes'
enabled: true
cinder_extra_pools: [altrbd, pool2, pool3]
in cifmw , we have
cifmw_cephadm_pools:
- name: volumes
pg_autoscale_mode: True
target_size_ratio: 0.3
application: rbd
@fultonj
so we have only one pool volumes for cinder, should we set the rbd trash purge scheduler only for that pool and remove the loop ?
There was a problem hiding this comment.
I would only set for volumes if is part of the cifmw_cephadm_pools (this should be the when condition). Right now this is wrong because you're setting the trash scheduler for all pools, and I don't think this was the original goal.
| block: | ||
| - name: Get the RBD ceph_cli | ||
| ansible.builtin.include_tasks: ceph_cli.yml | ||
| vars: | ||
| ceph_command: rbd | ||
|
|
||
| - name: Set trash interval | ||
| when: item.application == 'rbd' |
There was a problem hiding this comment.
This seems fragile to me, because if by any chance application is not set in the data structure, this task fails w/ application not defined.
am I right? @katarimanojk not sure you can double check this.
There was a problem hiding this comment.
I think most of the time people will use the hard coded cifmw_cephadm_pools and I think @katarimanojk was trying to set it for all RBD and exclude it for cephfs. However, I agree with @fmount that only want to set it for cinder, which will be called volumes. Unless someone overrides it, but in that case we'll have no way knowing which is the cinder volume unless we have some other sort of label. We could introduce a separate variable like volumes_with_trash_purge and default it to ['volumes']. That at least makes a clear interface and allows the user to override which volumes will have it set. I'm open to other methods.
https://github.com/openstack-k8s-operators/ci-framework/blob/main/playbooks/ceph.yml#L273-L297
There was a problem hiding this comment.
Yeah, that solution is feasible but more complicated (I'm thinking for long term maintenance).
I was thinking more about something like "tagging" the pool where we want to enable trashing:
cifmw_cephadm_pools:
name: volumes
pg_autoscale_mode: True
target_size_ratio: 0.3
application: rbd
trash_enabled: True
so we can keep iterating over the data structure, and enable the trash purge scheduler only for pools where we passed that trash_enabled boolean (that can be |default(false) for a general use case).
This should be simple enough and make that code less error prone.
There was a problem hiding this comment.
I like your proposal of the tag more. Let's go for that.
There was a problem hiding this comment.
@fmount Thanks for your suggestion, i will update the code accordingly.
This patch ensures to use the right conditions
and parameters while trash purge scheduling on
the rbd pools.
Jira: https://issues.redhat.com/browse/OSPRH-8504