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: add support for kubernetes workload maintenance #77

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jon4hz
Copy link
Member

@jon4hz jon4hz commented Oct 25, 2024

Hi,

This PR adds basic support for maintenance on kubernetes workloads

@jon4hz jon4hz force-pushed the feat-maintenance-63-k8s-workload branch from b0b255a to 4d8f659 Compare October 25, 2024 18:57
@jon4hz jon4hz force-pushed the feat-maintenance-63-k8s-workload branch from 4d8f659 to 53f302d Compare October 28, 2024 17:38
@jon4hz jon4hz requested a review from s3lph October 28, 2024 17:40
@jon4hz jon4hz marked this pull request as ready for review October 28, 2024 19:46
@SaschaD-Adfinis
Copy link
Contributor

@hairmare @eyenx Can you provide a review here?
I am not confident to say if this is how maintenance on k8s should look like ^^

@@ -0,0 +1 @@
# Ansible Role adfinis.maintenance.maintenance_63_kubernetes_workload
Copy link

Choose a reason for hiding this comment

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

no docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

well... none of the other roles have any docs in their README, so I didn't really want to break the pattern :)

@@ -0,0 +1,181 @@
---

- name: This task only serves as a template for the tasks below
Copy link

Choose a reason for hiding this comment

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

what the ansible?

are we sure there isn't an idiomatic way to do this?

e.g. if it's just about the when: bit, why not put it into a block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing in this collection is idiomatic, because it's using Ansible for something that Ansible shouldn't be used for. See https://github.com/adfinis/ansible-collection-maintenance/blob/main/README.md?plain=1#L10-L18

Copy link

Choose a reason for hiding this comment

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

oh my

Copy link

Choose a reason for hiding this comment

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

can you point me towards some documentation on the reasoning for using anchors and aliases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is any documentation regarding this. From what I saw, it's important that every task has an ID and name as variable because the callback plugin expects this:

taskid = result._task.vars.get('taskid')
and the anchors just provide some reusability for the actual task name and a simple when condition so you can skip each task based on it's ID.

And generally speaking, I just followed the pattern from the other roles. It probably isn't very idiomatic, but as @s3lph already said, nothing in this collection is really idiomatic...

@@ -0,0 +1,74 @@
from datetime import datetime

class FilterModule(object):
Copy link

Choose a reason for hiding this comment

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

did you consider testing this with pytest-ansible?

@eyenx
Copy link
Member

eyenx commented Jan 10, 2025

Can u tell me the reasoning behind writing this? Apparently to run it when maintenance is done in k8s.

I am asking this dumb question as most if not all of the checks kinda are taken care of already by proper alerting or proper configuration with gitops. Checking them with ansible feels redundant and prone to give a false sense of security as it would be a whole hell of work to cover all the possible mishap in Configuration that are possible with k8s.

@jon4hz
Copy link
Member Author

jon4hz commented Jan 10, 2025

Can u tell me the reasoning behind writing this? Apparently to run it when maintenance is done in k8s.

I am asking this dumb question as most if not all of the checks kinda are taken care of already by proper alerting or proper configuration with gitops. Checking them with ansible feels redundant and prone to give a false sense of security as it would be a whole hell of work to cover all the possible mishap in Configuration that are possible with k8s.

Ever since the AKS takeover, internal IT is given regular maintenance tickets with a checklist to work through. The goal of this PR is to automate as many of those checks as possible. Of course, we won't be able to check for all possible misconfigurations, but some specific things like checking for a specific PullPolicy can be covered pretty well and save you some time when doing maintenance.

And yes, some checks can also be covered by looking at the monitoring, but I think it doesn't hurt to double-check them. Especially because ansible doesn't rely on a proper monitoring setup but communicates with the k8s api directly.

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.

5 participants