keda icon indicating copy to clipboard operation
keda copied to clipboard

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

Open jan-mrm opened this issue 2 years ago • 18 comments

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 avatar Mar 07 '24 19:03 jan-mrm

hey, any discussion or feedback on this? 🙂

jan-mrm avatar May 04 '24 08:05 jan-mrm

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?

JorTurFer avatar May 07 '24 20:05 JorTurFer

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.

Eldarrin avatar May 08 '24 11:05 Eldarrin

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

JorTurFer avatar May 18 '24 10:05 JorTurFer

@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

jan-mrm avatar May 18 '24 13:05 jan-mrm

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

Eldarrin avatar May 19 '24 15:05 Eldarrin

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.

Nohac avatar May 29 '24 14:05 Nohac

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.

jan-mrm avatar May 30 '24 08:05 jan-mrm

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

Nohac avatar May 30 '24 09:05 Nohac

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?

jan-mrm avatar May 30 '24 09:05 jan-mrm

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.

Nohac avatar May 30 '24 21:05 Nohac

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

jan-mrm avatar Jun 05 '24 19:06 jan-mrm

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

JorTurFer avatar Jun 24 '24 13:06 JorTurFer

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.

Nohac avatar Aug 07 '24 10:08 Nohac

Ping @tomkerkhove @Eldarrin ?

JorTurFer avatar Aug 07 '24 11:08 JorTurFer

PR looks fine to me, its non breaking :)

Eldarrin avatar Aug 07 '24 13:08 Eldarrin

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[bot] avatar Oct 19 '24 08:10 stale[bot]

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

jan-mrm avatar Oct 19 '24 12:10 jan-mrm

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.

Nohac avatar Nov 07 '24 09:11 Nohac

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[bot] avatar Jan 09 '25 05:01 stale[bot]

I still think this feature is helpful. @Nohac do you have some time to take a look at the feedback in the pull request you opened to implement this feature? 🙂

jan-mrm avatar Jan 09 '25 06:01 jan-mrm

Sorry about this. They have so many requirements to the commits that's hard to follow and I can't understand being necessary, but I'll try to find some time this week to finish it.

Nohac avatar Jan 09 '25 09:01 Nohac