Skip to content

Conversation

@DanielMorales9
Copy link
Contributor

@DanielMorales9 DanielMorales9 commented Sep 25, 2022

What does your PR do?

Introducing the scheduled pool implementation enables sync functionality to bump or reduce the number of pool slots on a specific recurrence.

Checklist

For all Pull Requests

For releasing ONLY

Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
@DanielMorales9
Copy link
Contributor Author

Hi @thesuperzapper, I would like to introduce the scheduled pools to enable scheduled autoscaling policies to the sync pools. The code is very simple just like its interface as shown below:

- name: "pool"
   description: "example pool with 10 slots"
   slots: 10
   schedules:
     - name: "ScaleUp"
        slots: 28
        recurrence: 0 19 * * *
     - name: "ScaleDown"
        slots: 10
        recurrence: 0 6 * * *

@stale
Copy link

stale bot commented Nov 26, 2022

This issue has been automatically marked as stale because it has not had activity in 60 days.
It will be closed in 7 days if no further activity occurs.

Thank you for your contributions.


Issues never become stale if any of the following is true:

  1. they are added to a Project
  2. they are added to a Milestone
  3. they have the lifecycle/frozen label

@stale stale bot added the lifecycle/stale lifecycle - this is stale label Nov 26, 2022
@stale stale bot closed this Dec 3, 2022
@thesuperzapper thesuperzapper reopened this Feb 7, 2023
@thesuperzapper thesuperzapper added the status/needs-discussion status - this needs discussion label Feb 7, 2023
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@DanielMorales9 This is an interesting idea, but it needs more discussion.

I think we can remove the need for the global variables, while also making the behavior more stable by making a change to your logic.

The main goal I want to achieve is that even if we are not "at a scheduled update time" we should revert to the slot_count that would have been most recently applied (if someone makes a manual change in the UI we should revert it at the next sync, for example).

We can do this by using the croniter.get_prev() function on each of the defined schedules at each sync using the current time as the start_time, and applying the schedule with the most recent previous time.

For example:

croniter(expr_format="0 12 * * *", start_time=datetime.now()).get_prev(ret_type=datetime)

This approach means we have no state to hold, and there is no ambiguity at the first sync about which schedule to use. Also note, in this approach the non-schedule slots defined in the user's values will never be applied.

@thesuperzapper thesuperzapper added this to the airflow-8.7.0 milestone Feb 7, 2023
@thesuperzapper thesuperzapper removed the status/needs-discussion status - this needs discussion label Feb 7, 2023
@thesuperzapper thesuperzapper changed the title scheduled pools code support changing pool slots on cron schedules Feb 7, 2023
@stale stale bot removed the lifecycle/stale lifecycle - this is stale label Feb 11, 2023
@DanielMorales9 DanielMorales9 force-pushed the scheduled_pools branch 2 times, most recently from 9a2c726 to c7722a7 Compare February 15, 2023 21:15
Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
Daniel Morales added 3 commits February 16, 2023 00:22
Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
@DanielMorales9
Copy link
Contributor Author

Hi @thesuperzapper could you take another look at the PR?
Thank you

Signed-off-by: Daniel Morales <[email protected]>
Signed-off-by: Daniel Morales <[email protected]>
@thesuperzapper thesuperzapper changed the title support changing pool slots on cron schedules feat: allow changing pool slots on cron schedules Apr 5, 2023
thesuperzapper
thesuperzapper previously approved these changes Apr 6, 2023
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@DanielMorales9 thanks for your patience, I will be including this in the 8.7.0 chart release today!

Also, I have pushed a commit to your branch with some changes and docs in preparation for this.

@thesuperzapper thesuperzapper added the status/ready-to-merge status - this will be merged into next release label Apr 6, 2023
@thesuperzapper thesuperzapper merged commit e2943b9 into airflow-helm:main Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/ready-to-merge status - this will be merged into next release

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants