Preventing build from inheriting master log level #9905

jupierce · 2016-07-18T15:49:12Z

Fixes #8738
This implementation:

Prevents builds from inheriting the logging level of the master
Does not break any existing use of BuildDefaults (and does not change how proxy information is managed)
Ultimate default for builder is --loglevel=1 . This can be overridden multiple ways. In order of decreasing precedence:
- Specifying --build-loglevel on start-build (except on binary builds, which has always been the case since the function relies on env variables)
- BUILD_LOGLEVEL in buildconfig
- BuildDefaults/env/BUILD_LOGLEVEL variable in master-config.

Example master-config entry:

kubernetesMasterConfig:
  admissionConfig:
    pluginConfig:
      BuildDefaults:
        configuration:
          apiVersion: v1
          kind: BuildDefaultsConfig
          env:
          - name: "BUILD_LOGLEVEL"
            value: "5"

jupierce · 2016-07-18T15:51:35Z

Commit also includes minor modifications of sti -> s2i

jupierce · 2016-07-18T15:54:36Z

@bparees @smarterclayton @csrwng

jupierce · 2016-07-18T16:15:09Z

For reference, the previous behavior was to apply the BUILD_LOGLEVEL at build pod creation time. The application was achieved by setting args=--loglevel=## directly in the build pod (e.g. https://github.com/openshift/origin/pull/9905/files#diff-5c9f2bf12ec684975c1fd68247f951dbL85).

As @csrwng points out in the issue, this means the BuildDefaults' env variables were too late to the party to affect logging since they are merged in after the pod is defined (and the loglevel already established as a pod/spec/args entry). I could have chosen to have the BuildDefaults plugin inspect and potentially modify the --loglevel arg in the pod definition, but this would have led to relatively complexity parsing code and, more importantly, established three areas of code all attempting to manage this one argument.

Instead, I moved the full responsibility of setting the loglevel to the BuildDefaults admission plugin.

jupierce · 2016-07-18T16:15:20Z

[test]

bparees · 2016-07-18T16:58:33Z

Specifying --build-loglevel on start-build (except on binary builds, which has always been the case since the function relies on env variables)

binary builds still create build pods, why can't they set this env variable on the build pod that is being created?

bparees · 2016-07-18T17:14:19Z

pkg/build/admission/buildpodutil.go

+	err = applyBuildLogLevel(build, pod)
+	if err != nil {
+		return admission.NewForbidden(a, fmt.Errorf("unable to set loglevel in pod: %v", err))
+	}


shouldn't this be in its own function? SetBuild is for setting the BUILD env variable w/ the build json definition.

jupierce · 2016-07-18T17:17:41Z

@bparees I had the same reaction since it seemed heavy handed. It is an architectural implication of the REST calls.

POST https://10.13.137.126:8443/oapi/v1/namespaces/default/buildconfigs/test/instantiatebinary?asFile=Dockerfile&name=test&namespace=default

vs non-binary:

 Request Body: {"kind":"BuildRequest","apiVersion":"v1","metadata":{"name":"ruby-hello-world","creationTimestamp":null},"env":[**{"name":"BUILD_LOGLEVEL","value":"17"}**],"triggeredBy":[{"message":"Manually triggered"}]}
POST https://10.13.137.126:8443/oapi/v1/namespaces/default/buildconfigs/ruby-hello-world/instantiate

And intentionally codified:
https://github.com/openshift/origin/blob/master/pkg/cmd/cli/cmd/startbuild.go#L284

bparees · 2016-07-18T17:20:38Z

@jupierce hm. I guess since the binaryinstantiate POST body is the files being sent in, there is nowhere to provide a request object that defines the log level for the build request. (unless we added support for query parameters)

jupierce · 2016-07-18T17:24:58Z

@bparees Do we not support multipart form data?

bparees · 2016-07-18T17:36:32Z

@bparees Do we not support multipart form data?

I'm not actually sure.

jupierce · 2016-07-18T19:58:41Z

New commit:

Fixes instances of BUILD_LOGLEVEL being out of place in testdata (unrelated to this Issue; just stemming the tide of copy and paste errors)
Sets uberdefault for build loglevel == 0
Adds unit test for new approach
Adds extended test for buildconfig BUILD_LOGLEVEL testing
Method extraction from SetBuild

jupierce · 2016-07-18T20:15:21Z

[testextended][extended:core(BUILD_LOGLEVEL)]

bparees · 2016-07-18T20:34:13Z

lgtm pending passing tests.

bparees · 2016-07-18T21:04:08Z

[testextended][extended:core(builds)]

bparees · 2016-07-19T03:10:19Z

--- FAIL: TestSetBuildLogLevel (0.00s)
    buildpodutil_test.go:102: Build pod loglevel was not transferred from BUILD_LOGLEVEL environment

jupierce · 2016-07-19T18:58:43Z

@bparees govet bombing on files I haven't changed...?

bparees · 2016-07-19T19:47:37Z

@jupierce known flake issue w/ travis, don't worry about it.

and the jenkins failures are yum repos you also can't do anything about.

i'll kick the travis test to run again, but the failure doesn't really matter.

jupierce · 2016-07-22T21:25:06Z

[test]

stevekuznetsov · 2016-08-01T17:24:51Z

re[test]

jupierce · 2016-08-02T14:59:08Z

re[test]

jupierce · 2016-08-02T17:48:13Z

@bparees Just to get back on your radar.

bparees · 2016-08-02T18:49:41Z

test/extended/builds/start.go

+			fmt.Fprintf(g.GinkgoWriter, "\nbuild log:\n%s\n", buildLog)
+			o.Expect(err).NotTo(o.HaveOccurred())
+
+			g.By(fmt.Sprintf("verifying the build output is verbose"))


is not verbose?

That's right, thanks. The correction actually applies to the subsequent test spec "BUILD_LOGLEVEL in buildconfig can be overridden by build-loglevel".

bparees · 2016-08-02T18:51:44Z

one nit and lgtm

bparees · 2016-08-02T18:51:57Z

(assuming tests start passing of course :) )

bparees · 2016-08-02T19:44:49Z

lgtm, will merge on passing extended tests

openshift-bot · 2016-08-04T15:57:37Z

Evaluated for origin testextended up to fee7e4d

openshift-bot · 2016-08-04T17:12:32Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/399/) (Extended Tests: core(builds), core(BUILD_LOGLEVEL))

openshift-bot · 2016-08-04T19:08:00Z

Evaluated for origin test up to fee7e4d

openshift-bot · 2016-08-04T20:27:36Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7523/)

jupierce · 2016-08-05T20:54:49Z

@bparees All clear.

bparees · 2016-08-05T21:26:32Z

[merge]

openshift-bot · 2016-08-05T21:36:10Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7599/) (Image: devenv-rhel7_4770)

openshift-bot · 2016-08-05T21:36:10Z

Evaluated for origin merge up to fee7e4d

bparees reviewed Jul 18, 2016
View reviewed changes

bparees reviewed Aug 2, 2016
View reviewed changes

Preventing build from inheriting master log level

fee7e4d

openshift-bot merged commit 87b68d6 into openshift:master Aug 5, 2016

jim-minter mentioned this pull request Dec 15, 2016

build --loglevel docs out of date openshift/openshift-docs#3399

Closed

jupierce mentioned this pull request Jan 19, 2017

Change in BUILD_LOGLEVEL handling openshift/openshift-docs#3535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preventing build from inheriting master log level #9905

Preventing build from inheriting master log level #9905

jupierce commented Jul 18, 2016 •

edited

Loading

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees Jul 18, 2016

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

jupierce commented Jul 18, 2016 •

edited

Loading

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees commented Jul 19, 2016

jupierce commented Jul 19, 2016

bparees commented Jul 19, 2016

jupierce commented Jul 22, 2016

stevekuznetsov commented Aug 1, 2016

jupierce commented Aug 2, 2016

jupierce commented Aug 2, 2016

bparees Aug 2, 2016

jupierce Aug 2, 2016

bparees commented Aug 2, 2016

bparees commented Aug 2, 2016

bparees commented Aug 2, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

jupierce commented Aug 5, 2016

bparees commented Aug 5, 2016

openshift-bot commented Aug 5, 2016 •

edited

Loading

openshift-bot commented Aug 5, 2016

Preventing build from inheriting master log level #9905

Preventing build from inheriting master log level #9905

Conversation

jupierce commented Jul 18, 2016 • edited Loading

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees Jul 18, 2016

Choose a reason for hiding this comment

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

jupierce commented Jul 18, 2016 • edited Loading

jupierce commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees commented Jul 18, 2016

bparees commented Jul 19, 2016

jupierce commented Jul 19, 2016

bparees commented Jul 19, 2016

jupierce commented Jul 22, 2016

stevekuznetsov commented Aug 1, 2016

jupierce commented Aug 2, 2016

jupierce commented Aug 2, 2016

bparees Aug 2, 2016

Choose a reason for hiding this comment

jupierce Aug 2, 2016

Choose a reason for hiding this comment

bparees commented Aug 2, 2016

bparees commented Aug 2, 2016

bparees commented Aug 2, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

jupierce commented Aug 5, 2016

bparees commented Aug 5, 2016

openshift-bot commented Aug 5, 2016 • edited Loading

openshift-bot commented Aug 5, 2016

jupierce commented Jul 18, 2016 •

edited

Loading

jupierce commented Jul 18, 2016 •

edited

Loading

openshift-bot commented Aug 5, 2016 •

edited

Loading