Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit ceb45c6

Browse files
cheesesashimicheesesashimi
committedMar 26, 2025··
allows user-configurable resource requests and limits via MachineOSConfig annotations
1 parent 53e78f3 commit ceb45c6

File tree

8 files changed

+445
-16
lines changed

8 files changed

+445
-16
lines changed
 

‎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

‎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)

‎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
}

‎pkg/controller/build/buildrequest/resources.go

+175
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
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.
162+
func getResourceList(in map[corev1.ResourceName]string) (corev1.ResourceList, error) {
163+
out := corev1.ResourceList{}
164+
165+
for name, qtyString := range in {
166+
qty, err := resource.ParseQuantity(qtyString)
167+
if err != nil {
168+
return nil, err
169+
}
170+
171+
out[name] = qty
172+
}
173+
174+
return out, nil
175+
}

‎pkg/controller/build/buildrequest/resources_test.go

+224
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
package buildrequest
2+
3+
import (
4+
"testing"
5+
6+
mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
batchv1 "k8s.io/api/batch/v1"
10+
corev1 "k8s.io/api/core/v1"
11+
)
12+
13+
// Validates that resource requirements are set correctly, based upon whether
14+
// they are user-provided or the system defaults. This is done by ensuring that
15+
// the resource request annotations from the MachineOSConfig are honored. These
16+
// are checked both in isolation as well as on a per-container basis.
17+
//
18+
// In the future, the MachineOSConfig API should be modified to allow the
19+
// corev1.ResourceRequirements struct to be embedded within it. That would make
20+
// this test mostly moot with the exception of allowing default values.
21+
func TestResources(t *testing.T) {
22+
t.Parallel()
23+
24+
defaultResourceReq := &corev1.ResourceRequirements{
25+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
26+
corev1.ResourceCPU: defaultCPURequest,
27+
corev1.ResourceMemory: defaultMemoryRequest,
28+
}),
29+
Limits: corev1.ResourceList{},
30+
}
31+
32+
req, err := getResourceRequirements(&mcfgv1.MachineOSConfig{})
33+
require.NoError(t, err)
34+
35+
assert.Equal(t, req.defaults, defaultResourceReq)
36+
37+
testCases := []struct {
38+
name string
39+
annotations map[resourceAnnotationKey]string
40+
expectedResources *corev1.ResourceRequirements
41+
errExpected bool
42+
}{
43+
{
44+
name: "No resource requests",
45+
annotations: map[resourceAnnotationKey]string{},
46+
expectedResources: defaultResourceReq,
47+
},
48+
{
49+
name: "CPU request only",
50+
annotations: map[resourceAnnotationKey]string{
51+
cpuResourceRequestAnnotationKey: "750m",
52+
},
53+
expectedResources: &corev1.ResourceRequirements{
54+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
55+
corev1.ResourceCPU: "750m",
56+
corev1.ResourceMemory: defaultMemoryRequest,
57+
}),
58+
Limits: corev1.ResourceList{},
59+
},
60+
},
61+
{
62+
name: "Memory request only",
63+
annotations: map[resourceAnnotationKey]string{
64+
memoryResourceRequestAnnotationKey: "2Gi",
65+
},
66+
expectedResources: &corev1.ResourceRequirements{
67+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
68+
corev1.ResourceCPU: defaultCPURequest,
69+
corev1.ResourceMemory: "2Gi",
70+
}),
71+
Limits: corev1.ResourceList{},
72+
},
73+
},
74+
{
75+
name: "Requests only",
76+
annotations: map[resourceAnnotationKey]string{
77+
cpuResourceRequestAnnotationKey: "750m",
78+
memoryResourceRequestAnnotationKey: "2Gi",
79+
storageResourceRequestAnnotationKey: "20Gi",
80+
ephemeralStorageResourceRequestAnnotationKey: "20Gi",
81+
},
82+
expectedResources: &corev1.ResourceRequirements{
83+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
84+
corev1.ResourceCPU: "750m",
85+
corev1.ResourceMemory: "2Gi",
86+
corev1.ResourceStorage: "20Gi",
87+
corev1.ResourceEphemeralStorage: "20Gi",
88+
}),
89+
Limits: corev1.ResourceList{},
90+
},
91+
},
92+
{
93+
name: "Limits only",
94+
annotations: map[resourceAnnotationKey]string{
95+
cpuResourceLimitAnnotationKey: "750m",
96+
memoryResourceLimitAnnotationKey: "2Gi",
97+
storageResourceLimitAnnotationKey: "20Gi",
98+
ephemeralStorageResourceLimitAnnotationKey: "20Gi",
99+
},
100+
expectedResources: &corev1.ResourceRequirements{
101+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
102+
corev1.ResourceCPU: defaultCPURequest,
103+
corev1.ResourceMemory: defaultMemoryRequest,
104+
}),
105+
Limits: mustGetResourceList(map[corev1.ResourceName]string{
106+
corev1.ResourceCPU: "750m",
107+
corev1.ResourceMemory: "2Gi",
108+
corev1.ResourceStorage: "20Gi",
109+
corev1.ResourceEphemeralStorage: "20Gi",
110+
}),
111+
},
112+
},
113+
{
114+
name: "Requests and limits",
115+
annotations: map[resourceAnnotationKey]string{
116+
cpuResourceRequestAnnotationKey: "750m",
117+
memoryResourceRequestAnnotationKey: "2Gi",
118+
storageResourceRequestAnnotationKey: "20Gi",
119+
ephemeralStorageResourceRequestAnnotationKey: "20Gi",
120+
cpuResourceLimitAnnotationKey: "750m",
121+
memoryResourceLimitAnnotationKey: "2Gi",
122+
storageResourceLimitAnnotationKey: "20Gi",
123+
ephemeralStorageResourceLimitAnnotationKey: "20Gi",
124+
},
125+
expectedResources: &corev1.ResourceRequirements{
126+
Requests: mustGetResourceList(map[corev1.ResourceName]string{
127+
corev1.ResourceCPU: "750m",
128+
corev1.ResourceMemory: "2Gi",
129+
corev1.ResourceStorage: "20Gi",
130+
corev1.ResourceEphemeralStorage: "20Gi",
131+
}),
132+
Limits: mustGetResourceList(map[corev1.ResourceName]string{
133+
corev1.ResourceCPU: "750m",
134+
corev1.ResourceMemory: "2Gi",
135+
corev1.ResourceStorage: "20Gi",
136+
corev1.ResourceEphemeralStorage: "20Gi",
137+
}),
138+
},
139+
},
140+
{
141+
name: "Invalid resource quantity",
142+
annotations: map[resourceAnnotationKey]string{
143+
cpuResourceRequestAnnotationKey: "invalid-value",
144+
},
145+
errExpected: true,
146+
},
147+
{
148+
name: "Empty annotation value",
149+
annotations: map[resourceAnnotationKey]string{
150+
cpuResourceRequestAnnotationKey: "",
151+
},
152+
// TODO: What happens when a resource field is requested but is empty?
153+
// This should mimic that behavior.
154+
errExpected: true,
155+
},
156+
}
157+
158+
for _, testCase := range testCases {
159+
testCase := testCase
160+
t.Run(testCase.name, func(t *testing.T) {
161+
t.Parallel()
162+
163+
t.Run("ResourceRequests", func(t *testing.T) {
164+
t.Parallel()
165+
166+
opts := getBuildRequestOpts()
167+
168+
if testCase.annotations != nil {
169+
for key, val := range testCase.annotations {
170+
opts.MachineOSConfig.Annotations[string(key)] = val
171+
}
172+
}
173+
174+
resourceReq, err := getResourceRequirements(opts.MachineOSConfig)
175+
if testCase.errExpected {
176+
assert.Error(t, err)
177+
return
178+
}
179+
180+
assert.Equal(t, testCase.expectedResources, resourceReq.builder)
181+
})
182+
183+
t.Run("BuildRequest", func(t *testing.T) {
184+
t.Parallel()
185+
186+
opts := getBuildRequestOpts()
187+
188+
if testCase.annotations != nil {
189+
for key, val := range testCase.annotations {
190+
opts.MachineOSConfig.Annotations[string(key)] = val
191+
}
192+
}
193+
194+
br := newBuildRequest(opts)
195+
196+
builder, err := br.Builder()
197+
if testCase.errExpected {
198+
assert.Error(t, err)
199+
return
200+
}
201+
202+
assert.NoError(t, err)
203+
204+
buildJob := builder.GetObject().(*batchv1.Job)
205+
// TODO(zzlotnik): This field is still alpha and is not respected when the
206+
// PodLevelResources feature gate is not enabled. Additionally, this
207+
// field only accepts CPU and memory values, so non-CPU / memory values
208+
// should be stripped.
209+
//
210+
// assert.Equal(t, testCase.expectedResources, buildJob.Spec.Template.Spec.Resources)
211+
212+
for _, container := range buildJob.Spec.Template.Spec.Containers {
213+
if container.Name == imageBuildContainerName {
214+
assert.Equal(t, *testCase.expectedResources, container.Resources)
215+
}
216+
217+
if container.Name == waitForDoneContainerName {
218+
assert.Equal(t, *defaultResourceReq, container.Resources)
219+
}
220+
}
221+
})
222+
})
223+
}
224+
}

