fix: handle multiple compute classes in a project #2459
Conversation
keyallis
requested review from
thedadams,
njhale,
StrongMonkey,
sosodev,
iwilltry42,
g-linville and
tylerslaton
There was a problem hiding this comment.
The two failing tests here are testing that an app cannot use a compute class that is not for its region. The reason they are failing is that the app is not specifying a region, so the "wrong" compute class is no longer being included.
The tests should be updated such that the app specifically tries to use the compute class that is not for its region. That should fail (which would mean the test would pass).
| @@ -584,13 +584,20 @@ func (s *Validator) checkScheduling(ctx context.Context, params *apiv1.App, proj | |||
| return append(validationErrors, field.Invalid(field.NewPath("spec", "image"), params.Spec.Image, fmt.Sprintf("error listing compute classes: %v", err))) | |||
| } | |||
|
|
|||
| filteredComputeClasses := new(apiv1.ComputeClassList) | |||
| for _, cc := range computeClasses.Items { | |||
| if slices.Contains(cc.SupportedRegions, params.Spec.Region) || slices.Contains(cc.SupportedRegions, defaultRegion) { | |||
There was a problem hiding this comment.
If the params.Spec.Region != "", then we don't want to check the defaultRegion here.
However, we also need to collect all the compute classes that may be specified by a user or acornfile. In order of precidence:
params.Spec.ComputeClassesapp.Status.AppSpec.{Containers,Jobs}[].ComputeClassas well asapp.Status.AppSpec.{Containers,Jobs}[].Sidecars[].ComputeClass- The default for the region.
By including all of these here, the checks below will catch if an app is trying to use a compute class that is not for the region.
In order to check 2, you would need the imageDetails.AppSpec from where this method is called.
| if !tt.noComputeClass { | ||
| for _, computeClass := range computeClasses { | ||
| if err := kclient.Create(ctx, computeClass); err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
nit: you can use require.NoError(t, err) to end the testcase early.
There was a problem hiding this comment.
The test structure was lifted from the function below this one so I'd prefer not to change much about it other than the looping. I thought about combining the two blocks together as well but figured it would be easier to review if it was just it's own function
…default for a project spanning multiple regions Signed-off-by: Oscar Ward <[email protected]>
…t set Signed-off-by: Oscar Ward <[email protected]>
Signed-off-by: Oscar Ward <[email protected]>
Signed-off-by: Oscar Ward <[email protected]>
… class, update unit tests Signed-off-by: Oscar Ward <[email protected]>
Signed-off-by: Oscar Ward <[email protected]>
Signed-off-by: Oscar Ward <[email protected]>
integration/run/run_test.go
Outdated
| app, err := c.AppRun(ctx, image.ID, &client.AppRunOptions{Name: testcase}) | ||
| app, err := c.AppRun(ctx, image.ID, &client.AppRunOptions{ | ||
| Name: testcase, | ||
| Region: apiv1.LocalRegion, |
There was a problem hiding this comment.
Why is this changing? We should be able to continue to use the default region here instead of specifying one, correct?
There was a problem hiding this comment.
Yes, that was left over from previous versions of this PR, I've reverted it.
Signed-off-by: Oscar Ward <[email protected]>
…2465) This reverts commit ce9166e. Signed-off-by: Oscar Ward <[email protected]>
acorn-io/manager#1819
Handle the case of multiple compute classes being configured as default for a project spanning multiple regions.
Checklist
This is a title (#1216). Here's an example