Bug 1769717: Add support to EnsureServicePorts #272

enxebre · 2019-11-25T11:35:55Z

No description provided.

openshift-ci-robot · 2019-11-25T11:36:02Z

@enxebre: This pull request references Bugzilla bug 1769717, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

bug 1769717: Add support to EnsureServicePorts

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.

enxebre · 2019-11-25T11:37:06Z

cc @wking @abhinavdahiya

abhinavdahiya · 2019-11-25T16:48:30Z

The services have a lot of defaulting from API server, can we make sure we have tests that show it works in case of defaults...? like TCP/UDP, http/https etc...

lib/resourceapply/core.go

wking · 2019-11-25T23:41:48Z

/cherrypick release-4.3

openshift-cherrypick-robot · 2019-11-25T23:41:49Z

@wking: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-4.3

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.

wking · 2019-11-25T23:55:43Z

lib/resourcemerge/core.go

+	for i, existingServicePort := range *existing {
+		var existingCurr *corev1.ServicePort
+		for _, requiredServicePort := range required {
+			if existingServicePort.Name == requiredServicePort.Name {


Matching by name and then taking the union of in-cluster and in-manifest ports means there's no way to remove an entry. Also, name is optional, although I'm not clear on how that squares with:

All ports within a ServiceSpec must have unique names.

Maybe it's "If you set a name, it must not duplicate an existing ports entry in the same Service"?

Do we even need to support folks adding additional ports values to Service manifests managed by the CVO? Can we just clobber the existing value with the required value and set modified if that changed anything?

Notice that:

// Optional if only one ServicePort is defined on this service.

https://github.com/kubernetes/api/blob/11707872ac1cab52aefea0325f4698eaffeda3c8/core/v1/types.go#L3984

Matching by name and then taking the union of in-cluster and in-manifest ports means there's no way to remove an entry

There's no union, required overwrite existing via ensureServicePort. Entries are removed via https://github.com/openshift/cluster-version-operator/pull/272/files#diff-f7675c7b39553e431cdc6c3ba8f5c482R215-R218.

See test cases for each scenario (with/without name) https://github.com/openshift/cluster-version-operator/pull/272/files#diff-415c13f11ffc32696c5d69b900b3fe58R283.

Instead of a nested loop this could get better performance by hashing the port name but I just wanted to replicate the pattern already present for similar funcs, e.g ensureContainers

cluster-version-operator/lib/resourcemerge/core.go

Line 64 in df38364

func ensureContainers(modified *bool, existing *[]corev1.Container, required []corev1.Container) {

Also I'm wondering why the need for the merge library rather than just unconditionally overwrite/apply with whatever is required, but that's another discussion.

enxebre · 2019-11-26T09:15:51Z

The services have a lot of defaulting from API server, can we make sure we have tests that show it works in case of defaults...? like TCP/UDP, http/https etc...

@abhinavdahiya not sure how you mean to test cover that? the cvo does a regular Semantic.DeepEqual comparison in all ensureX funcs. If what's required does not have explicitly set the defaults given by the API machinery for existing then it'll return *modified = true

abhinavdahiya · 2019-11-26T16:42:28Z

/approve

abhinavdahiya · 2019-11-26T16:48:31Z

lib/resourcemerge/core_test.go

+					},
+				},
+			},
+		},


can you add a unit test wrt

input service has Protocol set due to defaulting and required doesn't https://github.com/kubernetes/api/blob/11707872ac1cab52aefea0325f4698eaffeda3c8/core/v1/types.go#L3988-L3991

TargetPort https://github.com/kubernetes/api/blob/11707872ac1cab52aefea0325f4698eaffeda3c8/core/v1/types.go#L4005

the API defaults to Port if unset, also it might be a string defaulted if the port is known IANA etc..

also NodePort https://github.com/kubernetes/api/blob/11707872ac1cab52aefea0325f4698eaffeda3c8/core/v1/types.go#L4013

