Don't pivot if identical sha256 #43

cgwalters · 2019-03-08T15:09:14Z

This is a lowering of
openshift/machine-config-operator#463
to pivot. We need it for the case of doing an early pivot
before the MCD comes up.

cgwalters · 2019-03-08T15:13:58Z

This is failing for me with:

F0308 12:54:37.000370   12936 root.go:136] parsing current osImageURL: parsing reference: "exampleregistry/redhat-coreos/maipo@sha256:c09f455cc09673a1a13ae7b54cc4348cda0411e06dfa79ecd0130b35d62e8670": unsupported digest algorithm

Which um...what? This must be something with github.com/opencontainers/go-digest...it looks like the algorithms are package-scoped variables. But why is it failing?

jlebon · 2019-03-08T15:24:16Z

This looks good overall! Would be nice to also directly edit the origin so the custom string points to the new image. Basically reframe this not as a special case, but just following through with the operation and being smart about not rebooting like we currently do. Not essential right now though if we're going for a tactical fix.

jlebon · 2019-03-08T15:25:57Z

cmd/root.go

-	if previousPivot == imgid {
+	targetMatched, err := compareOSImageURL(previousPivot, imgid)
+	if err != nil {
+		glog.Fatalf("%v", err)


Minor: we pretty liberally use glog.Fatalf everywhere in this codebase. And since we're not actually adding any context here, I'd say just drop error-handling and Fatalf directly in getRefDigest().

It feels easier to keep the code looking similar to the MCD code - why change it unnecessarily, plus while I get the policy of using Fatalf where it's convenient, why change working code to stop using "proper" error handling?

why change working code to stop using "proper" error handling?

To keep it consistent with the rest of the whole codebase. Not strongly opposed though, just seems out of place. Keeping in sync with the MCD version is a good point though. Maybe add a comment about that?

Gopkg.lock

cgwalters · 2019-03-08T15:45:13Z

Added a test, now banging my head against:

$ go test ./cmd/...
/tmp/go-build756046464/b001/cmd.test flag redefined: log_dir
panic: /tmp/go-build756046464/b001/cmd.test flag redefined: log_dir

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc00006a180, 0x670f20, 0xc000056660, 0x639cb8, 0x7, 0x644aa5, 0x2f)
        /usr/lib/golang/src/flag/flag.go:805 +0x529
flag.(*FlagSet).StringVar(0xc00006a180, 0xc000056660, 0x639cb8, 0x7, 0x0, 0x0, 0x644aa5, 0x2f)
        /usr/lib/golang/src/flag/flag.go:708 +0x8a
flag.(*FlagSet).String(0xc00006a180, 0x639cb8, 0x7, 0x0, 0x0, 0x644aa5, 0x2f, 0xc000056650)
        /usr/lib/golang/src/flag/flag.go:721 +0x8b
flag.String(0x639cb8, 0x7, 0x0, 0x0, 0x644aa5, 0x2f, 0xc00007e210)
        /usr/lib/golang/src/flag/flag.go:728 +0x69
FAIL    github/openshift/pivot/cmd      0.005s

cgwalters · 2019-03-08T16:08:13Z

https://stackoverflow.com/questions/37284423/glog-flag-redefined-error apparently...man Golang vendor/ stuff is a huge mess in comparison to Cargo.

jlebon · 2019-03-08T16:19:34Z

Hmm, go test working OK here (though seems like one of the tests is failing):

$ go test ./cmd/...
--- FAIL: TestCompareOSImageURL (0.00s)
    root_test.go:17: Expected refA = refB
FAIL
FAIL    github.com/openshift/pivot/cmd  0.006s

jlebon · 2019-03-08T16:20:59Z

Is your setup similar to this:

$ echo $GOPATH
/code/go
$ pwd
/code/go/src/github.com/openshift/pivot

(Similar -> pwd is $GOPATH + src/github.com/openshift/pivot)

cgwalters · 2019-03-08T16:21:26Z

OK yeah, no idea what was going on before. I just pushed a tweak so that we display errors, now I get:

$ go test -v ./cmd/...
=== RUN   TestCompareOSImageURL
--- FAIL: TestCompareOSImageURL (0.00s)
    root_test.go:10: parsing current osImageURL: parsing reference: "registry.example.com/foo/bar@sha256:0743a3cc3bcf3b4aabb814500c2739f84cb085ff4e7ec7996aef7977c4c19c7f": unsupported digest algorithm
FAIL
FAIL    github/openshift/pivot/cmd      0.005s

cgwalters · 2019-03-08T16:40:13Z

OK so apparently
https://golang.org/pkg/crypto/#Hash.Available is returning false for crypto.SHA256...

cgwalters · 2019-03-08T19:20:50Z

https://golang.org/pkg/crypto/#Hash.Available is returning false for crypto.SHA256...

And import ("crypto/sha256") fixes this...then to avoid unused imports errors I had to add a dummy sha256 usage. If this (or a better way to do it) is documented at all anywhere I can't find it.

cmd/root.go

cmd/root_test.go

ashcrow · 2019-03-08T20:05:03Z

man Golang vendor/ stuff is a huge mess in comparison to Cargo.

Yeah, it really is.

So we can parse container references.

This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.

cgwalters · 2019-03-11T17:51:30Z

Rebased, though one open discussion.

cgwalters · 2019-03-11T20:11:54Z

Let's get this shipped please?

ashcrow · 2019-03-11T20:16:42Z

/lgtm

ashcrow · 2019-03-11T20:17:50Z

/cc @yuqi-zhang for syncing over

openshift-ci-robot · 2019-03-11T20:17:52Z

@ashcrow: GitHub didn't allow me to request PR reviews from the following users: syncing, over, for.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @yuqi-zhang for syncing over

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2019

jlebon reviewed Mar 8, 2019

View reviewed changes

cgwalters commented Mar 8, 2019

View reviewed changes

Gopkg.lock Outdated Show resolved Hide resolved

cgwalters force-pushed the no-pivot-identical branch 2 times, most recently from ca0909a to cc36276 Compare March 8, 2019 15:44

cgwalters force-pushed the no-pivot-identical branch from cc36276 to f91621d Compare March 8, 2019 16:20

cgwalters force-pushed the no-pivot-identical branch from 3bd36df to b26b9e1 Compare March 8, 2019 19:19

cgwalters force-pushed the no-pivot-identical branch from b26b9e1 to 4a065e8 Compare March 8, 2019 19:48

ashcrow reviewed Mar 8, 2019

View reviewed changes

cmd/root.go Show resolved Hide resolved

ashcrow reviewed Mar 8, 2019

View reviewed changes

cmd/root.go Show resolved Hide resolved

ashcrow reviewed Mar 8, 2019

View reviewed changes

cmd/root_test.go Show resolved Hide resolved

cgwalters force-pushed the no-pivot-identical branch from 4a065e8 to e0e0317 Compare March 11, 2019 16:23

cgwalters changed the title ~~WIP: Don't pivot if identical sha256~~ Don't pivot if identical sha256 Mar 11, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2019

cgwalters added 2 commits March 11, 2019 17:17

vendor: Add github.com/containers/image/docker/reference

c32645b

So we can parse container references.

Don't pivot if identical sha256

71d6d77

This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.

cgwalters force-pushed the no-pivot-identical branch from e0e0317 to 71d6d77 Compare March 11, 2019 17:17

ashcrow approved these changes Mar 11, 2019

View reviewed changes

openshift-ci-robot assigned ashcrow Mar 11, 2019

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

ashcrow merged commit 297a0ec into openshift:master Mar 11, 2019

openshift-ci-robot requested a review from yuqi-zhang March 11, 2019 20:17

Don't pivot if identical sha256 #43

Don't pivot if identical sha256 #43

Uh oh!

Conversation

cgwalters commented Mar 8, 2019

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

jlebon commented Mar 8, 2019

Uh oh!

jlebon Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

cgwalters Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

jlebon Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

jlebon commented Mar 8, 2019

Uh oh!

jlebon commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

cgwalters commented Mar 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashcrow commented Mar 8, 2019

Uh oh!

cgwalters commented Mar 11, 2019

Uh oh!

cgwalters commented Mar 11, 2019

Uh oh!

ashcrow commented Mar 11, 2019

Uh oh!

ashcrow commented Mar 11, 2019

Uh oh!

openshift-ci-robot commented Mar 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jlebon commented Mar 8, 2019 •

edited

Loading