Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Resolved offerings #2369

Merged
merged 8 commits into from
Dec 13, 2023
Merged

Resolved offerings #2369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4aca73a
Refactor defaults to resolvedOfferings
g-linville Dec 5, 2023
5e2b809
PR changes
g-linville Dec 10, 2023
9d221f1
PR changes (WIP)
g-linville Dec 11, 2023
abe2384
PR changes
g-linville Dec 12, 2023
9f7cc65
Remove accidentally committed file
g-linville Dec 12, 2023
00b4f52
Fix lint issue and remove unneeded function parameter
g-linville Dec 12, 2023
337e1aa
Fix issue finding bound volumes
g-linville Dec 12, 2023
2dfc0ed
Last changes for suggestions from Donnie
g-linville Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/docs/40-admin/03-computeclasses.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ affinity: # The same affinity fields for Pods
operator: In
values:
- bar
supportedRegions: ["local"] # should always be set to ["local"]
```

If `memory.min`, `memory.max`, `memory.values`, `affinity`, and `tolerations` are not given, then there are no scheduling rules for workloads using the compute class.
Expand Down
5 changes: 1 addition & 4 deletions pkg/apis/api.acorn.io/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@ func (in *App) GetStopped() bool {
}

func (in *App) GetRegion() string {
if in.Spec.Region != "" {
return in.Spec.Region
}
return in.Status.Defaults.Region
return in.Status.ResolvedOfferings.Region
StrongMonkey marked this conversation as resolved.
Show resolved Hide resolved
}

type Acornfile v1.AppSpec
Expand Down
38 changes: 19 additions & 19 deletions pkg/apis/internal.acorn.io/v1/appinstance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type AppInstanceCondition string

var (
AppInstanceConditionDefined = "defined"
AppInstanceConditionDefaults = "defaults"
AppInstanceConditionResolvedOfferings = "resolved-offerings"
AppInstanceConditionScheduling = "scheduling"
AppInstanceConditionNamespace = "namespace"
AppInstanceConditionParsed = "parsed"
Expand Down Expand Up @@ -52,24 +52,18 @@ type AppInstance struct {
}

func (in *AppInstance) HasRegion(region string) bool {
return in.Status.Defaults.Region == region || in.Spec.Region == region
return in.Status.ResolvedOfferings.Region == region || in.Spec.Region == region
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return in.Status.ResolvedOfferings.Region == region || in.Spec.Region == region
return in.Status.ResolvedOfferings.Region == region

}

func (in *AppInstance) GetRegion() string {
if in.Spec.Region != "" {
return in.Spec.Region
if in.Status.ResolvedOfferings.Region != "" {
return in.Status.ResolvedOfferings.Region
}
return in.Status.Defaults.Region
return in.Spec.Region
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just return in.Status.ResolvedOfferings.Region here.

}

func (in *AppInstance) SetDefaultRegion(region string) {
if in.Spec.Region == "" {
if in.Status.Defaults.Region == "" {
in.Status.Defaults.Region = region
}
} else {
in.Status.Defaults.Region = ""
}
in.Status.ResolvedOfferings.Region = region
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here was that we only set the region here if it is blank. Some of our code relies on this (e.g. I can call SetDefaultRegion as many times as I want and it will do the right thing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thedadams let me know if this function looks good to you now. I am still a little bit confused as to how it is supposed to work to be honest.

}

func (in *AppInstance) ShortID() string {
Expand Down Expand Up @@ -209,7 +203,7 @@ type AppInstanceStatus struct {
AppStatus AppStatus `json:"appStatus,omitempty"`
Scheduling map[string]Scheduling `json:"scheduling,omitempty"`
Conditions []Condition `json:"conditions,omitempty"`
Defaults Defaults `json:"defaults,omitempty"`
ResolvedOfferings ResolvedOfferings `json:"resolvedOfferings,omitempty"`
Summary CommonSummary `json:"summary,omitempty"`
Permissions []Permissions `json:"permissions,omitempty"` // Permissions given to this appInstance (only containers within, not nested Acorns/Services)
DeniedConsumerPermissions []Permissions `json:"deniedConsumerPermissions,omitempty"` // Permissions given to this appInstance by a consumed service, which it is not authorized to have
Expand All @@ -229,19 +223,25 @@ type AppStatusStaged struct {
ImageAllowed *bool `json:"imageAllowed,omitempty"`
}

type Defaults struct {
VolumeSize *resource.Quantity `json:"volumeSize,omitempty"`
Volumes map[string]VolumeDefault `json:"volumes,omitempty"`
Memory map[string]*int64 `json:"memory,omitempty"`
Region string `json:"region,omitempty"`
type ResolvedOfferings struct {
Volumes map[string]VolumeResolvedOffering `json:"volumes,omitempty"`
VolumeSize *resource.Quantity `json:"volumeSize,omitempty"`
Containers map[string]ContainerResolvedOffering `json:"containers,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of storing a map of container name to memory request, we're storing a map of container name to a struct containing the following info:

  • memory request
  • computeclass name
  • cpuscaler value

