Skip to content

Commit ee2b811

Browse files
committed
allows user-configurable resource requests and limits via MachineOSConfig annotations
1 parent 53e78f3 commit ee2b811

File tree

8 files changed

+446
-16
lines changed

8 files changed

+446
-16
lines changed

Diff for: pkg/controller/build/buildrequest/buildrequest.go

+32-11
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,13 @@ const (
3737
machineConfigJSONFilename string = "machineconfig.json.gz"
3838
)
3939

40+
const (
41+
// The name of the container responsible for running Buildah.
42+
imageBuildContainerName string = "image-build"
43+
// the name of the container responsible for waiting for the build to finish.
44+
waitForDoneContainerName string = "wait-for-done"
45+
)
46+
4047
// Represents the request to build a layered OS image.
4148
type buildRequestImpl struct {
4249
opts BuildRequestOpts
@@ -76,8 +83,13 @@ func (br buildRequestImpl) Opts() BuildRequestOpts {
7683
}
7784

7885
// Creates the Build Job object.
79-
func (br buildRequestImpl) Builder() Builder {
80-
return newBuilder(br.podToJob(br.toBuildahPod()))
86+
func (br buildRequestImpl) Builder() (Builder, error) {
87+
pod, err := br.toBuildahPod()
88+
if err != nil {
89+
return nil, err
90+
}
91+
92+
return newBuilder(br.podToJob(pod)), nil
8193
}
8294

8395
// Takes the configured secrets and creates an ephemeral clone of them, canonicalizing them, if needed.
@@ -284,13 +296,19 @@ func (br buildRequestImpl) podToJob(pod *corev1.Pod) *batchv1.Job {
284296
// context enabled to allow us to use UID 1000, which maps to the UID within
285297
// the official Buildah image.
286298
// nolint:dupl // I don't want to deduplicate this yet since there are still some unknowns.
287-
func (br buildRequestImpl) toBuildahPod() *corev1.Pod {
299+
func (br buildRequestImpl) toBuildahPod() (*corev1.Pod, error) {
300+
resourceReqs, err := getResourceRequirements(br.opts.MachineOSConfig)
301+
if err != nil {
302+
return nil, fmt.Errorf("could not get resource requests for build job %s: %w", br.getBuildName(), err)
303+
}
304+
288305
var httpProxy, httpsProxy, noProxy string
289306
if br.opts.Proxy != nil {
290307
httpProxy = br.opts.Proxy.HTTPProxy
291308
httpsProxy = br.opts.Proxy.HTTPSProxy
292309
noProxy = br.opts.Proxy.NoProxy
293310
}
311+
294312
env := []corev1.EnvVar{
295313
// How many times the build / push steps should be retried. In the future,
296314
// this should be wired up to the MachineOSConfig or other higher-level
@@ -505,11 +523,11 @@ func (br buildRequestImpl) toBuildahPod() *corev1.Pod {
505523
Containers: []corev1.Container{
506524
{
507525
// This container performs the image build / push process.
508-
Name: "image-build",
526+
Name: imageBuildContainerName,
509527
Image: br.opts.Images.MachineConfigOperator,
510528
Env: env,
511529
Command: append(command, buildahBuildScript),
512-
ImagePullPolicy: corev1.PullAlways,
530+
Resources: *resourceReqs.builder,
513531
SecurityContext: securityContext,
514532
// Only attach the buildah-cache volume mount to the buildah container.
515533
VolumeMounts: append(volumeMounts, corev1.VolumeMount{
@@ -523,19 +541,22 @@ func (br buildRequestImpl) toBuildahPod() *corev1.Pod {
523541
// the base OS image (which contains the "oc" binary) to create a
524542
// ConfigMap from the digestfile that Buildah creates, which allows
525543
// us to avoid parsing log files.
526-
Name: "wait-for-done",
527-
Command: append(command, waitScript),
528-
Image: br.opts.OSImageURLConfig.BaseOSContainerImage,
529-
Env: env,
530-
ImagePullPolicy: corev1.PullAlways,
544+
Name: waitForDoneContainerName,
545+
Command: append(command, waitScript),
546+
Image: br.opts.OSImageURLConfig.BaseOSContainerImage,
547+
Env: env,
548+
// This should only use the default resource constraints since this
549+
// container waits before invoking an oc command. In other words, it
550+
// does not take much resources to run this container.
551+
Resources: *resourceReqs.defaults,
531552
SecurityContext: securityContext,
532553
VolumeMounts: volumeMounts,
533554
},
534555
},
535556
ServiceAccountName: "machine-os-builder",
536557
Volumes: volumes,
537558
},
538-
}
559+
}, nil
539560
}
540561

541562
// Populates the labels map for all objects created by imageBuildRequest

Diff for: pkg/controller/build/buildrequest/buildrequest_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,10 @@ func TestBuildRequest(t *testing.T) {
161161
assert.NotContains(t, containerfile, content)
162162
}
163163

164-
buildJob := br.Builder().GetObject().(*batchv1.Job)
164+
builder, err := br.Builder()
165+
assert.NoError(t, err)
166+
167+
buildJob := builder.GetObject().(*batchv1.Job)
165168

166169
_, err = NewBuilder(buildJob)
167170
assert.NoError(t, err)

Diff for: pkg/controller/build/buildrequest/interfaces.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77

88
type BuildRequest interface {
99
Opts() BuildRequestOpts
10-
Builder() Builder
10+
Builder() (Builder, error)
1111
Secrets() ([]*corev1.Secret, error)
1212
ConfigMaps() ([]*corev1.ConfigMap, error)
1313
}

Diff for: pkg/controller/build/buildrequest/resources.go

+176
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
package buildrequest
2+
3+
import (
4+
"fmt"
5+
6+
mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
7+
corev1 "k8s.io/api/core/v1"
8+
"k8s.io/apimachinery/pkg/api/resource"
9+
)
10+
11+
type resourceAnnotationKey string
12+
13+
// Resource request and limit annotation keys
14+
const (
15+
cpuResourceRequestAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/cpu-request"
16+
cpuResourceLimitAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/cpu-limit"
17+
memoryResourceRequestAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/memory-request"
18+
memoryResourceLimitAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/memory-limit"
19+
storageResourceRequestAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/storage-request"
20+
storageResourceLimitAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/storage-limit"
21+
ephemeralStorageResourceRequestAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/ephemeral-storage-request"
22+
ephemeralStorageResourceLimitAnnotationKey resourceAnnotationKey = "machineconfiguration.openshift.io/ephemeral-storage-limit"
23+
24+
// Default CPU request value. Taken from the machine-os-builder deployment
25+
// spec.
26+
defaultCPURequest string = "20m"
27+
// Default memory request value. Taken from the machine-os-builder deployment
28+
// spec.
29+
defaultMemoryRequest string = "50Mi"
30+
)
31+
32+
// Holds all of the functions required for getting the resource requests. This
33+
// allows to keep these functions cleanly separated while simultaneously
34+
// allowing a single linear path through them.
35+
type resources struct {
36+
mosc *mcfgv1.MachineOSConfig
37+
}
38+
39+
// The finalized resource requirements for both the builder container as well
40+
// as the waiting container.
41+
type resourceRequirements struct {
42+
builder *corev1.ResourceRequirements
43+
defaults *corev1.ResourceRequirements
44+
}
45+
46+
// The main entrypoint into getting the resource requirements for the
47+
// BuildRequest.
48+
func getResourceRequirements(mosc *mcfgv1.MachineOSConfig) (*resourceRequirements, error) {
49+
r := &resources{mosc: mosc}
50+
51+
builder, err := r.getResourceRequirements()
52+
if err != nil {
53+
return nil, fmt.Errorf("could not get builder resource requirements: %w", err)
54+
}
55+
56+
defaults, err := r.getDefaultResourceRequirements()
57+
if err != nil {
58+
return nil, fmt.Errorf("could not get default resource requirements: %w", err)
59+
}
60+
61+
return &resourceRequirements{
62+
builder: builder,
63+
defaults: defaults,
64+
}, nil
65+
}
66+
67+
// Gets the resource requirements for the build pod from the MachineOSConfig
68+
// annotations or falls back to the default hard-coded values we specified.
69+
func (r *resources) getResourceRequirements() (*corev1.ResourceRequirements, error) {
70+
defaults, err := r.getDefaultResourceRequirements()
71+
if err != nil {
72+
return nil, err
73+
}
74+
75+
userProvided, err := r.getUserProvidedResourceRequirements()
76+
if err != nil {
77+
return nil, err
78+
}
79+
80+
// User-provided requests and limits override the defaults.
81+
for key, val := range userProvided.Requests {
82+
defaults.Requests[key] = val
83+
}
84+
85+
for key, val := range userProvided.Limits {
86+
defaults.Limits[key] = val
87+
}
88+
89+
return defaults, nil
90+
}
91+
92+
// Gets the default resource requirements.
93+
func (r *resources) getDefaultResourceRequirements() (*corev1.ResourceRequirements, error) {
94+
qtys, err := getResourceList(map[corev1.ResourceName]string{
95+
corev1.ResourceCPU: defaultCPURequest,
96+
corev1.ResourceMemory: defaultMemoryRequest,
97+
})
98+
99+
if err != nil {
100+
return nil, err
101+
}
102+
103+
return &corev1.ResourceRequirements{
104+
Requests: qtys,
105+
Limits: corev1.ResourceList{},
106+
}, nil
107+
}
108+
109+
// Gets the user-provided resource requirements, if any.
110+
func (r *resources) getUserProvidedResourceRequirements() (*corev1.ResourceRequirements, error) {
111+
userProvidedRequests, err := r.getResourceListFromAnnotations(map[corev1.ResourceName]resourceAnnotationKey{
112+
corev1.ResourceCPU: cpuResourceRequestAnnotationKey,
113+
corev1.ResourceMemory: memoryResourceRequestAnnotationKey,
114+
corev1.ResourceStorage: storageResourceRequestAnnotationKey,
115+
corev1.ResourceEphemeralStorage: ephemeralStorageResourceRequestAnnotationKey,
116+
})
117+
118+
if err != nil {
119+
return nil, err
120+
}
121+
122+
userProvidedLimits, err := r.getResourceListFromAnnotations(map[corev1.ResourceName]resourceAnnotationKey{
123+
corev1.ResourceCPU: cpuResourceLimitAnnotationKey,
124+
corev1.ResourceMemory: memoryResourceLimitAnnotationKey,
125+
corev1.ResourceStorage: storageResourceLimitAnnotationKey,
126+
corev1.ResourceEphemeralStorage: ephemeralStorageResourceLimitAnnotationKey,
127+
})
128+
129+
if err != nil {
130+
return nil, err
131+
}
132+
133+
return &corev1.ResourceRequirements{
134+
Requests: userProvidedRequests,
135+
Limits: userProvidedLimits,
136+
}, nil
137+
}
138+
139+
// Gets the ResourceList from the MachineOSConfig annotation.
140+
func (r *resources) getResourceListFromAnnotations(in map[corev1.ResourceName]resourceAnnotationKey) (corev1.ResourceList, error) {
141+
out := corev1.ResourceList{}
142+
143+
for name, annoKey := range in {
144+
val, ok := r.mosc.Annotations[string(annoKey)]
145+
if !ok {
146+
continue
147+
}
148+
149+
parsed, err := resource.ParseQuantity(val)
150+
if err != nil {
151+
return nil, fmt.Errorf("could not parse resource annotation %q value %q: %w", annoKey, val, err)
152+
}
153+
154+
out[name] = parsed
155+
}
156+
157+
return out, nil
158+
}
159+
160+
// Parses the resource string for each resource name into a resource.Quantity
161+
// and inserts it into a ResourceList. This is needed because the
162+
// resource.Quantity values are private.
163+
func getResourceList(in map[corev1.ResourceName]string) (corev1.ResourceList, error) {
164+
out := corev1.ResourceList{}
165+
166+
for name, qtyString := range in {
167+
qty, err := resource.ParseQuantity(qtyString)
168+
if err != nil {
169+
return nil, err
170+
}
171+
172+
out[name] = qty
173+
}
174+
175+
return out, nil
176+
}

0 commit comments

Comments
 (0)