GREP-291: OnDelete update strategy for PodCliqueSet

[renormalize]

What type of PR is this?

What this PR does / why we need it:

/kind feature
/kind api

Introduces GREP-291.

#291 mentions certain realities of performing day 2 operations on a PodCliqueSet. The author mentions changing nodeAffinity in the pod template of the PodCliqueSet to ensure that pods are not scheduled onto failed nodes.

However, changing nodeAffinity will cause all pods of the PodClique to roll, which is undesirable since there might only be a small subset of pods that were on failed nodes. Therefore, to avoid the rolling of all pods in the PodClique, taking inspiration from Kubernetes StatefulSet, I would like to introduce an OnDelete update strategy for PodCliqueSet, where users can then update the nodeAffinity, and avoid rolling of all the pods in a PodClique.

Which issue(s) this PR fixes:

Fixes #291

Special notes for your reviewer:

N/A

Does this PR introduce a API change?

Introduce the new `spec.updateStrategy` field to specify `OnDelete` update strategy for `PodCliqueSet`

Additional documentation e.g., enhancement proposals, usage docs, etc.:

Introduce GREP-291 for the `OnDelete` update strategy for `PodCliqueSet`

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[renormalize]

In a discussion with the maintainers, they had expressed the following opinions:

• Change the scope of the GREP to define OnDelete for both PodCliques and ScalingGroups. Implementation can be split into different stages.
• Rename RollingUpdate to RollingRecreate, and define what RollingRecreate is. The update strategy currently implemented in grove is not strictly RollingUpdate as referred to by the community, and therefore it would be incorrect to call it that.
• Keep update strategy only at the PodCliqueSet level. In the future, update strategy can be defined at each hierarchy to specify different behavior for different PodCliques/ScalingGroups. This would align with the convention used for topology configuration in PCS.
• ScalingGroups and (standalone) PodCliques both will follow the same strategy. This can be configured hierarchically at all levels later on if needed, as mentioned above.

@unmarshall @sanjaychatterjee @Ronkahn21 @gflarity had suggested the above.

Please refrain from reviews before I adapt the GREP with the above suggestions, thanks.

[sanjaychatterjee]

Mostly looks good to me. Not convinced about the need for the extra condition.

[sanjaychatterjee]

Looks good to me. Thanks!

[renormalize]

After a proof of concept implementation, I had second thoughts about certain fields. I've reworked the GREP based on these opinions.

• I've removed the .spec changes to PodClique and PodCliqueScalingGroup. I feel we should not include the update strategy as a part of the spec for these resources now, and only include them when absolutely necessary. The update strategy can be fetched from the PodCliqueSet resource. I think this aligns with the original comments from the codeowners as well.
• I've renamed the rollingUpdateProgress status fields to updateProgress, to more generically track information about updates no matter what the update strategy is.

[Ronkahn21]

Looks good overall. Thoughts on the API break—do we introduce it now or wait for v1? @sanjaychatterjee @unmarshall

[renormalize]

Looks good overall. Thoughts on the API break—do we introduce it now or wait for v1? @sanjaychatterjee @unmarshall

@Ronkahn21 thanks for your review! Can you please elaborate on what you mean by the API break in this case? My proposed changes are backwards compatible.

We are indeed changing the status API that we expose to consumers by renaming this field.

@julienmancuso Could you let us know if renaming this field will cause issues for dynamo/operator?

[renormalize]

@Ronkahn21 I've introduced UpdateProgress as a separate field after discussion with @sanjaychatterjee and @unmarshall. We deprecate RollingUpdateProgress, and will continue to function as it is currently for the next 2 releases; after which it will be removed.

Once I have an approval for this GREP, I will open the implementation PR for review. Thanks.

[sanjaychatterjee]

Looks good overall. Thoughts on the API break—do we introduce it now or wait for v1? @sanjaychatterjee @unmarshall

@Ronkahn21 thanks for your review! Can you please elaborate on what you mean by the API break in this case? My proposed changes are backwards compatible.

We are indeed changing the status API that we expose to consumers by renaming this field.

@julienmancuso Could you let us know if renaming this field will cause issues for dynamo/operator?

Julien already confirmed on slack that Dynamo is not using the field but still we need to follow deprecation process as part of changelog notifications for two consecutive releases. cc @danbar2