Region string `json:"region,omitempty"`
}

type VolumeDefault struct {
type VolumeResolvedOffering struct {
Class string `json:"class,omitempty"`
Size Quantity `json:"size,omitempty"`
AccessModes AccessModes `json:"accessModes,omitempty"`
}

type ContainerResolvedOffering struct {
Class string `json:"class,omitempty"`
Memory *int64 `json:"memory,omitempty"`
CPUScaler *float64 `json:"cpuScaler,omitempty"`
}

type Scheduling struct {
Requirements corev1.ResourceRequirements `json:"requirements,omitempty"`
Affinity *corev1.Affinity `json:"affinity,omitempty"`
Expand Down
143 changes: 80 additions & 63 deletions pkg/apis/internal.acorn.io/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/cli/testdata/TestAll/acorn_all_-o_json.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ ACORNS:
},
"appSpec": {},
"appStatus": {},
"defaults": {},
"resolvedOfferings": {},
"summary": {}
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/testdata/TestAll/acorn_all_-o_yaml.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ items:
appSpec: {}
appStatus: {}
columns: {}
defaults: {}
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down
13 changes: 13 additions & 0 deletions pkg/controller/appdefinition/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,12 +597,25 @@ func containerAnnotation(container v1.Container) string {
return string(json)
}

func resolvedOfferingsAnnotation(appInstance *v1.AppInstance, container v1.Container) string {
if resolved, exists := appInstance.Status.ResolvedOfferings.Containers[container.Name]; exists {
data, _ := convert.EncodeToMap(resolved)
j, _ := json.Marshal(data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of ignoring errors here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yeah, should always handle error in case of bad inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return string(j)
}
return ""
}

func podAnnotations(appInstance *v1.AppInstance, container v1.Container) map[string]string {
annotations := map[string]string{
labels.AcornContainerSpec: containerAnnotation(container),
}
addPrometheusAnnotations(annotations, container)

if offerings := resolvedOfferingsAnnotation(appInstance, container); offerings != "" {
annotations[labels.AcornContainerResolvedOfferings] = offerings
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to put this annotation onto the ContainerReplica so that I can determine the name of the container's ComputeClass in something else I am working on, where the only info that I have is the ContainerReplica and not the corresponding AppInstance.


images := map[string]string{}
addImageAnnotations(images, appInstance, container)

Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/appdefinition/testdata/TestComputeMem.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ status:
appSpec: {}
appStatus: {}
columns: {}
defaults: {}
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down Expand Up @@ -157,7 +157,7 @@ status:
appSpec: {}
appStatus: {}
columns: {}
defaults: {}
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down Expand Up @@ -200,8 +200,8 @@ status:
status: "True"
success: true
type: defined
defaults: {}
namespace: app-created-namespace
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/appdefinition/testdata/acorn/basic/expected.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ status:
appSpec: {}
appStatus: {}
columns: {}
defaults: {}
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down Expand Up @@ -212,8 +212,8 @@ status:
status: "True"
success: true
type: defined
defaults: {}
namespace: app-created-namespace
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/appdefinition/testdata/acorn/labels/expected.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ status:
appSpec: {}
appStatus: {}
columns: {}
defaults: {}
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down Expand Up @@ -254,8 +254,8 @@ status:
status: "True"
success: true
type: defined
defaults: {}
namespace: app-created-namespace
resolvedOfferings: {}
staged:
appImage:
buildContext: {}
Expand Down
Loading