as this value is assigned some value if none is already provided.

@abhinavdahiya sorry still not clear what we want to cover. EnsureServicePorts as any other ensuring function in this library do not known about defaults. EnsureServicePorts is just mutating a struct when !equality.Semantic.DeepEqual and it will clobber whatever existing has regardless this has been previously applied explicitly by a client or a by the api machinery defaulting. The clobbered struct will get defaults back once applied as it goes through api machinery.

The only think I can think of to increase coverage would be to apply the manifest after running EnsureServicePorts, then client.get the object back and validate that defaults are applied back properly, but the fake client does not apply any defaults unless we build a custom Reactor, therefore we'll be actually just covering our custom Reactor logic which is meaningless and this seems out of the scope to unit for EnsureServicePorts.
Let me know if this does not make sense to you and still want me to add anything.

EnsureServicePorts as any other ensuring function in this library do not known about defaults. EnsureServicePorts is just mutating a struct when !equality.Semantic.DeepEqual and it will clobber whatever existing has regardless this has been previously applied explicitly by a client or a by the api machinery defaulting.

I think we should prevent un-necessary updates, for the api machinery defaulting.. because that is known right...?
client changes are more than welcome to be over-written.

The only think I can think of to increase coverage would be to apply the manifest after running EnsureServicePorts, then client.get the object back and validate that defaults are applied back properly,

the unit tests already have an option of what exists in cluster currently and what we want..

for example, if the Protocol is empty (required), let's not run an update call because we know that it was defaulted. or when TargetPort is empty (required) make sure we are only running update when targetport-port differ because that is client change and not defaulting..

etc..

Even if we don't want to change these, unit-test cases that show that we are running update in presence of defaults is useful for future work when we can be smarter..

hopefully that provides more context...

@abhinavdahiya thanks! Ok I see, I added defaulting for Protocol https://github.com/openshift/cluster-version-operator/pull/272/files#diff-f7675c7b39553e431cdc6c3ba8f5c482R244 and test coverage https://github.com/openshift/cluster-version-operator/pull/272/files#diff-415c13f11ffc32696c5d69b900b3fe58R551-R638
Other values might depend on the serviceType, I don't think we should redo api machinery defaulting logic here any farther.
Please let me know if it looks good now or still wanting any change

abhinavdahiya · 2019-11-26T16:49:37Z

@enxebre approved the PR as it seems okay in terms of direction, but would make sure we have unit tests for #272 (comment) before we merge.

sdodson · 2019-12-02T14:59:23Z

/bugzilla refresh

openshift-ci-robot · 2019-12-02T14:59:26Z

@sdodson: This pull request references Bugzilla bug 1769717, which is valid.

Details

In response to this:

/bugzilla refresh

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.

enxebre · 2019-12-03T11:32:36Z

/test e2e-aws

abhinavdahiya · 2019-12-03T18:02:57Z

/lgtm

openshift-ci-robot · 2019-12-03T18:03:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, enxebre

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:

~~OWNERS~~ [abhinavdahiya]

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

openshift-ci-robot · 2019-12-03T18:05:13Z

@enxebre: All pull requests linked via external trackers have merged. Bugzilla bug 1769717 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1769717: Add support to EnsureServicePorts

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-cherrypick-robot · 2019-12-03T18:05:35Z

@wking: new pull request created: #275

Details

In response to this:

/cherrypick release-4.3

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.

