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

Azure DevOps Scaler Demands Must Be a Subset #4195

Closed
ruisantox opened this issue Feb 3, 2023 · 16 comments
Closed

Azure DevOps Scaler Demands Must Be a Subset #4195

ruisantox opened this issue Feb 3, 2023 · 16 comments
Labels
bug Something isn't working

Comments

@ruisantox
Copy link

Report

Possible bug Please review if is correct.

Reading the documentation: (https://keda.sh/docs/2.9/scalers/azure-pipelines/)

Using demands: KEDA will determine which agents can fulfill the job based on the demands provided. The demands are provided as a comma-separated list and must be a subset of the actual capabilities of the agent. (For example maven,java,make. Note: Agent.Version is ignored)

Reading the code:
https://github.com/kedacore/keda/blob/main/pkg/scalers/azure_pipelines_scaler.go

Line 364:

// Determine if the scaledjob has the right demands to spin up
func getCanAgentDemandFulfilJob(jr JobRequest, metadata *azurePipelinesMetadata) bool {
	var demandsReq = jr.Demands
	var demandsAvail = strings.Split(metadata.demands, ",")
	var countDemands = 0
	for _, dr := range demandsReq {
		for _, da := range demandsAvail {
			strDr := fmt.Sprintf("%v", dr)
			if !strings.HasPrefix(strDr, "Agent.Version") {
				if strDr == da {
					countDemands++
				}
			}
		}
	}

	return countDemands == len(demandsReq)-1
}

Not familiar with the code but looks to me it is not the subset but all the demands minus the version demand.

Expected Behavior

Demands should be a subset. Like the webpage says.

Actual Behavior

All the demands need to mache minus one

Steps to Reproduce the Problem

  1. Demand a subset
  2. do not use parent

Logs from KEDA operator

example

KEDA Version

2.9.3

Kubernetes Version

1.26

Platform

Other

Scaler Details

azure devops

Anything else?

I hope that I read correctly the code.

@ruisantox ruisantox added the bug Something isn't working label Feb 3, 2023
@ruisantox ruisantox changed the title Azure Deops Scaler Demands Must Be a Subset Azure DevOps Scaler Demands Must Be a Subset Feb 3, 2023
@JorTurFer
Copy link
Member

@Eldarrin , could you take a look? 🙏

@Eldarrin
Copy link
Contributor

Eldarrin commented Feb 6, 2023 via email

@Eldarrin
Copy link
Contributor

Eldarrin commented Feb 8, 2023

So, this is a bug in the documentation, not a bug in the implementation. It is designed to match all, not match any.

I would suggest that I don't change this implementation, but extend it to have 2 options (demandsMatch: any and demandsMatch: all). Be aware though that with demandsMatch: any the scaler will spin up all agents that can match, not just the first one that hits as each scaler is independent of the others and there is no state across the scalers.

And/or fix the docs :)

Parent type matching works better for these scenarios as it passes all the matching complexity into ADO rather than KEDA having to do it.

@ruisantox
Copy link
Author

Thank you for the feedback. The demand "any" for us might work. ( I assume any as a subset of demands) We have our deployment per scaling, this is the reason for us being more useful to match a set only .

But I must say that we problay could change our architecture to work more with the scalar. I trust in your good judgement to decide how you think is better for your product. Thank you for your work.

@patst
Copy link
Contributor

patst commented Feb 8, 2023

hey there,

if I understand your requirement correct probably my PR will help you : #4203

The part in the docs with

Using demands: KEDA will determine which agents can fulfill the job based on the demands provided. The demands are provided as a comma-separated list and must be a subset of the actual capabilities of the agent. (For example maven,java,make. Note: Agent.Version is ignored)

was hard to understand for me as well. But I think we must differentiate three things:

  1. the running buildagents have capabilities (the ones shown on the Agent details -> Capability tab)
  2. the KEDA trigger has demands (specified in YAML)
  3. the build jobs have demands

The part in the docs is related to 1. + 2.. If the KEDA triggers scales up a new build agent, the build agents capabilities have no direct relationship to the KEDA triggers demands. In theory the ScaledObject can have totally different capabilities to what the trigger has specified as demands and what the build job is expecting.

So, the docs stating the agents capabilities should be a subset of the KEDA triggers demands is correct, imho.

The problem the current implementation has and what I am trying to solve with #4203 is that there is no possibility to have a exact matching between 2. + 3. .
At the moment build jobs without any demands will scale all deployments.

@JorTurFer
Copy link
Member

