Adding loki and minio roles#3793
Adding loki and minio roles#3793lnatapov wants to merge 2 commits intoopenstack-k8s-operators:mainfrom
Conversation
|
Hi @lnatapov. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
c27b274 to
234412e
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/31a8ffddf00f47b0b5c670e611539d7d ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 02m 36s |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
92166c4 to
dad3cd0
Compare
283cab5 to
f950d83
Compare
michburk
left a comment
There was a problem hiding this comment.
Would it be possible to use kubernetes.core.k8s and kubernetes.core.k8s_info in place of running oc commands through anisble.builtin.shell/ansible.builtin.command?
Additionally, README files for each role would help explain the usage and purpose of each role. In particular, explaining how the deploy_loki role can deploy minio itself, and expectations around which vars are passed/set and where when using pre-deployed minio vs having the deploy_loki role deploy minio.
roles/deploy_minio/tasks/main.yml
Outdated
| - name: Render MinIO manifests for CloudKitty / Loki | ||
| ansible.builtin.template: | ||
| src: minio_for_ck.yaml.j2 |
There was a problem hiding this comment.
same. I think we should rename it to not reference CloudKitty or Loki
| - /bin/bash | ||
| - -c | ||
| - | | ||
| mkdir -p /data/loki && \ |
There was a problem hiding this comment.
I think there should be a parameter to the role with a list of buckets to be created. could default to loki for now, or already create one we then use for backup, like [loki, velero] or [loki, backup]
| name: {{ cifmw_deploy_minio_namespace }} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: Pod |
There was a problem hiding this comment.
I think we should use a deployment so the pod gets re-created when terminate for some reason
| weight: 100 | ||
| port: | ||
| targetPort: console | ||
| wildcardPolicy: None |
There was a problem hiding this comment.
maybe add tls?
tls:
termination: edge
insecureEdgeTerminationPolicy: Redirect
| weight: 100 | ||
| port: | ||
| targetPort: api | ||
| wildcardPolicy: None |
There was a problem hiding this comment.
same, maybe add tls?
tls:
termination: edge
insecureEdgeTerminationPolicy: Redirect
|
We need to get this merged as soon as possible, because openstack-k8s-operators/architecture#721 was not given a "depends-on" pointing to this PR (or an equivalent |
f950d83 to
f4eff0a
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0fbd359b8d334c0587d8e282ff5a393b ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 17m 59s |
e59193e to
8684b5d
Compare
Add deploy_loki and deploy_minio Ansible roles, the deploy-loki-for-ck hook playbook, and ci/config fragment so cifmw-molecule-deploy_loki runs when roles/deploy_minio changes. Regenerate zuul.d/molecule.yaml with the role_molecule script. Signed-off-by: Leonid Natapov <lnatapov@redhat.com> Made-with: Cursor
8684b5d to
475d513
Compare
| - name: Deploy MinIO for CloudKitty / Loki object storage | ||
| ansible.builtin.import_role: | ||
| name: deploy_minio | ||
| vars: | ||
| cifmw_deploy_minio_namespace: "{{ cifmw_deploy_loki_minio_namespace }}" | ||
| cifmw_deploy_minio_root_user: "{{ cifmw_deploy_loki_minio_access_key }}" | ||
| cifmw_deploy_minio_root_password: "{{ cifmw_deploy_loki_minio_secret_key }}" |
There was a problem hiding this comment.
If you're going to enable deploying minio from within the deploy_loki role, it would make sense to avoid an explicit call to deploy_minio here, and instead call deploy_loki without supplying cifmw_deploy_loki_deploy_minio: false.
This way you can avoid referencing the cifmw_deploy_loki_minio_* default values from outside of the deploy_loki role.
If you want to deploy minio and loki separately, you should define some other variables and pass both the deploy loki and deploy minio roles these values, ie:
# ... assume some task(s) or a `vars` block on the play these tasks are in
# defines the _example_minio_* vars here
- name: deploy minio
vars:
cifmw_deploy_minio_namespace: "{{ _example_minio_namespace }}"
cifmw_deploy_minio_root_user: "{{ _example_minio_user }}"
cifmw_deploy_minio_root_password: "{{ _example_minio_password }}"
ansible.builtin.import_role:
name: deploy_minio
# whatever arbitrary other tasks can be here
- name: deploy loki
vars:
cifmw_deploy_loki_minio_namespace: "{{ _example_minio_namespace }}"
cifmw_deploy_loki_minio_access_key: "{{ _example_minio_user }}"
cifmw_deploy_loki_minio_secret_key: "{{ _example_minio_password }}"
cifmw_deploy_loki_deploy_minio: false
ansible.builtin.import_role:
name: deploy_loki
This way both roles clearly share the same source of truth without needing to reference/interfere with default variables from each other.
| cifmw_deploy_loki_parent_dir: "{{ ansible_user_dir }}/ci-framework-data" | ||
| cifmw_deploy_loki_base_dir: "{{ cifmw_deploy_loki_parent_dir }}" |
There was a problem hiding this comment.
I don't understand the cifmw_deploy_loki_parent_dir variable - wouldn't
cifmw_deploy_loki_base_dir: "{{ cifmw_basedir }}"
be sufficient?
Also this is a small consistency nitpick, but most other *_basedir variables don't include an underscore between base and dir
Regardless, cifmw_deploy_loki_parent_dir shouldn't be defined in group_vars/all.yml
This same comment applies to the cifmw_deploy_minio_parent_dir and cifmw_deploy_minio_base_dir vars
| 3. Applies that manifest with **`kubernetes.core.k8s`**. | ||
| 4. Optionally waits until the **ClusterServiceVersion** for the Loki subscription reports phase **`Succeeded`** (when **`cifmw_deploy_loki_wait_for_csv`** is true). | ||
|
|
||
| If you should not run Loki at all, do **not** import this role—or use **`when:`** on **`import_role` / `include_role`** (see **Skipping the role** below). There is no in-role master toggle. |
There was a problem hiding this comment.
Unnecessary, see below comment.
| ## Skipping the role | ||
|
|
||
| Calling **`import_role: deploy_loki`** or **`include_role: deploy_loki`** is the signal that you want this deployment. To make execution conditional, put **`when`** on that task (or omit the task). Example: | ||
|
|
||
| ```yaml | ||
| - ansible.builtin.import_role: | ||
| name: deploy_loki | ||
| when: my_condition | default(false) | bool | ||
| ``` |
There was a problem hiding this comment.
As far as I'm concerned, this should go without saying. No need to document the fact that if you don't want to deploy loki, you shouldn't be calling the deploy_loki role.
|
(non-blocking) question: could this be splitten in two differents MRs and commits? |
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Adding loki and minio roles