ignore setSelfLink errors

[deads2k]

We think this is fixed in upstream, but it doesn't patch in cleanly, so we'll ignore the setSelfLink errors for now so that we can continue with other features.

The failure breaks *AccessReview and blocks list projects.

@smarterclayton ptal

/cc @derekwaynecarr

[smarterclayton]

SetSelfLink should return a specific error we can test against - this approach breaks the runtime.SelfLinker abstraction.

[deads2k]

More like this?

[smarterclayton]

I mean runtime.HasSelfLink is not something that should exist. namer.SetSelfLink calls the appropriate accessor - the source methods that implement that should return an appropriate ErrNoSelfLink

[deads2k]

Ok, so before setting the SelfLink, the name is built. Building that name requires retrieving name and namespace. ResourceAccessReview has neither of those fields. Right now, that get attempt calls Accessor() which produces an error. That causes a failure before any attempt to SetSelfLink.

The godoc for Accessor() (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/meta/meta.go#L33) suggests that the failures I see about missing fields are intended and intentional.

That means that unless we change Accessor's contract to not fail like that, we aren't going to be guaranteed to ever see the error we want because we'll fail when doing thinks like getting names and namespaces.

[smarterclayton]

----- Original Message -----

@@ -328,6 +328,11 @@ func finishRequest(timeout time.Duration, fn
resultFunc) (result runtime.Object,
// setSelfLink sets the self link of an object (or the child items in a
list) to the base URL of the request
// plus the path and query generated by the provided linkFunc
func setSelfLink(obj runtime.Object, req *restful.Request, namer
ScopeNamer) error {

• // if the object doesn't have a selfLink, no-op
• if !runtime.HasSelfLink(obj) {

Ok, so before setting the SelfLink, the name is built. Building that name
requires retrieving name and namespace. ResourceAccessReview has neither of
those fields. Right now, that get attempt calls Accessor() which
produces an error. That causes a failure before any attempt to SetSelfLink.

The godoc for Accessor()
(https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/meta/meta.go#L33)
suggests that the failures I see about missing fields are intended and
intentional.

That means that unless we change Accessor's contract to not fail like that,
we aren't going to be guaranteed to ever see the error we want because we'll
fail when doing thinks like getting names and namespaces.

Accessor should return a typed known error that clients can choose to ignore.

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1049/files#r24848688

[deads2k]

changed to ignore the failures and log

[smarterclayton]

... inside setSelfLink

[deads2k]

... inside setSelfLink

"And I must have blue window treatments with purple polkadots on my condemned house...."

[smarterclayton]

I meant mutilate in the loosest possible sense :)

----- Original Message -----

@@ -69,8 +70,7 @@ func GetResource(r RESTGetter, ctxFn ContextFunc, namer
ScopeNamer, codec runtim
return
}
if err := setSelfLink(result, req, namer); err != nil {

•       errorJSON(err, codec, w)
  

•       return
  

•       glog.Errorf("failure while setting SelfLink: %v", err)
  

... inside setSelfLink

"And I must have blue window treatments with purple polkadots on my condemned
house...."

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1049/files#r24853961

[deads2k]

I'm now at the point where having tea party with a stuffed cow is preferable to touching this again...

[smarterclayton]

Don't hate on stuffed cows.

[smarterclayton]

LGTM [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/946/) (Image: devenv-fedora_818)

[openshift-bot]

Evaluated for origin up to e7f867e

[derekwaynecarr]

This spams the logs horribly. And makes you think the system is not stable. We should not be logging at such a high level if its normal allowed behavior.

[deads2k]

But we must not mutilate throw away code. Our throw away patches must be perfect snowflakes.

I know, let's detect if we can set a self link before we attempt it and avoid trying. That way, we can avoid spamming the log, but still see all the expected message when we get actual failures.

[derekwaynecarr]

To put another way, if its not an actual error for us right now in OpenShift, since we swallow it, then we should not log an error to the log.

[smarterclayton]

Oh, missed that in the review. This should be logged at V(4) / V(5).