Sorry for my ignorant question (I'm not an expert in AzDO). Doesn't #4203 fulfill your requirements? I mean, you could specify the demands you want as required.

@Eldarrin
Copy link
Contributor

PR #4405 should fix this subset to be true subset or all matching

@arthurgawronski
Copy link

hey there,

if I understand your requirement correct probably my PR will help you : #4203

The part in the docs with

Using demands: KEDA will determine which agents can fulfill the job based on the demands provided. The demands are provided as a comma-separated list and must be a subset of the actual capabilities of the agent. (For example maven,java,make. Note: Agent.Version is ignored)

was hard to understand for me as well. But I think we must differentiate three things:

  1. the running buildagents have capabilities (the ones shown on the Agent details -> Capability tab)
  2. the KEDA trigger has demands (specified in YAML)
  3. the build jobs have demands

The part in the docs is related to 1. + 2.. If the KEDA triggers scales up a new build agent, the build agents capabilities have no direct relationship to the KEDA triggers demands. In theory the ScaledObject can have totally different capabilities to what the trigger has specified as demands and what the build job is expecting.

So, the docs stating the agents capabilities should be a subset of the KEDA triggers demands is correct, imho.

The problem the current implementation has and what I am trying to solve with #4203 is that there is no possibility to have a exact matching between 2. + 3. . At the moment build jobs without any demands will scale all deployments.

This does not fix the problem where demands are uniquely set in 2 different ScaledJobs for the same agent pool but no demands are specified in the Azure Pipeline Job, which UNEXPECTEDLY results in ALL deployments being scaled (two agents are created instead of 1). In theory, this should trigger one ScaledJob or the other but not both - one agent only for one of the capability sets (the empty pipeline demands/empty set use case). I'm not sure if this is an architecture limitation or something the operator can have an awareness of so that some round-robin or load balancing occurs when the pipeline doesn't care about any capability but the ScaledJobs have demands specified.

@Eldarrin
Copy link
Contributor

@arthurgawronski The upcoming patch with requireAllDemands set to true on the ScaledJob will mean that the demands in the pipeline must exactly match the demands in the ScaledJob (with the exception of Agent.Version) to scale, so no demands would scale nothing. However, you are correct in that without this parameter, no demands in the pipeline scales everything.

@arthurgawronski
Copy link

@Eldarrin Does this mean that there is no way to support a multi-cluster approach where a ScaledJob is defined in two places for a single node pool i.e. a separate "capability"? This would always trigger scaling in both instances and one instance would ultimately be abandoned (only a job would only be scheduled on one of the agents)?

@Eldarrin
Copy link
Contributor

@arthurgawronski I'm not sure I understand exactly what you mean, so if my understanding is incorrect I apologise, but I think its this:
Regardless of which method you use, parent, require all, require subset, if multiple scalers are eligible to scale, they will scale.
This is because each scaler is an independent entity that evaluates its scaling position on its own. The only way around this would be to have a multi state matrix shared across all scalers of the same type. I'm not sure we can do this; @JorTurFer any thoughts?

@arthurgawronski
Copy link

Yes @Eldarrin , that is basically it.

There seems to be an old thread here (#1587) that seems to have some thought around a single "aware" component rather than all instances and/or ScaledJobs acting independently.

My only workaround at this point to support the multi capability/multi cluster requirement while continuing to support the use-case where the demands are not required and/or not set in the Azure Pipeline is the following:

ScaledJob1 with demands: CapabilityA with requireAllDemands true (PoolX) -> force to KEDA operator/ScaledJob on ClusterA
ScaledJob2 with demands: CapabilityB with requireAllDemands true (PoolX) -> force to KEDA operator/ScaledJob on ClusterB
ScaledJob3 with no demands specified (PoolX) -> only deployed to ClusterA

The above would trigger if someone demands CapabilityA in one scenario or CapabilityB, but not the other and, also hopefully not ScaledJob3.

On the empty set scenario, if demands are not set, Job1 and Job2 should not be scaled and only Job3 should be used.

I have tried to solution a "ScaledJob4" with "no demands specified" for ClusterB but this would still cause ScaledJob3 and ScaledJob4 to scale causing an abandoned Agent.

Unless you or @JorTurFer say otherwise or can see alternatives, for now, I'll need to deal with this agent pool being backed by only one empty set location.

@Eldarrin
Copy link
Contributor

The only alternative I can think of, but would require a bit of re-architecting of your scaledjobs. If you use parent then you push the scaler selection out of keda and into AzDo. Maybe something there may do it

@JorTurFer
Copy link
Member

The problem is that currently there isn't any way to create some kind of KEDA cluster across different clusters, so different KEDA instances will see the same if the ScaledJob is the same. I can't see any way to achieve the behavior you want without communicating KEDA instances across all the clusters :(

@Eldarrin
Copy link
Contributor

I think the OP issue has been fixed by the PR#4405 & PR#4203; should this be closed?

I think @arthurgawronski 's issue is different to the OP's and actually is handled partially through ADO parent as this pushes that part of the state back to AzDO. We still have the concurrent spawn issue of #3782 however which also relates.

@JorTurFer
Copy link
Member

I close this issue, feel free to reopen it if the problem isn't solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Ready To Ship
Development

No branches or pull requests

5 participants