Correct Dynamic Queue logic for CLOUD_PROVIDER and Static node combinations

[andrew-m-leonard]

Fixes #6773

Concurrent tests with CLOUD_PROVIDER=azure, with 10 "static" nodes available:

• https://ci.adoptium.net/job/Test_openjdk21_hs_sanity.openjdk_x86-64_linux/363/console
  • All provisioned on 3 of the Static nodes
• https://ci.adoptium.net/view/Test_openjdk/job/Test_openjdk21_hs_extended.openjdk_x86-64_linux/207/console
  • 7 provisioned on the remaining Static nodes
  • 3 provisioned on Azure dynamic nodes: test-linux-x64-d0f330 test-linux-x64-d0f331 test-linux-x64-d1d830

00:00:13.906  Specified LABEL: ci.role.test&&hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12
[Pipeline] stage
[Pipeline] { (Queue)
Did you forget the `def` keyword? WorkflowScript seems to be setting a field named dynamicAgents (to a value of type ArrayList) which could lead to memory leaks or other issues.
[Pipeline] echo
00:00:13.923  dynamicAgents: [azure, fyre, EBC]
[Pipeline] echo
00:00:13.925  Added dynamic agent, nodeLabel: 'hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12&&(ci.role.test||ci.agent.dynamic)', dynamicLabel: 'azure'
[Pipeline] echo
00:00:13.926  Provisioning LABEL: hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12&&(ci.role.test||ci.agent.dynamic)
[Pipeline] nodesByLabel
00:00:13.934  Found a total of 10 nodes with the 'hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12&&(ci.role.test||ci.agent.dynamic)' label
[Pipeline] node
00:00:28.937  Still waiting to schedule task
00:00:28.937  Waiting for next available executor on ‘[hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12&&(ci.role.test||ci.agent.dynamic)](https://ci.adoptium.net/label/hw.arch.x86&&sw.os.linux&&!sw.tool.glibc.2_12&&(ci.role.test%7C%7Cci.agent.dynamic)/)’
00:03:26.402  Running on [test-linux-x64-d0f330](https://ci.adoptium.net/computer/test%2Dlinux%2Dx64%2Dd0f330/) in /home/adoptopenjdk/workspace/Test_openjdk21_hs_extended.openjdk_x86-64_linux_testList_0

[karianna]

It's starting to feel like we need a state machine here...

[smlambert]

thanks @andrew-m-leonard !

FYI - @JasonFengJ9 @pshipton

[sophia-guo]

If static nodes are available they should be used first. Is this still expected behaviour for now? Or it doesn't matter to use dynamic ones or static ones?

[smlambert]

If static nodes are available they should be sued first. Is this still expected behaviour for now? Or it doesn't matter to use dynamic ones or static ones?

This PR fixes the behaviour. Somehow over the past year, that functionality got 'broken', to only use dynamic agents if CLOUD_PROVIDER is set. Andrew has restored the behaviour to use idle static nodes first, then spin up dynamic agents if all static nodes idle.

[andrew-m-leonard]

If static nodes are available they should be sued first. Is this still expected behaviour for now? Or it doesn't matter to use dynamic ones or static ones?

If BOTH a static node and a dynamic node(ie.it hasn't been released after doing work..) is available, then it can use either I think that's reasonable.

[sophia-guo]

Instead of changing the order of if clause if (!areNodesWithLabelOnline(LABEL)) {} does one line of change LABEL += '&&ci.agent.dynamic' to LABEL += '&&(ci.role.test||ci.agent.dynamic)' also work, which re-check the statics nodes status in case they are available after the initial check time-out?

[andrew-m-leonard]

If static nodes are available they should be sued first. Is this still expected behaviour for now? Or it doesn't matter to use dynamic ones or static ones?

This PR fixes the behaviour. Somehow over the past year, that functionality got 'broken', to only use dynamic agents if CLOUD_PROVIDER is set. Andrew has restored the behaviour to use idle static nodes first, then spin up dynamic agents if all static nodes idle.

Yes, if a Static node is "available", then it will get used first, although if both a "Static" & a "Dynamic" are available, ie.a previous provisioned Dynamic has just finished some work and is "idle" and not yet been returned to the pool, then either could be used...
That is very subtly different to what the previous code was trying to do, in that it was trying to query "idle" Static... and only add Dynamic label if it could not find any, but I believe that was a bit flawed, it is better to add (ci.role.test || ci.agent.dynamic) and let Jenkins make the choice... as if both types are available, we should let it choose.

[smlambert]

as if both types are available, we should let it choose.

Agree, and especially as we may configure via the Jenkins plugin how long a particular dynamic agent stays alive once it is spun up...

[andrew-m-leonard]

Instead of changing the order of if clause if (!areNodesWithLabelOnline(LABEL)) {} does one line of change LABEL += '&&ci.agent.dynamic' to LABEL += '&&(ci.role.test||ci.agent.dynamic)' also work, which re-check the statics nodes status in case they are available after the initial check time-out?

The problem previously was if (!areNodesWithLabelOnline(LABEL)) is not checking "available"... it is only checking if "online".
As you highlight also there is a race condition when trying to check "available" (using APIs or...), as by the time you get to the node(LABEL) the available one may not be available any longer...!

So i've changed the logic to basically:

• Construct the correct required LABEL based on static (ci.role.test) or dynamic (ci.agent.dynamic) requirement
• Then do a "online" check in case ALL nodes are "Down/Disconnected"
• Then do the node(LABEL) provision

[andrew-m-leonard]

Instead of changing the order of if clause if (!areNodesWithLabelOnline(LABEL)) {} does one line of change LABEL += '&&ci.agent.dynamic' to LABEL += '&&(ci.role.test||ci.agent.dynamic)' also work, which re-check the statics nodes status in case they are available after the initial check time-out?

The other problem with that was also if no Static's are online, then the Dynamic logic was ignored and a Timeout abort occured

[andrew-m-leonard]

Also tested on a local Jenkins with no Cloud dynamic agents, and if CLOUD_PROVIDER is specified for any reason it will correctly just use the Statics (previously it would hang)

[smlambert]

re: #6776 (comment)

@andrew-m-leonard - if there is not already a clear comment in that piece of code, it might be good to add your clarifying comment from above to guide others looking at that code in the future

[andrew-m-leonard]

re: #6776 (comment)

@andrew-m-leonard - if there is not already a clear comment in that piece of code, it might be good to add your clarifying comment from above to guide others looking at that code in the future

yep, i'll beef up the comments I added, thanks

[sophia-guo]

In this case I feel like a static will be used first but meanwhile the dynamic one will still be spun up since node(Label) has 'ci.agent.dynamic ' ?

Yes, if a Static node is "available", then it will get used first. ( No previous provisioned Dynamic available)

[andrew-m-leonard]

In this case I feel like a static will be used first but meanwhile the dynamic one will still be spun up since node(Label) has 'ci.agent.dynamic ' ?

Yes, if a Static node is "available", then it will get used first. ( No previous provisioned Dynamic available)

Not sure that matters, but I don't think Jenkins provisioning will trigger a Cloud plugin when it is scheduling it to a Static...

My test above didn't do that either, there were 10 Static nodes available when I triggered the sanity.openjdk, all 3 went on the Static nodes, no Dynamics were spun up. I then triggered the extended.openjdk which requested 10 nodes, the first 7 went on the Static nodes, the remaining 3 then waited 2mins or so for the Dynamic agents to spin up. So Jenkins did a great job!