Avoid: $ go test ./lib/resourcemerge/ panic: runtime error: index out of range [recovered] panic: runtime error: index out of range goroutine 38 [running]: testing.tRunner.func1(0xc0001ab000) .../sdk/go1.12.9/src/testing/testing.go:830 +0x392 panic(0xccb520, 0x163f880) .../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5 github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1) .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840 github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...) .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6 github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000) .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7 testing.tRunner(0xc0001ab000, 0xc0001d8770) .../sdk/go1.12.9/src/testing/testing.go:865 +0xc0 created by testing.(*T).Run .../sdk/go1.12.9/src/testing/testing.go:916 +0x35a FAIL github.com/openshift/cluster-version-operator/lib/resourcemerge 0.010s (with the core_test.go but the old core.go) when removing an entry mutated the existing slice without re-entering the: for i, whatever := range *existing With this commit, we iterate from the back of the existing slice, so any removals affect indexes that we've already covered. For both containers and service ports, any appends happen later in the function, so we don't need to worry about slice expansion at this point. The buggy logic was originally from 3d1ad76 (Remove containers if requested in Update, 2019-04-26, openshift#178). This commit cherry-picks e8e80c2 (lib/resourcemerge/core: Fix panic on container/port removal, 2019-12-16, openshift#282) back to the 4.2 branch. But since f268b6d (Add support to EnsureServicePorts, 2019-11-25, openshift#272) and its EnsureServicePorts was never backported to the 4.2 branch, I've only kept the container portion of e8e80c2 in this commit.

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 25, 2019

openshift-ci-robot requested review from smarterclayton and wking November 25, 2019 11:36

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 25, 2019

enxebre force-pushed the servicePort branch 2 times, most recently from 0c20f1d to a05dd54 Compare November 25, 2019 12:39

wking reviewed Nov 25, 2019

View reviewed changes

lib/resourceapply/core.go Outdated Show resolved Hide resolved

wking changed the title ~~bug 1769717: Add support to EnsureServicePorts~~ Bug 1769717: Add support to EnsureServicePorts Nov 25, 2019

wking reviewed Nov 25, 2019

View reviewed changes

enxebre force-pushed the servicePort branch from a05dd54 to a6e044f Compare November 26, 2019 08:47

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2019

abhinavdahiya reviewed Nov 26, 2019

View reviewed changes

Add support to EnsureServicePorts

f268b6d

enxebre force-pushed the servicePort branch from a6e044f to f268b6d Compare December 3, 2019 10:13

openshift-ci-robot assigned abhinavdahiya Dec 3, 2019

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

openshift-merge-robot merged commit 98d173e into openshift:master Dec 3, 2019

openshift-cherrypick-robot mentioned this pull request Dec 3, 2019

Bug 1776297: Add support to EnsureServicePorts #275

Merged

wking mentioned this pull request Feb 6, 2020

Bug 1800346: lib/resourcemerge/core: Fix panic on container removal #314

Merged

Bug 1769717: Add support to EnsureServicePorts #272

Bug 1769717: Add support to EnsureServicePorts #272

Uh oh!

Conversation

enxebre commented Nov 25, 2019

Uh oh!

openshift-ci-robot commented Nov 25, 2019

Uh oh!

enxebre commented Nov 25, 2019

Uh oh!

abhinavdahiya commented Nov 25, 2019

Uh oh!

Uh oh!

wking commented Nov 25, 2019

Uh oh!

openshift-cherrypick-robot commented Nov 25, 2019

Uh oh!

wking Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinavdahiya commented Nov 26, 2019

Uh oh!

abhinavdahiya Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

enxebre Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

enxebre Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Nov 26, 2019

Uh oh!

sdodson commented Dec 2, 2019

Uh oh!

openshift-ci-robot commented Dec 2, 2019

Uh oh!

enxebre commented Dec 3, 2019

Uh oh!

abhinavdahiya commented Dec 3, 2019

Uh oh!

openshift-ci-robot commented Dec 3, 2019

Uh oh!

openshift-ci-robot commented Dec 3, 2019

Uh oh!

openshift-cherrypick-robot commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wking Nov 25, 2019 •

edited

Loading

enxebre Nov 26, 2019 •

edited

Loading

enxebre commented Nov 26, 2019 •

edited

Loading

enxebre Nov 28, 2019 •

edited

Loading