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

Add priority class options to high priority pods #862

Merged
merged 4 commits into from
Apr 21, 2022

Conversation

fosterseth
Copy link
Member

@fosterseth fosterseth commented Apr 7, 2022

Adds options for setting the priority class to the control plane and postgres pods
the priorityclasses are presumed to already exist

e.g.

---
apiVersion: awx.ansible.com/v1beta1
kind: AWX
metadata:
  name: awx-demo
spec:
  service_type: nodeport
  control_plane_priority_class: awx-demo-high-priority
  postgres_priority_class: awx-demo-medium-priority

also sets default requests for the postgres pod to ensure a Burstable QoS

@@ -223,7 +226,12 @@ postgres_storage_requirements:
requests:
storage: 8Gi
postgres_init_container_resource_requirements: {}
postgres_resource_requirements: {}
postgres_resource_requirements:
Copy link
Member

Choose a reason for hiding this comment

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

@fosterseth I noticed this PR after putting up this other PR that makes a similar change: #876

I think your default request values may be a little too high (requests are guaranteed resources, it doesn't prevent the container from using more when needed).

Copy link
Member

Choose a reason for hiding this comment

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

Let's sync up next week on how to merge these. Proposal: if you fold in my changes in #876 and fix the conflicts, we can just merge this and close the other.

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 pulled your commit into this PR

Copy link
Member

@rooftopcellist rooftopcellist Apr 20, 2022

Choose a reason for hiding this comment

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

Thank you @fosterseth

I will pull this down and test it out again.

@fosterseth fosterseth force-pushed the add_priorityclass_option branch from af58394 to 7156cc4 Compare April 18, 2022 16:27
fosterseth and others added 2 commits April 18, 2022 12:29
- Add postgres_priority_class
- Add control_plane_priority_class
- Add default requests for postgres pod to ensure at a "Burstable" QoS
@fosterseth fosterseth force-pushed the add_priorityclass_option branch from 7156cc4 to 21062f0 Compare April 18, 2022 16:30
@ikhan2010
Copy link

@shanemcd do you mind giving this a review when you get chance, please?

@rooftopcellist
Copy link
Member

Here are a couple relevant snippets from my deployment:

AWX Deployment

        name: awx-ee
        resources:
          requests:
            cpu: 100m
            memory: 64Mi
        name: awx-task
        resources:
          requests:
            cpu: 100m
            memory: 128Mi
        name: awx-web
        resources:
          requests:
            cpu: 100m
            memory: 128Mi

Postgres StatefulSet

        name: database-check # <------------- init container
        resources:
          requests:
            cpu: 10m
            memory: 64Mi
        name: postgres
        ports:
        - containerPort: 5432
          name: postgres
          protocol: TCP
        resources:
          requests:
            cpu: 10m
            memory: 64Mi
...
      resources:
        requests:
          storage: 8Gi

@@ -41,6 +41,9 @@ spec:
{% for secret in image_pull_secrets %}
- name: {{ secret }}
{% endfor %}
{% endif %}
{% if control_plane_priorityclass is defined %}
Copy link
Member

Choose a reason for hiding this comment

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

There is a typo here: control_plane_priority_class

@rooftopcellist
Copy link
Member

It would be nice to have a section about setting these priority classes in the README.md.

@rooftopcellist
Copy link
Member

rooftopcellist commented Apr 21, 2022

@fosterseth This commit adds some info to the readme and fixes the typo noted above if you want to just cherry-pick it in (dd3aa99, and here is the branch: https://github.com/ansible/awx-operator/compare/devel...rooftopcellist:add-to-priorityclass-option?expand=1).
The priority classes are now properly added to the deployment and statefulset:

$ oc get deployment awx-demo -o yaml | grep priorityClassName:
            f:priorityClassName: {}
      priorityClassName: high-priority

$ oc get sts awx-demo-postgres -o yaml | grep priorityClassName:
            f:priorityClassName: {}
      priorityClassName: high-priority

@rooftopcellist
Copy link
Member

Hmm... looks like CI is failing because the pod to run the job template is timing out:

Error creating pod: timed out waiting for the condition

This may be because of that typo. fwiw, with the commit I linked earlier, I am able to launch jobs on my deployment of awx without issue.

@rooftopcellist
Copy link
Member

Looks like CI still failed, it is because the environment that GitHub workflows runs ion does not have a lot of resources. The new defaults the pg init container that were added were enough to make it so that the pods never got scheduled. I also reduced the requests for the other containers to avoid running into something like this in the future. Changes are in this commit - 4b6f052

  * GitHub Workflows run in a resource constrained environment, we were
    asking too much of it, so pods never got scheduled.
Copy link
Member

@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

@fosterseth fosterseth merged commit 7fd5083 into ansible:devel Apr 21, 2022
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.

3 participants