New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve NPE merging yaml when resource requests/limits are not set #310

Merged
merged 4 commits into from Apr 24, 2018

Conversation

Projects
None yet
3 participants
@oliverlockwood

oliverlockwood commented Apr 16, 2018

If you have a PodTemplate with a ContainerSpec, and want to also set the yaml field to apply some things that aren't directly configurable through this plugin, then it's possible to hit NPEs if resource requests / limits aren't set. (In Kubernetes, it's perfectly valid not to set resource request / limit values for cpu and memory.)

This PR defensively codes to cover that case.

Function<ResourceRequirements, Map<String, Quantity>> resourceTypeMapper,
String field) {
Quantity templateField = resourceTypeMapper.apply(template.getResources()).get(field);
if (templateField != null && !Strings.isNullOrEmpty(templateField.getAmount())) {

This comment has been minimized.

This comment has been minimized.

@oliverlockwood

oliverlockwood Apr 17, 2018

@carlossg thanks for the feedback. I've addressed this in the latest commit.

@carlossg carlossg changed the title from Resolve NPE in pod yaml templating to Resolve NPE merging yaml when resource requests/limits are not set Apr 24, 2018

@carlossg carlossg merged commit cf7e719 into jenkinsci:master Apr 24, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@oliverlockwood oliverlockwood deleted the oliverlockwood:resolve-npe-in-pod-yaml-templating branch Apr 24, 2018

@therc

This comment has been minimized.

therc commented Jun 12, 2018

Is this the proper fix? Either this change or something that was merged later results in objects that match the schema, but are rejected by the API server at validation time. The test case checks the former, but not the latter (which would require the Kubernetes Golang codebase). I suspect the .orElse(new Quantity("")) statement.

This is what I am seeing if I don't specify all limits and requests, with line splits added for clarity:

Received status: Status(apiVersion=v1, code=400, details=null, kind=Status,
 message=Pod in version "v1" cannot be handled as a Pod: v1.Pod: Spec: v1.PodSpec:
 Containers: []v1.Container: v1.Container: Resources: v1.ResourceRequirements: Limits:
 unmarshalerDecoder: quantities must match the regular expression
 '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$', error found in #10 byte of
 ...|:{"cpu":"","memory":|...,
 bigger context ...|ubectl","ports":[],"resources":{"limits":{"cpu":"","memory":""},"requests":{"cpu":"","memory":""}},"|...,
 metadata=ListMeta(resourceVersion=null, selfLink=null, additionalProperties={}),
 reason=BadRequest, status=Failure, additionalProperties={}).

The object might be well-formed, but the server rejects it at validation time and returns a 400, at least with Kubernetes 1.10.

Proof that empty strings are problematic:

$ kubectl apply -f test.yaml
pod "podtest" created
$ kubectl apply -f test2.yaml
Error from server (BadRequest): error when creating "test2.yaml":
 Pod in version "v1" cannot be handled as a Pod: v1.Pod: Spec: v1.PodSpec:
 Containers: []v1.Container: v1.Container: Resources: v1.ResourceRequirements: Limits:
 unmarshalerDecoder: quantities must match the regular expression
 '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$', error found in #10 byte of
 ...|:{"cpu":""}}}]}}|...,
 bigger context ...|","name":"busybox","resources":{"limits":{"cpu":""}}}]}}|...
$ diff test.yaml test2.yaml
4c4
<   name: podtest
---
>   name: podtest2
10a11,13
>     resources:
>       limits:
>         cpu: ""

Reverting to 1.5.2 results in NPEs, of course. 1.6.0 has the same validation errors. I can file an issue in the tracker with all the above.

@carlossg

This comment has been minimized.

carlossg commented Jun 12, 2018

I think orElse(null) could be a better approach if you can test and submit a PR

@therc

This comment has been minimized.

therc commented Jun 12, 2018

I can try, but this is going to be a bit rough, since I haven't written any Java in 15 years. :)

@therc

This comment has been minimized.

therc commented Jun 13, 2018

Ok, #342 seems to work, but it wasn't fun to write using only Chrome as the IDE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment