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

Extending the Azure DevOps Agent scaling - possibility to "ignore" demands #5579

Open
jan-mrm opened this issue Mar 7, 2024 · 19 comments · May be fixed by #5839 or kedacore/keda-docs#1496
Open

Extending the Azure DevOps Agent scaling - possibility to "ignore" demands #5579

jan-mrm opened this issue Mar 7, 2024 · 19 comments · May be fixed by #5839 or kedacore/keda-docs#1496
Labels
feature-request All issues for new features that have not been committed to needs-discussion

Comments

@jan-mrm
Copy link

jan-mrm commented Mar 7, 2024

Proposal

I would like the possibility within the azure-pipelines trigger of a ScaledObject and ScaledJob to differentiate between demands which are required for scaling up and demands which are ignore/accepted but not important for actually triggering the scaling.

Idea:

demands: 'cap-8gb'
requireAllDemands: true
demandsToIgnore: 'npm' # new configuration

This is because we are not fully in control of the demands which are requested by an Azure DevOps pipeline. Demands can be defined in the pipeline definition. But demands are also just added automatically when using certain tasks.

Use-Case

We currently use Keda to scale our Azure DevOps agents using the azure-pipelines trigger, with the following setup:

  • We have two kinds of agents which differ in the resources (more kinds or other differentiation could come). Lets say the two kinds are 4GB and 8GB. So as demand something like cap-4gb and cap-8gb.
  • We set requireAllDemands: false for the 4GB agent, so that it would be the default, since a pipeline job with no demands at all will trigger this agent (and it is also triggered when a pipeline has the demand cap-4gb).
demands: 'cap-4gb'
requireAllDemands: false
  • We set requireAllDemands: true for the 8GB agent, because we want to avoid scaling both agents when a pipeline job with no demands is issued
demands: 'cap-8gb'
requireAllDemands: true

This worked out for us until we investigated a scaling issue where a job with demand cap-8gb would not trigger Keda to scale. It turned out the pipeline job uses a Npm@1 task which as stated at the bottom under Requirements will add the demand npm to the pipeline just through using that task.
This does conflict with requireAllDemands: true since it checks for the exact match (in this case the job will have demands cap-8gb and npm).

Is this a feature you are interested in implementing yourself?

Yes

Anything else?

Quick fixes are

  • adding a third agent with demands cap-8gb,npm and requireAllDemands: true
  • adding another trigger for both kinds of agents which account for npm

Both solutions work for now, but I'm not sure if there are more pipeline task that add demands because the possibilities that would be needed to be covered by trigger combinations would grow fast.

Would love some feedback about how you feel about adding something like demandsToIgnore.

