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

Feat/valkey support #597

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Feat/valkey support #597

merged 5 commits into from
Oct 9, 2024

Conversation

mfocko
Copy link
Member

@mfocko mfocko commented Sep 20, 2024

  • Deployed on both stage and prod… :PepeLaugh:

    Friday evening deployments hit different…

Related to packit/packit-service#2522

@mfocko mfocko self-assigned this Sep 20, 2024
Copy link
Contributor

@mfocko mfocko force-pushed the feat/valkey-support branch from 6d6d60a to 4d0d159 Compare October 1, 2024 07:50
Copy link
Contributor

@mfocko mfocko force-pushed the feat/valkey-support branch from 4d0d159 to 713d64c Compare October 2, 2024 07:22
Copy link
Contributor

@mfocko mfocko force-pushed the feat/valkey-support branch from 713d64c to f153287 Compare October 3, 2024 07:39
@mfocko mfocko requested a review from majamassarini October 3, 2024 07:39
Copy link
Contributor

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/3033f96778014bde8e293da08f162b3f

pre-commit NODE_FAILURE Node request 200-0007605115 failed in 0s

Copy link
Contributor

Copy link
Contributor

mfocko and others added 4 commits October 8, 2024 13:09
1. Rely on a single tag ‹with_kv_database› for deciding whether it
   should be deployed or not.
2. Introduce a new variable ‹kv_database› that holds the name of the KV
   database that might be deployed.
   - Also allows for easier deduction of the hostname and makes sure
     that there's always at least one deployment of any kind, i.e., it
     is now not possible to require it present, but have incorrect
     combination of flags, e.g., neither ‹with_redis› nor ‹with_redict›
     set.
3. Add it to templates where it's been forgotten.

Signed-off-by: Matej Focko <[email protected]>
The leaks in short-running pods result in idle connections
to Redict/Valkey, all of these KV-databases have ‹timeout› option in
their config that allows for iterative cleanup of hanging connections.

This mitigates the issue to the point of still having free connections
slots to Redict/Valkey, i.e., the pods shall be killed, but handlers do
not end up in a retry-loop trying to connect to Redict/Valkey.

Since the config is 1:1 between all Redis, Redict, and Valkey, create
one ConfigMap, map the config into the databases and pass the path
to the config as an argument.

Tested with Redict and Valkey. »NOT« tested with Redis.

Related to packit/packit-service#2522

Signed-off-by: Matej Focko <[email protected]>
@mfocko mfocko force-pushed the feat/valkey-support branch from c000b31 to 64cdcec Compare October 8, 2024 11:09
Copy link
Contributor

redis: "{{ with_redis }}"
redict: "{{ with_redict }}"
redis: "{{ with_kv_database and kv_database == 'redis' }}"
redict: "{{ with_kv_database and kv_database == 'redict' }}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
redict: "{{ with_kv_database and kv_database == 'redict' }}"
redict: "{{ with_kv_database and kv_database == 'redict' }}"
valkey: "{{ with_kv_database and kv_database == 'valkey' }}"

Not sure why we need this facts but isn't valkey missing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, must've missed it.

This builds up the optional_deploymentconfigs, from that we build the deploymentconfigs and finally it's checked here

- name: Wait for deploymentconfig rollouts to complete
# timeout 15min to not wait indefinitely in case of a problem
ansible.builtin.command: timeout 15m oc rollout status -w deploy/{{ item }}
register: oc_rollout_status
changed_when: false
failed_when: '"successfully rolled out" not in oc_rollout_status.stdout'
loop: "{{ deploymentconfigs }}"

that they're running. It's a bit flaky though, cause it doesn't work properly if you don't run oc project ‹packit namespace› beforehand in the terminal.

Copy link
Member

@majamassarini majamassarini left a comment

Choose a reason for hiding this comment

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

🙏🏻

vars/packit/dev_template.yml Outdated Show resolved Hide resolved
vars/packit/prod_template.yml Outdated Show resolved Hide resolved
vars/packit/stg_template.yml Outdated Show resolved Hide resolved
playbooks/roles/deploy/tasks/main.yml Outdated Show resolved Hide resolved
playbooks/roles/deploy/tasks/main.yml Outdated Show resolved Hide resolved
playbooks/deploy.yml Outdated Show resolved Hide resolved
playbooks/deploy.yml Outdated Show resolved Hide resolved
@mfocko mfocko force-pushed the feat/valkey-support branch from 0dc44f5 to 9bb28fb Compare October 9, 2024 07:55
Copy link
Contributor

- Remove tags for redis and redict, they would not be very helpful,
  since the specific implementation of KV-DB is taken from the variables
  anyways
- Add missing Valkey to the deploymentconfigs
- Add comment with options to the deployment templates

Co-authored-by: Maja Massarini <[email protected]>
Signed-off-by: Matej Focko <[email protected]>
@mfocko mfocko force-pushed the feat/valkey-support branch from 9bb28fb to 660a23a Compare October 9, 2024 08:05
Copy link
Contributor

Copy link
Member

@majamassarini majamassarini left a comment

Choose a reason for hiding this comment

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

Thanks!

@mfocko mfocko merged commit c911c4f into packit:main Oct 9, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants