implemented glog wrapper for use in delegated commands #8024

stevekuznetsov · 2016-03-15T17:35:24Z

deads2k · 2016-03-15T18:16:50Z

pkg/cmd/server/admin/create_keypair.go

@@ -112,7 +112,7 @@ func (o CreateKeyPairOptions) CreateKeyPair() error {
 		return err
 	}

-	fmt.Fprintf(o.Output, "Generated new key pair as %s and %s\n", o.PublicKeyFile, o.PrivateKeyFile)


Yeah, this is the delegation thing I was talking about. @sosiouxme requested these as Printfs in the case where you're actually trying to run this command. When you run this command in a delegated case, you should pass an o.Output that actual writes to glog.V(3).Info.

Yeah, need glog io.Writer wrapper

On Tue, Mar 15, 2016 at 2:16 PM, David Eads [email protected]
wrote:

In pkg/cmd/server/admin/create_keypair.go
#8024 (comment):

@@ -112,7 +112,7 @@ func (o CreateKeyPairOptions) CreateKeyPair() error {
return err
}

fmt.Fprintf(o.Output, "Generated new key pair as %s and %s\n", o.PublicKeyFile, o.PrivateKeyFile)

Yeah, this is the delegation thing I was talking about. @sosiouxme
https://github.com/sosiouxme requested these as Printfs in the case
where you're actually trying to run this command. When you run this command
in a delegated case, you should pass an o.Output that actual writes to
glog.V(3).Info.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8024/files/011a2f607bc9309896480a82044992be53dc3f3b#r56213717

stevekuznetsov · 2016-04-08T16:06:58Z

@deads2k PTAL

stevekuznetsov · 2016-04-08T16:40:46Z

Travis flake (?):

[INFO] Creating OpenShift node config
Error: invalid argument "" for --hostnames=: EOF
...
'openshift admin create-node-config \
--listen="${KUBELET_SCHEME}://${KUBELET_BIND_HOST}:${KUBELET_PORT}" \
--node-dir="${NODE_CONFIG_DIR}" \
--node="${KUBELET_HOST}" \
--hostnames="${KUBELET_HOST}" \
--master="${MASTER_ADDR}" \
--node-client-certificate-authority="${MASTER_CONFIG_DIR}/ca.crt" \
--certificate-authority="${MASTER_CONFIG_DIR}/ca.crt" \
--signer-cert="${MASTER_CONFIG_DIR}/ca.crt" \
--signer-key="${MASTER_CONFIG_DIR}/ca.key" \
--signer-serial="${MASTER_CONFIG_DIR}/ca.serial.txt"' exited with status 1

@smarterclayton have we seen this before? I don't think this was something I changed.

smarterclayton · 2016-04-08T16:46:10Z

Travis may not have returned any IP addresses in ip addr list?

On Fri, Apr 8, 2016 at 12:40 PM, Steve Kuznetsov [email protected]
wrote:

Travis flake (?):

[INFO] Creating OpenShift node config
Error: invalid argument "" for --hostnames=: EOF
...
'openshift admin create-node-config
--listen="${KUBELET_SCHEME}://${KUBELET_BIND_HOST}:${KUBELET_PORT}"
--node-dir="${NODE_CONFIG_DIR}"
--node="${KUBELET_HOST}"
--hostnames="${KUBELET_HOST}"
--master="${MASTER_ADDR}"
--node-client-certificate-authority="${MASTER_CONFIG_DIR}/ca.crt"
--certificate-authority="${MASTER_CONFIG_DIR}/ca.crt"
--signer-cert="${MASTER_CONFIG_DIR}/ca.crt"
--signer-key="${MASTER_CONFIG_DIR}/ca.key"
--signer-serial="${MASTER_CONFIG_DIR}/ca.serial.txt"' exited with status 1

@smarterclayton https://github.com/smarterclayton have we seen this
before? I don't think this was something I changed.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8024 (comment)

stevekuznetsov · 2016-04-13T13:17:47Z

@smarterclayton this is ready for a look. The downside of using a wrapper here is that all of the glog output will look like it's coming from log.go, unclear if that's really an issue.

deads2k · 2016-04-13T14:55:47Z

@smarterclayton this is ready for a look. The downside of using a wrapper here is that all of the glog output will look like it's coming from log.go, unclear if that's really an issue.

Use InfoDepth

stevekuznetsov · 2016-04-13T15:15:36Z

Use InfoDepth

V(<int>) doesn't support InfoDepth unless I'm totally mis-reading the API.

deads2k · 2016-04-13T15:26:44Z

V() doesn't support InfoDepth unless I'm totally mis-reading the API.

you ought to be able to layer it by doing your own level check and then calling InfoDepth, right?

You could also open a pull to try to get it added upstream.

stevekuznetsov · 2016-04-13T16:35:28Z

I'll try it upstream.

stevekuznetsov · 2016-04-13T19:05:15Z

[test] me

deads2k · 2016-04-14T12:50:33Z

Godeps/_workspace/src/github.com/golang/glog/glog.go

@@ -876,7 +876,7 @@ const flushInterval = 30 * time.Second

 // flushDaemon periodically flushes the log file buffers.
 func (l *loggingT) flushDaemon() {
-	for _ = range time.NewTicker(flushInterval).C {


don't mess with things you don't need to.

Amazingly it looks like the Go authors don't run go fmt -s ... this was an automated change, I'll revert

deads2k · 2016-04-14T12:50:44Z

did you submit a pull upstream?

stevekuznetsov · 2016-04-14T13:12:33Z

@deads2k they don't allow pulls for the project, but I attempted to reach out to the authors and ask if they'd consider the patch

stevekuznetsov · 2016-04-15T14:23:47Z

@deads2k reverted change from go fmt -s, PTAL

deads2k · 2016-04-15T15:05:13Z

I'm not seeing how this moves "Generating node credentials ..." to a verbose glog level. Pointer to that bit? In fact, I think this moving things backwards

Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov · 2016-04-20T12:47:43Z

I'm not seeing how this moves "Generating node credentials ..."

Missed that! Added more delegation.

deads2k · 2016-04-20T13:12:40Z

pkg/cmd/server/admin/create_keypair.go

@@ -86,15 +85,15 @@ func (o CreateKeyPairOptions) Validate(args []string) error {
 }

 func (o CreateKeyPairOptions) CreateKeyPair() error {
-	glog.V(4).Infof("Creating a key pair with: %#v", o)
+	fmt.Fprintf(o.Output, "Creating a key pair with: %#v", o)


undo this, the glog was correct.

deads2k · 2016-04-20T13:15:03Z

find the spots where we're calling commands to generate the config and update those instances with your new glogl writer.

Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov · 2016-04-20T13:49:30Z

pkg/cmd/server/start/start_allinone.go

@@ -227,7 +228,7 @@ func (o AllInOneOptions) StartAllInOne() error {
 		if err != nil {
 			return err
 		}
-		fmt.Fprintf(o.MasterOptions.Output, "%s\n", host)
+		fmt.Fprintf(o.Output, "%s\n", host)


Can revert as technically right now o.MasterOptions.Output == o.Output but as written previously this didn't really make sense, as this is output of allinone.

stevekuznetsov · 2016-04-20T13:50:46Z

@deads2k tightened up where we use the delegation wrapper. Feels like we could use a scrub of the code to make sure we use it wherever we are supposed to, but I don't think I have the knowledge right now to make the call on when we should and shouldn't, so I'll leave that off until later.

stevekuznetsov · 2016-04-20T16:28:12Z

check flaked on #8039
integration flaked on TestBuildOutputCycleDetection:

--- FAIL: TestBuildOutputCycleDetection-2 (0.47s)
    newapp_test.go:1340: successful build from dockerfile with identical input and output image references with warning: unexpected error: no match for "centos"

extended flaked on yum: #8571

#triplewhammy
re[test]

openshift-bot · 2016-04-20T18:36:03Z

Evaluated for origin test up to 5963a5c

openshift-bot · 2016-04-20T19:44:16Z

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

stevekuznetsov · 2016-04-22T14:08:58Z

@deads2k ready for a look, have green tests

deads2k · 2016-04-22T18:34:41Z

@stevekuznetsov add output to your description.

lgtm

stevekuznetsov · 2016-05-02T13:56:02Z

Sample output:

$ sudo $(which openshift) start --loglevel=3
I0502 09:51:14.027914   25011 start_master.go:200] Generating master configuration
...
I0502 09:51:14.426958   25011 create_keypair.go:115] Generated new key pair as openshift.local.config/master/serviceaccounts.public.key and 
...
I0502 09:51:17.992881   25011 create_nodeconfig.go:247] Generating node credentials ...

stevekuznetsov · 2016-05-02T13:59:42Z

@deads2k are these the examples you were looking for?

deads2k · 2016-05-02T14:29:05Z

[merge]

openshift-bot · 2016-05-02T14:30:21Z

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

openshift-bot · 2016-05-02T14:30:21Z

Evaluated for origin merge up to 5963a5c

deads2k reviewed Mar 15, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/glog branch 3 times, most recently from 0ac16e4 to 23839b4 Compare April 8, 2016 16:06

stevekuznetsov force-pushed the skuznets/glog branch 3 times, most recently from 43459aa to 0bd4cec Compare April 13, 2016 13:13

stevekuznetsov force-pushed the skuznets/glog branch 4 times, most recently from 5c56f9d to 4ecf430 Compare April 13, 2016 18:49

stevekuznetsov changed the title ~~moved output to glog~~ implemented glog wrapper for use in delegated commands Apr 13, 2016

deads2k reviewed Apr 14, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/glog branch from 4ecf430 to ca6a26f Compare April 15, 2016 14:23

UPSTREAM: golang/glog: <carry>: add 'InfoDepth' to 'V' guard

433ceb1

Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov force-pushed the skuznets/glog branch from ca6a26f to 9d0dd6d Compare April 20, 2016 12:43

deads2k reviewed Apr 20, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/glog branch 4 times, most recently from 1b7ba72 to 5963a5c Compare April 20, 2016 13:47

implemented glog wrapper for use in delegated commands

5963a5c

Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov reviewed Apr 20, 2016
View reviewed changes

deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2016

openshift-bot merged commit b50fa54 into openshift:master May 2, 2016

stevekuznetsov deleted the skuznets/glog branch June 15, 2016 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented glog wrapper for use in delegated commands #8024

implemented glog wrapper for use in delegated commands #8024

stevekuznetsov commented Mar 15, 2016

deads2k Mar 15, 2016

smarterclayton Mar 15, 2016

stevekuznetsov commented Apr 8, 2016

stevekuznetsov commented Apr 8, 2016

smarterclayton commented Apr 8, 2016

stevekuznetsov commented Apr 13, 2016

deads2k commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

deads2k commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

deads2k Apr 14, 2016

stevekuznetsov Apr 14, 2016

deads2k commented Apr 14, 2016

stevekuznetsov commented Apr 14, 2016

stevekuznetsov commented Apr 15, 2016

deads2k commented Apr 15, 2016

stevekuznetsov commented Apr 20, 2016

deads2k Apr 20, 2016

deads2k commented Apr 20, 2016

stevekuznetsov Apr 20, 2016

stevekuznetsov commented Apr 20, 2016

stevekuznetsov commented Apr 20, 2016 •

edited

Loading

openshift-bot commented Apr 20, 2016

openshift-bot commented Apr 20, 2016

stevekuznetsov commented Apr 22, 2016

deads2k commented Apr 22, 2016

stevekuznetsov commented May 2, 2016

stevekuznetsov commented May 2, 2016

deads2k commented May 2, 2016

openshift-bot commented May 2, 2016 •

edited

Loading

openshift-bot commented May 2, 2016

implemented glog wrapper for use in delegated commands #8024

implemented glog wrapper for use in delegated commands #8024

Conversation

stevekuznetsov commented Mar 15, 2016

deads2k Mar 15, 2016

Choose a reason for hiding this comment

smarterclayton Mar 15, 2016

Choose a reason for hiding this comment

stevekuznetsov commented Apr 8, 2016

stevekuznetsov commented Apr 8, 2016

smarterclayton commented Apr 8, 2016

stevekuznetsov commented Apr 13, 2016

deads2k commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

deads2k commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

stevekuznetsov commented Apr 13, 2016

deads2k Apr 14, 2016

Choose a reason for hiding this comment

stevekuznetsov Apr 14, 2016

Choose a reason for hiding this comment

deads2k commented Apr 14, 2016

stevekuznetsov commented Apr 14, 2016

stevekuznetsov commented Apr 15, 2016

deads2k commented Apr 15, 2016

stevekuznetsov commented Apr 20, 2016

deads2k Apr 20, 2016

Choose a reason for hiding this comment

deads2k commented Apr 20, 2016

stevekuznetsov Apr 20, 2016

Choose a reason for hiding this comment

stevekuznetsov commented Apr 20, 2016

stevekuznetsov commented Apr 20, 2016 • edited Loading

openshift-bot commented Apr 20, 2016

openshift-bot commented Apr 20, 2016

stevekuznetsov commented Apr 22, 2016

deads2k commented Apr 22, 2016

stevekuznetsov commented May 2, 2016

stevekuznetsov commented May 2, 2016

deads2k commented May 2, 2016

openshift-bot commented May 2, 2016 • edited Loading

openshift-bot commented May 2, 2016

stevekuznetsov commented Apr 20, 2016 •

edited

Loading

openshift-bot commented May 2, 2016 •

edited

Loading