[Azure][Cluster] Only set evictionPolicy to Delete when spot is used

[kekulai-fredchang]

previous update assumed evictionPolicy on all nodes as 'Delete', which yields error for those azure users that do not launch a Spot instance.

@gramhagen , @eisber , @ijrsvt

Why are these changes needed?

Since Spot instance availability is low in azure, many users will launch with regular instance and encounter error.

added conditional logic in master azure json file to check if priority is indeed Spot , if so, then yield evictionPolicy as 'Delete', otherwise yield json('null') to skip this policy.

Related issue number

fixes #46198

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[gramhagen]

This makes sense. Although if the user provides this parameter, I would default to use that first before filling in a default. I'm assuming it's null if not provided? If so you should be able to check if the policy is null and the priority is for spot then use delete otherwise keep the parameter as is.

[frdchang]

This makes sense. Although if the user provides this parameter, I would default to use that first before filling in a default. I'm assuming it's null if not provided? If so you should be able to check if the policy is null and the priority is for spot then use delete otherwise keep the parameter as is.

@gramhagen new logic was added to check if the user supplied an eviction policy & a spot instance is used, otherwise if spot instance is regular, it passes null for eviction policy.

[gramhagen]

does this work? my eyes are getting crossed looking at the parentheses in the if condition 😵‍💫

[gramhagen]

oops, didn't realize there was already a default for this, i'd suggest putting this back and just catching the error case below. that should also allow people to maintain any overrides (just not an invalid one)

Suggested change

	"defaultValue": "json('null')",
	"defaultValue": "Delete",

[gramhagen]

Suggested change

	"evictionPolicy": "[if(and(equals(parameters('priority'), 'Spot'), or(equals(parameters('evictionPolicy'), ''), equals(parameters('evictionPolicy'), json('null')))), 'Delete', parameters('evictionPolicy'))]",
	"evictionPolicy": "[if(and(equals(parameters('priority'), 'Spot'), equals(parameters('evictionPolicy'), 'Delete')), json('null'), parameters('evictionPolicy'))]",

[anyscalesam]

@kekulai-fredchang can you address comments here?

[frdchang]

@kekulai-fredchang can you address comments here?

I looked at @gramhagen suggestions, and from my limited azure knowledge it did not seem correct because it will pass json('null') even when 'Delete' is desired.

I redid the logic as follows, which allows both spot instances with delete default and user defined pass through, and regular instances with evictionPolicy set to json('null').

IF priority IS 'Spot' THEN
    IF evictionPolicy IS empty OR null THEN
        SET evictionPolicy TO 'Delete'
    ELSE
        USE user-defined evictionPolicy
    ENDIF
ELSE
    USE user-defined evictionPolicy
ENDIF 

[gramhagen]

ok, lgtm

[mjd3]

I was running into this same issue, but I think this can be a lot simpler. The evictionPolicy parameter already defaults to Delete in the template, so you can just set it to an empty string in the case of Regular priority and use the user-defined evictionPolicy if it is provided in the Spot priority case. If the user doesn't specify an evictionPolicy and the priority is Spot then we default to Delete anyway. I just replaced this line with:

"evictionPolicy": "[if(equals(parameters('priority'), 'Spot'), parameters('evictionPolicy'), '')]",

and it works for me.

[gramhagen]

@mjd3 that does seem like the easiest option!

[mjd3]

Also, as a workaround until this PR is merged in, you can add evictionPolicy: null in your cluster cfg yaml file for any nodes with priority: Regular.

[kekulai-fredchang]

Also, as a workaround until this PR is merged in, you can add evictionPolicy: null in your cluster cfg yaml file for any nodes with priority: Regular.

good point @mjd3.

This either should be annotated in the docs or have this PR merged.

[mjd3]

@kekulai-fredchang do you agree with the suggestion above? From looking back, it seems like my suggestion mirrors @gramhagen's initial suggestion too. If so, can you update this PR? Would love to get this one merged in.

[kekulai-fredchang]

@kekulai-fredchang do you agree with the suggestion above? From looking back, it seems like my suggestion mirrors @gramhagen's initial suggestion too. If so, can you update this PR? Would love to get this one merged in.

Agreed.

[mjd3]

Thanks @kekulai-fredchang! @anyscalesam this should be ready to merge.

[anyscalesam]

@hongchaodeng can you please take a look?

[bthananjeyan]

Sorry for introducing this issue in the earlier PR. I confirmed that this fix works for me locally. @ericl @architkulkarni @hongchaodeng would you be able to review this PR?

[kevin85421]

I don't have an Azure account to test this PR, but some folks have already verified it. Hence, I approve this PR.

[bthananjeyan]

@kekulai-fredchang It looks like you could run these instructions to pass the DCO check: https://github.com/ray-project/ray/pull/46959/checks?check_run_id=30602666160

Thanks for fixing this issue.

[kevin85421]

It looks like you could run these instructions to pass the DCO check

The DCO check failing is fine with me. We just need to make sure the premerge check passes.

[kekulai-fredchang]

@bthananjeyan @kevin85421 I updated the branch to include signoff