Fix proto.Merge of IntOrString type

[liggitt]

What type of PR is this?
/kind bug

What this PR does / why we need it:
The unsized int in the IntOrString type breaks proto.Merge. The proto serialization was already using int64, and json serialization used custom marshaling, so this doesn't change serialization at all.

Which issue(s) this PR fixes:
Fixes #83912

Does this PR introduce a user-facing change?:

Switched intstr.Type to sized integer to follow API guidelines and improve compatibility with proto libraries

/sig api-machinery
/cc @smarterclayton

[liggitt]

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/83956/pull-kubernetes-e2e-kind/1184127598125387777

ERROR: failed to create cluster: failed to init node with kubeadm: command "docker exec --privileged kind-control-plane kubeadm init --ignore-preflight-errors=all --config=/kind/kubeadm.conf --skip-token-print --v=6" failed with error: signal: broken pipe

cc @BenTheElder

/retest

[howardjohn]

Thanks!

[BenTheElder]

@liggitt thanks - this is kubernetes-sigs/kind#949, it's happening very rarely and I've not been able to replicate it in a controlled environment yet, seems like possibly a docker issue :(

https://prow.k8s.io/?job=pull-kubernetes-e2e-kind - 93% / 92% / 79 % passing for the past 3 / 12 / 48 hours, the failing runs are generally bad commits (EG code that doesn't compile).

[lavalamp]

Do we generally support proto.Merge? What are people using this for? What is the result of this operation?

[howardjohn]

We are using proto.Clone which seems to call proto.Merge at some point in the call tree

[lavalamp]

Why are you using that rather than the DeepCopy functions we generate? (assuming it does the same thing?)

[lavalamp]

I don't object to this change, but I am not at all sure I want to sign up for promising that proto.Merge works on all of our objects; we don't test it AFAIK, I have no idea if it does the right thing.

[liggitt]

I am not at all sure I want to sign up for promising that proto.Merge works on all of our objects

agree, I care more about this because all serialized primitive fields should be sized (even though practically this field value would only be 0 or 1 and the size wouldn't affect serialization). the fact that it fixes a serialization-related panic is a bonus.

[lavalamp]

But... Type is not serialized?

[liggitt]

But... Type is not serialized?

It is in proto.

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/intstr/generated.pb.go

Line 130 in cb3b715

i = encodeVarintGenerated(dAtA, i, uint64(m.Type))

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/intstr/generated.pb.go

Lines 195 to 213 in cb3b715

	case 1:
	if wireType != 0 {
	return fmt.Errorf("proto: wrong wireType = %d for field Type", wireType)
	}
	m.Type = 0
	for shift := uint(0); ; shift += 7 {
	if shift >= 64 {
	return ErrIntOverflowGenerated
	}
	if iNdEx >= l {
	return io.ErrUnexpectedEOF
	}
	b := dAtA[iNdEx]
	iNdEx++
	m.Type |= Type(b&0x7F) << shift
	if b < 0x80 {
	break
	}
	}

[lavalamp]

ew. hm. OK.

Is there a way to test this that won't imply to people that we expect proto.Merge to function everywhere?

[liggitt]

Is there a way to test this that won't imply to people that we expect proto.Merge to function everywhere?

other than dropping the proto.Merge test or adding a comment to that effect, not that I can think of... preference?

[lavalamp]

Either of those is fine with me.

…

On Tue, Oct 15, 2019 at 2:01 PM Jordan Liggitt ***@***.***> wrote: Is there a way to test this that won't imply to people that we expect proto.Merge to function everywhere? other than dropping the proto.Merge test or adding a comment to that effect, not that I can think of... preference? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#83956?email_source=notifications&email_token=AAE6BFSAR2TRCYRQIDHEZWTQOYVSVA5CNFSM4JA6HVY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBKGK4I#issuecomment-542401905>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE6BFWV2F34O7DRISVMGSLQOYVSVANCNFSM4JA6HVYQ> .

[liggitt]

dropped the proto.Merge test

[liggitt]

/priority backlog

[lavalamp]

/lgtm
/approve

(I can't believe I asked you to remove a test, sigh... ;) )

[liggitt]

I can't believe I asked you to remove a test, sigh... ;)

I know... where is Daniel and what have you done with him?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apimachinery/pkg/OWNERS [lavalamp,liggitt]

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

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.