@jan-mrm jan-mrm added feature-request All issues for new features that have not been committed to needs-discussion labels Mar 7, 2024
jan-mrm added a commit to jan-mrm/keda that referenced this issue May 4, 2024
jan-mrm added a commit to jan-mrm/keda that referenced this issue May 4, 2024
…ccept demands but not require them for scaling (kedacore#5579)

Signed-off-by: jan-mrm <[email protected]>
@jan-mrm
Copy link
Author

jan-mrm commented May 4, 2024

hey, any discussion or feedback on this? 🙂

@JorTurFer
Copy link
Member

I missed this issue, sorry for that :(
I guess that it can make sense but I can me missing something.
@tomkerkhove @Eldarrin , WDYT about this option?

@Eldarrin
Copy link
Contributor

Eldarrin commented May 8, 2024

IT looks fine to me, there is an existing function that already strips the agent version from the demand array so that could be extended. Although I would suggest looking at the parent matching system rather than the demand matching system as although that is more complex to initially set up that passes the scaling problem back to ADO to handle the agent matching paradigms so that keda doesn't have to do it.

@JorTurFer
Copy link
Member

is the parent approach worth it for your use case @jan-mrm ?

@jan-mrm
Copy link
Author

jan-mrm commented May 18, 2024

@JorTurFer I have looked at it a while ago but I didn't really understand how to use it I guess. I can take another look at it

@Eldarrin
Copy link
Contributor

If you are struggling reach out and I can help. If the documentation is insufficient I probably need to improve it :)

@Nohac
Copy link

Nohac commented May 29, 2024

I'm also facing this. Random azure pipeline tasks like CMake adds "invisible" demands to a job, release pipelines also has demands that's added based on what tasks are being used that can't be removed.

A workaround for this would be to not use the requireAllDemands and just generate unique permutation of all demands.
An example of this would be turning the following separate demands cpu-16, cpu-32, mem-8, mem-32, into cpu-16-mem-16, cpu-16-mem-32 etc..
That will avoid the issue where a demand matches multiple scale definitions, as mentioned in the docs:

Note: If more than one scaling definition is able to fulfill the demands of the job then they will both spin up an agent.

I don't think the suggestion of adding a demandsToIgnore would scale well, because a list like this would need to be constantly updated. Using a "parent" would also not solve my use-case, I think.

In my opinion, this could be solved in a few ways:

  • Change the scaling behavior to only dispatch a single job even if multiple demands match.
  • Add a demandApproveList that can include all the demands you want to consider when matching.

Example using approve list:

demands: mem-8,cpu-16
requireAllDemands: true
demandApproveList: cpu-16,cpu-32,mem-8,mem-32

So even if "CMake" is part of the demands, it will not be considered. The downside of this is that all definitions need to know about all the demands, but that could be easily automated using helm.

Edit: If possible, making the demandApproveList a "global" KEDA configuration would probably be better.

@jan-mrm
Copy link
Author

jan-mrm commented May 30, 2024

I don't think the suggestion of adding a demandsToIgnore would scale well, because a list like this would need to be constantly updated. Using a "parent" would also not solve my use-case, I think.

I see your point. If there are new demands, which are added by task, encountered, then the list would need to be updated. Not sure how often that happens and for my use case I'd think that's okay. Also since you would make sure that the self-hosted agent actually fulfils the demand. How many are known to you at this point, besides cmake?

  • Add a demandApproveList that can include all the demands you want to consider when matching.
demands: mem-8,cpu-16
requireAllDemands: true
demandApproveList: cpu-16,cpu-32,mem-8,mem-32

I think I do not fully understand your demandApproveList idea and the example yet. Could you explain it again?
I see you set the two demands and requireAllDemands. What is the effect of the list in the trigger definition. For example what does the cpu-32 demand in the list do?
To be triggered a pipeline would have set the two required demands.


Just another idea:

Add a flag like allowAdditionalDemands as a boolean. Since requireAllDemands exactly matches the given demands and does not allow 'more' / 'additional' demands. (That it does not allow less demands is the expected behavior but an idea would be to allow any additional)

That would not need any lists to be updated or demands to be known.

What do you think about that?

Edit: Or a change of the requireAllDemands behavior to allow additional demands in general (without a new flag), but that would mean a behavior change of a current feature.

@Nohac
Copy link

Nohac commented May 30, 2024

As I mentioned in my edit, the demandApproveList would make more sense as a "global" config and would tell KEDA what demands to consider when matching and ignore anything else, but I think KEDA is designed in a way that makes that hard, because each scale definition seems very "standalone".

The main issue now, is that demands: mem-8,cpu-16 without requireAllDemands would mach any subset of that list, as far as I understand? So a job with only mem-8 demand would match.
Adding a allowAdditionalDemands would make little sense as you would need the following config that would look a bit contradicting:

demands: mem-8,cpu-16
requireAllDemands: true
allowAdditionalDemands: true

What we need is a flag that conveys "require at least all specified demands, ignore anything else", but coming up with a user friendly name for it seems difficult.
Here's a few suggestions from chatgpt:

  1. requireExactOrSupersetDemands
  2. ignoreUnspecifiedDemands
  3. exactOrSupersetDemandsOnly
  4. strictDemandMatching
  5. minimumRequiredDemands
  6. matchAtLeastSpecifiedDemands
  7. enforceSpecifiedDemands
  8. mandatoryDemandsOnly
  9. requireAndIgnoreOthers
  10. demandSubsetRestriction

@jan-mrm
Copy link
Author

jan-mrm commented May 30, 2024

As I mentioned in my edit, the demandApproveList would make more sense as a "global" config and would tell KEDA what demands to consider when matching and ignore anything else, but I think KEDA is designed in a way that makes that hard, because each scale definition seems very "standalone".

Alright, kind of got it.

The main issue now, is that demands: mem-8,cpu-16 without requireAllDemands would mach any subset of that list, as far as I understand? So a job with only mem-8 demand would match. Adding a allowAdditionalDemands would make little sense as you would need the following config that would look a bit contradicting:

demands: mem-8,cpu-16
requireAllDemands: true
allowAdditionalDemands: true

Yes that is true. Having to set both flags seems ugly and also the allowAdditionalDemands would seem to have no effect when not also setting requireAllDemands.

What we need is a flag that conveys "require at least all specified demands, ignore anything else"

So just so sum up the idea:
We should not change the behavior of the current requireAllDemands? So if not then the idea is to add one additional flag besides requireAllDemands.
requireAllDemands would stay as it is right now, as the exact match.
The new flag - something like requireAllDemandsAndIgnoreOthers (just something to convey the meaning here, the actual user friendly name would need to be decided) - would check that all given demands from field demands are present but any additional ones defined in the pipeline would be ignored/accepted.

Is that right?

How should we handle setting both flags to true since they conflict?

@Nohac Nohac linked a pull request May 30, 2024 that will close this issue
7 tasks
@Nohac
Copy link

Nohac commented May 30, 2024

How should we handle setting both flags to true since they conflict?

Because requireAllDemands is a "stronger" rule, I'm thinking it can take precedence.

I opened a draft PR just to get the ball rolling on this.

@jan-mrm
Copy link
Author

jan-mrm commented Jun 5, 2024

maybe @JorTurFer and/or @Eldarrin have feedback to this idea Nohac an I discussed (see draft PR of Nohac):
not having a list of demands to ignore anymore (original idea of this issue) but a less strict version of requireAllDemands which does allow additional demands

@JorTurFer
Copy link
Member

JorTurFer commented Jun 24, 2024

I think that the approach proposed by @Nohac of having different "matching" rules can bring a lot of flexibility. @tomkerkhove @Eldarrin ?

@Nohac
Copy link

Nohac commented Aug 7, 2024

I feel like this is a relatively low risk feature to add, and shouldn't need to much consideration apart from the naming. If my changes looks OK and we can agree on the name, I will publish the PR I made and open a PR to update the docs as well.

@JorTurFer
Copy link
Member

Ping @tomkerkhove @Eldarrin ?

@Eldarrin
Copy link
Contributor

Eldarrin commented Aug 7, 2024

PR looks fine to me, its non breaking :)

Copy link

stale bot commented Oct 19, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Oct 19, 2024
@jan-mrm
Copy link
Author

jan-mrm commented Oct 19, 2024

so we just need to agree on a name (in the already opened draft pr) and update the docs?

What about the currently proposed name 'requireAllDemandsAndIgnoreOthers'? I feel like it does match the functionality. Feels a bit clunky too but yea

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Oct 19, 2024
@Nohac
Copy link

Nohac commented Nov 7, 2024

I'm sorry about the delay, the PR should be ready now.

Let's just keep the name as is. It's definitely a bit clunky, but at the same time also the most descriptive name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion
Projects
Status: To Triage
4 participants