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

fix: respect all required demands in azure pipeline scaler #4405

Conversation

EugeneLugovtsov
Copy link
Contributor

@EugeneLugovtsov EugeneLugovtsov commented Mar 23, 2023

Fix issue with Azure Pipelines Scaler. When Azure DevOps does not provide Agent.Version keda may scale a wrong agent.

Checklist

Fixes #
#4404
Relates to #

Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@EugeneLugovtsov EugeneLugovtsov requested a review from a team as a code owner March 23, 2023 20:15
Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@EugeneLugovtsov
Copy link
Contributor Author

@zroubalik may I ask you to review this PR?

@JorTurFer
Copy link
Member

@Eldarrin, could you take a look?

Copy link
Contributor

@Eldarrin Eldarrin left a comment

Choose a reason for hiding this comment

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

Can you replace the function with this and resubmit, may fix the issue.

`
func stripAgentVFromArray(array []string) []string {
var result []string
for _, item := range array {
if !strings.HasPrefix(item, "Agent.Version") {
result = append(result, item)
}
}
return result
}

func getCanAgentDemandFulfilJob(jr JobRequest, metadata *azurePipelinesMetadata) bool {
countDemands := 0
demandsInJob := stripAgentVFromArray(jr.Demands)
demandsInScaler := stripAgentVFromArray(strings.Split(metadata.demands, ","))
for _, demandInJob := range demandsInJob {
for _, demandInScaler := range demandsInScaler {
if demandInJob == demandInScaler {
countDemands++
}
}
}
if metadata.requireAllDemands {
return countDemands == len(demandsInJob) && countDemands == len(demandsInScaler)
} else {
return countDemands == len(demandsInJob)
}
}`

Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@EugeneLugovtsov
Copy link
Contributor Author

@Eldarrin thanks for your help. I updated PR

Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@EugeneLugovtsov
Copy link
Contributor Author

@Eldarrin could you please help with it?
image

@Eldarrin
Copy link
Contributor

@Eldarrin could you please help with it? image

HI @EugeneLugovtsov I can't see anything wrong with it, @JorTurFer is there some sort of idiomatic thing I'm missing or is it possible to //nolint this?

@zroubalik
Copy link
Member

zroubalik commented Mar 28, 2023

@Eldarrin could you please help with it? image

HI @EugeneLugovtsov I can't see anything wrong with it, @JorTurFer is there some sort of idiomatic thing I'm missing or is it possible to //nolint this?

What is potentially suspicious is, that we are comparing the same property countDemands against different 2 properties but these should have the same value, because we have AND there. If this is really what we want to have, then we should add exception to https://github.com/kedacore/keda/blob/main/.golangci.yml

Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@EugeneLugovtsov
Copy link
Contributor Author

@zroubalik yep, we want to be sure, that countDemands, len(demandsInJob), and len(demandsInScaler) are the same.

Signed-off-by: Eugene Lugovtsov <lug.zhenia@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Mar 28, 2023

/run-e2e azure*
Update: You can check the progress here

@Eldarrin
Copy link
Contributor

/run-e2e azure* Update: You can check the progress here

@zroubalik can you do the e2e with pipelines, not the whole of azure? looks like those are failing.

Cheers :)

@JorTurFer
Copy link
Member

JorTurFer commented Mar 29, 2023

/run-e2e pipeline*
Update: You can check the progress here

@JorTurFer
Copy link
Member

Yeah, Azure test are failing because this commit is required

@EugeneLugovtsov
Copy link
Contributor Author

@JorTurFer @Eldarrin do I need to do something? do I need to add this commit ?

@Eldarrin
Copy link
Contributor

@JorTurFer @Eldarrin do I need to do something? do I need to add this commit ?

Think you just need to complete the task list by checking the boxes at the top.

@EugeneLugovtsov
Copy link
Contributor Author

@JorTurFer @Eldarrin could you help with the next steps? what do we need to do?

Copy link
Contributor

@Eldarrin Eldarrin left a comment

Choose a reason for hiding this comment

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

Approved from my end, all looks good

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix! ❤️

@JorTurFer
Copy link
Member

JorTurFer commented Apr 5, 2023

/run-e2e pipeline*
Update: You can check the progress here

@JorTurFer JorTurFer enabled auto-merge (squash) April 5, 2023 06:04
@JorTurFer JorTurFer mentioned this pull request Apr 5, 2023
17 tasks
@JorTurFer
Copy link
Member

JorTurFer commented Apr 5, 2023

/run-e2e pipeline
Update: You can check the progress here

@JorTurFer
Copy link
Member

The e2e tests are failing due to expired PAT, I'll fix it asap

@JorTurFer
Copy link
Member

JorTurFer commented Apr 11, 2023

/run-e2e pipeline
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 3dc157b into kedacore:main Apr 11, 2023
22 checks passed
JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Apr 13, 2023
JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Apr 13, 2023
…4405)

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
JorTurFer added a commit that referenced this pull request Apr 13, 2023
…<Jorge_turrado@hotmail.es> Co-authored-by: Andy Ward <mortx@toothless.eldarrin.io> Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com> Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com> Co-authored-by: Eldarrin <32762846+Eldarrin@users.noreply.github.com> Co-authored-by: Eugene Lugovtsov <34510252+EugeneLugovtsov@users.noreply.github.com> fix(aws-sqs): Respect `scaleOnInFlight` value (#4358) fix odd number of arguments passed as key-value pairs for logging (#4369) fix: Azure Pipelines Scaler uses correct endpoint when demands are set  (#4387) (#4401) fix: respect all required demands in azure pipeline scaler (#4405) fix: Allow to remove the finalizer even if the ScaledObject isn't valid (#4397)

* Drop a transitive dependency on bou.ke/monkey (#4366)

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix(aws-sqs): Respect `scaleOnInFlight` value (#4358)

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* chore: update supported versions in the welcome message (#4360)

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix odd number of arguments passed as key-value pairs for logging (#4369)

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix: Azure Pipelines Scaler uses correct endpoint when demands are set  (#4387) (#4401)

Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Co-authored-by: Andy Ward <mortx@toothless.eldarrin.io>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix: respect all required demands in azure pipeline scaler (#4405)

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* fix: Allow to remove the finalizer even if the ScaledObject isn't valid (#4397)

Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update changelog

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

---------

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Co-authored-by: Eldarrin <32762846+Eldarrin@users.noreply.github.com>
Co-authored-by: Andy Ward <mortx@toothless.eldarrin.io>
Co-authored-by: Eugene Lugovtsov <34510252+EugeneLugovtsov@users.noreply.github.com>
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
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.

None yet

4 participants