‎pkg/controller/build/imagebuilder/base.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -197,5 +197,5 @@ func (b *baseImageBuilder) prepareForBuild(ctx context.Context) (buildrequest.Bu
197197

198198
b.buildrequest = br
199199

200-
return br.Builder(), nil
200+
return br.Builder()
201201
}

‎pkg/controller/build/imagebuilder/preparer_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@ func TestPreparer(t *testing.T) {
6060
// c3 uses the Builder object from the BuildRequest instead so that we can
6161
// ensure that ephemeral build objects will be removed even if only the Builder object
6262
// is available.
63-
c3 := newCleanerFromBuilder(kubeclient, mcfgclient, br3.Builder())
63+
64+
build, err := br3.Builder()
65+
require.NoError(t, err)
66+
c3 := newCleanerFromBuilder(kubeclient, mcfgclient, build)
6467

6568
// Cleanup the ephemeral objects from the first MachineOSBuild.
6669
assert.NoError(t, c1.Clean(ctx))

‎pkg/controller/build/osbuildcontroller_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,10 @@ func TestOSBuildControllerReconcilesJobsAfterRestart(t *testing.T) {
639639
br, err := buildrequest.NewBuildRequestFromAPI(ctx, kubeclient, mcfgclient, apiMosb, mosc)
640640
require.NoError(t, err)
641641

642-
buildJob := br.Builder().GetObject().(*batchv1.Job)
642+
builder, err := br.Builder()
643+
require.NoError(t, err)
644+
645+
buildJob := builder.GetObject().(*batchv1.Job)
643646

644647
_, err = kubeclient.BatchV1().Jobs(ctrlcommon.MCONamespace).Create(ctx, buildJob, metav1.CreateOptions{})
645648
require.NoError(t, err)

0 commit comments

Comments
 (0)
Please sign in to comment.