[YUNIKORN-1793] Handle placement rule and queue changes during initialisation #601

craigcondit · 2023-07-31T15:56:29Z

What is this PR for?

Adds a tag to mark applications as forced create. This allows most validation to be suppressed and if necessary, will redirect the app to a recovery queue.

What type of PR is it?

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1793

How should this be tested?

Screenshots (if appropriate)

Questions:

- The licenses files need update.
- There is breaking changes for older versions.
- It needs documentation.

codecov · 2023-07-31T16:02:22Z

Codecov Report

Merging #601 (63633a3) into master (a4b2ebb) will increase coverage by 0.10%.
Report is 2 commits behind head on master.
The diff coverage is 76.50%.

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
+ Coverage   77.46%   77.56%   +0.10%     
==========================================
  Files          77       78       +1     
  Lines       12927    12977      +50     
==========================================
+ Hits        10014    10066      +52     
+ Misses       2593     2591       -2     
  Partials      320      320

Files Changed	Coverage Δ
pkg/scheduler/context.go	`29.61% <0.00%> (ø)`
pkg/scheduler/placement/rule.go	`93.93% <ø> (ø)`
pkg/scheduler/partition.go	`78.03% <50.00%> (+0.19%)`	⬆️
pkg/scheduler/placement/placement.go	`81.66% <71.42%> (-1.55%)`	⬇️
pkg/scheduler/placement/fixed_rule.go	`77.94% <77.77%> (+0.32%)`	⬆️
pkg/scheduler/placement/user_rule.go	`78.57% <77.77%> (+0.38%)`	⬆️
pkg/common/security/usergroup.go	`84.44% <78.57%> (-1.27%)`	⬇️
pkg/scheduler/placement/provided_rule.go	`80.32% <80.00%> (+0.32%)`	⬆️
pkg/scheduler/placement/tag_rule.go	`80.59% <80.00%> (+0.29%)`	⬆️
pkg/common/utils.go	`89.41% <84.61%> (-0.36%)`	⬇️
... and 4 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/scheduler/partition.go

craigcondit · 2023-08-03T17:28:11Z

Updated approach to move the queue placement logic entirely within the placeholder manager and incorporated YUNIKORN-19 (always use placement manager). This simplifies the logic considerably as the placement manager is now always active and if it returns a queue, that queue is always to be created if it does not exist.

wilfred-s

A 'recovery' rule (not available in the configurable list) pointing to a fixed queue (cannot be created in the config) which has a fixed ACL matching to an invalid user that cannot be specified all based on a tag that the k8shim always sets based on the state of init will I think simplify this solution.

pkg/scheduler/objects/application.go

pkg/scheduler/objects/queue.go

wilfred-s · 2023-08-07T08:11:43Z

pkg/scheduler/partition.go

-		if err != nil {
-			return fmt.Errorf("failed to create rule based queue %s for application %s", queueName, appID)
+		if common.IsRecoveryQueue(queueName) {
+			queue, err = pc.createRecoveryQueue()


I do not think that the recovery queue create should ever fail. It needs to always be there or be created otherwise we are in the same situation with an unrecoverable application again.

It should not fail, however it's still possible (and is most definitely a bug if it does). Nothing configuration-related is checked, only sanity checks such as the root queue being provided properly.

pkg/common/security/usergroup.go

pkg/scheduler/placement/provided_rule.go

craigcondit · 2023-08-08T19:04:46Z

Rebased on latest master.

…lisation Adds a tag to mark applications as forced create. This allows most validation to be suppressed and if necessary, will redirect the app to a recovery queue.

Refactored CreateRecoveryQueue / CreateDynamicQueue implementation

Recovery queue now always fails ACL checks so it can only be submitted to if the recovery rule triggers it.

pbacsko

+1 LGTM (after Wilfred's comments are addressed)

craigcondit · 2023-08-22T17:41:30Z

@wilfred-s I believe all the comments have been addressed. Can you do a final review?

craigcondit self-assigned this Jul 31, 2023

craigcondit requested review from wilfred-s, manirajv06, pbacsko and sunilgovind July 31, 2023 16:20

craigcondit force-pushed the YUNIKORN-1793 branch from 3ef792d to a2379d8 Compare July 31, 2023 23:16

pbacsko reviewed Aug 1, 2023

View reviewed changes

pkg/scheduler/partition.go Outdated Show resolved Hide resolved

craigcondit force-pushed the YUNIKORN-1793 branch 7 times, most recently from fdfde49 to 244d52c Compare August 3, 2023 17:19

craigcondit requested a review from pbacsko August 3, 2023 17:24

wilfred-s requested changes Aug 8, 2023

View reviewed changes

craigcondit force-pushed the YUNIKORN-1793 branch from 244d52c to 909ecb3 Compare August 8, 2023 19:04

craigcondit force-pushed the YUNIKORN-1793 branch from 89a304d to aca129e Compare August 8, 2023 19:56

craigcondit added 2 commits August 16, 2023 10:14

[YUNIKORN-1793] Handle placement rule and queue changes during initia…

191d128

…lisation Adds a tag to mark applications as forced create. This allows most validation to be suppressed and if necessary, will redirect the app to a recovery queue.

Added recovery_rule

9df92cf

Refactored CreateRecoveryQueue / CreateDynamicQueue implementation

craigcondit force-pushed the YUNIKORN-1793 branch from aca129e to 9df92cf Compare August 16, 2023 15:14

Refactored rules to specify whether ACLs should be checked.

5405a3a

Recovery queue now always fails ACL checks so it can only be submitted to if the recovery rule triggers it.

craigcondit requested a review from wilfred-s August 17, 2023 20:21

craigcondit added 2 commits August 18, 2023 12:03

Cleanup comments

09819af

Cleanup placement

63633a3

pbacsko approved these changes Aug 22, 2023

View reviewed changes

craigcondit closed this in 5a40def Aug 30, 2023

craigcondit deleted the YUNIKORN-1793 branch August 30, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-1793] Handle placement rule and queue changes during initialisation #601

[YUNIKORN-1793] Handle placement rule and queue changes during initialisation #601

craigcondit commented Jul 31, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading

craigcondit commented Aug 3, 2023

wilfred-s left a comment

wilfred-s Aug 7, 2023

craigcondit Aug 8, 2023

craigcondit commented Aug 8, 2023

pbacsko left a comment

craigcondit commented Aug 22, 2023

[YUNIKORN-1793] Handle placement rule and queue changes during initialisation #601

[YUNIKORN-1793] Handle placement rule and queue changes during initialisation #601

Conversation

craigcondit commented Jul 31, 2023

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

craigcondit commented Aug 3, 2023

wilfred-s left a comment

Choose a reason for hiding this comment

wilfred-s Aug 7, 2023

Choose a reason for hiding this comment

craigcondit Aug 8, 2023

Choose a reason for hiding this comment

craigcondit commented Aug 8, 2023

pbacsko left a comment

Choose a reason for hiding this comment

craigcondit commented Aug 22, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading