Skip to content

Update to go 1.12 and kube 1.16 - #40

Merged
openshift-merge-robot merged 6 commits into
openshift:masterfrom
ironcladlou:go-1.12
Oct 7, 2019
Merged

Update to go 1.12 and kube 1.16#40
openshift-merge-robot merged 6 commits into
openshift:masterfrom
ironcladlou:go-1.12

Conversation

@ironcladlou

@ironcladlou ironcladlou commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

Need I say more?

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 3, 2019
frobware
frobware reviewed Oct 3, 2019
View reviewed changes
Comment thread cmd/openshift-router/main.go
)

func main() {
logs.InitLogs()

@frobware frobware Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we don't need this (where "this" is logging) anymore?

@Miciah Miciah Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point—should we also be switching glog out for klog?

@ironcladlou ironcladlou Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the moved package an restored it. kubernetes/kubernetes@ba81a54

I don't want to re-do log handling in this PR unless absolutely necessary

@frobware frobware Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point—should we also be switching glog out for klog?

I would say so. Looks like everything that is in vendor is using klog.

@frobware frobware Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to re-do log handling in this PR unless absolutely necessary

Agreed. Should be a separate PR.

@frobware

frobware commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

The unit test are presumably failing because CI is pegged to Go 1.10 - looks like the TLS symbol requires Go 1.12:

@ironcladlou

ironcladlou commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

Depends on openshift/release#5289

@ironcladlou

ironcladlou commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

/retest all

@ironcladlou

ironcladlou commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

/retest

@ironcladlou

ironcladlou commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

/test all

@ironcladlou

ironcladlou commented Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@frobware

frobware commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

/retest

@ironcladlou
Remove extra log init call
cd80fd2 
This log init call now apparently re-initializes logging that is being
initialized somewhere else during startup, causing a panic due to duplicate
flag definitions.
@ironcladlou

ironcladlou commented Oct 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like the log init needs to go away after all, as some other package is already calling it.

/usr/bin/openshift-router flag redefined: log_dir
panic: /usr/bin/openshift-router flag redefined: log_dir

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc0000c0120, 0x192a3a0, 0x26624d0, 0x1695d41, 0x7, 0x16c9296, 0x2f)
	/usr/local/go/src/flag/flag.go:850 +0x4af
flag.(*FlagSet).StringVar(...)
	/usr/local/go/src/flag/flag.go:753
k8s.io/klog.InitFlags(0xc0000c0120)
	/go/src/github.com/openshift/router/vendor/k8s.io/klog/klog.go:428 +0x9d
k8s.io/component-base/logs.init.0()
	/go/src/github.com/openshift/router/vendor/k8s.io/component-base/logs/logs.go:35 +0x2d

https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_router/40/pull-ci-openshift-router-master-e2e-aws/118/artifacts/e2e-aws/pods/openshift-ingress_router-default-5454cfd6bb-ssxxs_router.log

We can untangle logging in another PR. This code predates several iterations of log library migrations.

@ironcladlou

ironcladlou commented Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

Okay, I've done some more local testing and logging is screwed up enough that I need to fix it here after all.

@ironcladlou

ironcladlou commented Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

Various infra failures.

/retest

@ironcladlou
Make logging init less broken
b92f9a4 
Paritally fix logging init. This is intended to fix things enough to
let us move forward to refactoring all logging.
@ironcladlou

ironcladlou commented Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

@openshift/sig-network-edge okay, I think I fixed logging "enough" to move forward — let's see the latest e2e runs to make sure things are being logged (even if we get some silly flag errors mixed amongst the output).

My goal is to get this working enough that we can do a followup to rip out glog entirely.

@frobware

frobware commented Oct 7, 2019

Copy link
Copy Markdown
Contributor

If we know what the logging issues are..., then:

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019
@openshift-ci-robot

openshift-ci-robot commented Oct 7, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, ironcladlou

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ironcladlou

ironcladlou commented Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-merge-robot
openshift-merge-robot merged commit 989679b into openshift:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JacobTanenbaum JacobTanenbaum Awaiting requested review from JacobTanenbaum
@knobunc knobunc Awaiting requested review from knobunc
2 more reviewers
@frobware frobware frobware left review comments
@Miciah Miciah Miciah left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@frobware frobware

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ironcladlou @frobware @openshift-ci-robot @Miciah @openshift-merge-robot