OCPBUGS-1458: Allow CVO to update `KUBERNETES_SERVICE_HOST` with LB address #851

petr-muller · 2022-10-13T16:45:17Z

The problem was identified to be a broken substitution of internal load balancer into KUBERNETES_SERVICE_HOST by Trevor and David (see my JIRA comment and related Slack thread).

CVO injects the LB hostname in the ModifyDeployment fine, but then the deployment gets applied in ApplyDeployment and the EnsureDeployment->ensurePodTemplateSpec->ensurePodSpec->ensureContainers->ensureContainer->ensureEnvVar chain stomps the updated value in required by the old value from existing and reverts the injection.

This behavior was added intentionally in #559 as a part of a fix for various hot-looping issues. The substitution apparently caused some hot-looping issues in the past (slack thread). I have tested removing the special handling KUBERNETES_SERVICE_HOST thoroughly, and saw no problematic behavior. After fixing other hot-looping problems in #855 to eliminate noise, no new hot-loops occurs with KUBERNETES_SERVICE_HOST handling removed.

Even with the substitution fix, I observed further network problems in CVO (example):

F1024 17:40:24.591942       1 start.go:29] error: Get "https://api-int.ci-op-02b7hikv-3302f.ci.azure.devcluster.openshift.com:6443/apis/config.openshift.io/v1/featuregates/cluster": dial tcp: lookup api-int.ci-op-02b7hikv-3302f.ci.azure.devcluster.openshift.com on 172.30.0.10:53: read udp 169.254.169.2:48475->172.30.0.10:53: i/o timeout

To get rid of these, I added a client-side poll to CVO, but I currently fail to reproduce the issue in #859, so maybe the i/o timeout issues were just some co-indidental disruption bug that we happened to fix in the meantime? Not sure.

openshift-ci-robot · 2022-10-13T16:45:21Z

@petr-muller: This pull request references Jira Issue OCPBUGS-1458, which is invalid:

expected the bug to target the "4.12.0" version, but it targets "4.12" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

This just adds the reproducer for the behavior that (I think) is the culprit of OCPBUGS-1458. The problem was identified to be a broken substitution of internal load balancer into KUBERNETES_SERVICE_HOST by Trevor and David (see my JIRA comment and related Slack thread).

CVO injects the LB hostname in the ModifyDeployment fine, but then the deployment gets applied in ApplyDeployment and the EnsureDeployment->ensurePodTemplateSpec->ensurePodSpec->ensureContainers->ensureContainer->ensureEnvVar chain stomps the updated value in required by the old value from existing and reverts the injection in this way:

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

Lines 115 to 120 in dc1ad0a

// Currently only CVO deployment uses this variable to inject internal LB host.

// This may result in an IP address being returned by API so assuming the

// returned value is correct.

if required[envidx].Name == "KUBERNETES_SERVICE_HOST" {

ensureEnvVarKubeService(*existing, &required[envidx])

}

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

Lines 132 to 138 in dc1ad0a

func ensureEnvVarKubeService(existing []corev1.EnvVar, required *corev1.EnvVar) {

for envidx := range existing {

if existing[envidx].Name == required.Name {

required.Value = existing[envidx].Value

}

}

}

This behavior was added intentionally in #559, but I do not understand the reasoning given in the comment:

Currently only CVO deployment uses KUBERNETES_SERVICE_HOST env variable to inject internal LB host.This may result in an IP address being returned by API so always assuming returned value is okay.

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 · 2022-10-13T16:49:53Z

@petr-muller: This pull request references Jira Issue OCPBUGS-1458, which is invalid:

expected the bug to target the "4.12.0" version, but it targets "4.12" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

For now, this just adds the reproducer for the behavior that (I think) is the culprit of OCPBUGS-1458. The problem was identified to be a broken substitution of internal load balancer into KUBERNETES_SERVICE_HOST by Trevor and David (see my JIRA comment and related Slack thread).

CVO injects the LB hostname in the ModifyDeployment fine, but then the deployment gets applied in ApplyDeployment and the EnsureDeployment->ensurePodTemplateSpec->ensurePodSpec->ensureContainers->ensureContainer->ensureEnvVar chain stomps the updated value in required by the old value from existing and reverts the injection in this way:

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

Lines 115 to 120 in dc1ad0a

// Currently only CVO deployment uses this variable to inject internal LB host.

// This may result in an IP address being returned by API so assuming the

// returned value is correct.

if required[envidx].Name == "KUBERNETES_SERVICE_HOST" {

ensureEnvVarKubeService(*existing, &required[envidx])

}

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

Lines 132 to 138 in dc1ad0a

func ensureEnvVarKubeService(existing []corev1.EnvVar, required *corev1.EnvVar) {

for envidx := range existing {

if existing[envidx].Name == required.Name {

required.Value = existing[envidx].Value

}

}

}

This behavior was added intentionally in #559, but I do not understand the reasoning given in the comment:

Currently only CVO deployment uses KUBERNETES_SERVICE_HOST env variable to inject internal LB host.This may result in an IP address being returned by API so always assuming returned value is okay.

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.

petr-muller · 2022-10-14T14:34:24Z

/test all

petr-muller · 2022-10-14T14:42:45Z

/test unit

petr-muller · 2022-10-16T14:25:06Z

/retest

petr-muller · 2022-10-17T11:26:50Z

/test all

petr-muller · 2022-10-17T16:51:45Z

/retest

petr-muller · 2022-10-24T13:06:50Z

/jira refresh

openshift-ci-robot · 2022-10-24T13:06:59Z

@petr-muller: This pull request references Jira Issue OCPBUGS-1458, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.12.0) matches configured target version for branch (4.12.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shellyyang1989

Details

In response to this:

/jira 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.

openshift-ci-robot · 2022-10-24T14:18:13Z

@petr-muller: This pull request references Jira Issue OCPBUGS-1458, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.12.0) matches configured target version for branch (4.12.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shellyyang1989

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

The problem was identified to be a broken substitution of internal load balancer into KUBERNETES_SERVICE_HOST by Trevor and David (see my JIRA comment and related Slack thread).

CVO injects the LB hostname in the ModifyDeployment fine, but then the deployment gets applied in ApplyDeployment and the EnsureDeployment->ensurePodTemplateSpec->ensurePodSpec->ensureContainers->ensureContainer->ensureEnvVar chain stomps the updated value in required by the old value from existing and reverts the injection in this way:

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

Lines 115 to 120 in dc1ad0a

// Currently only CVO deployment uses this variable to inject internal LB host.

// This may result in an IP address being returned by API so assuming the

// returned value is correct.

if required[envidx].Name == "KUBERNETES_SERVICE_HOST" {

ensureEnvVarKubeService(*existing, &required[envidx])

}

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

Lines 132 to 138 in dc1ad0a

func ensureEnvVarKubeService(existing []corev1.EnvVar, required *corev1.EnvVar) {

for envidx := range existing {

if existing[envidx].Name == required.Name {

required.Value = existing[envidx].Value

}

}

}

This behavior was added intentionally in #559 as a part of a fix for various hot-looping issues. The substitution apparently caused some hot-looping issues in the past (slack thread). I have tested removing the special handling KUBERNETES_SERVICE_HOST thoroughly, and saw no problematic behavior. After fixing other hot-looping problems in #855 to eliminate noise, no new hot-loops occurs with KUBERNETES_SERVICE_HOST handling removed.

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.

petr-muller · 2022-10-24T14:18:26Z

/wip

petr-muller · 2022-10-24T14:24:29Z

Needs #855 first

petr-muller · 2022-10-24T17:07:38Z

/retest

petr-muller · 2022-11-02T16:24:52Z

@wking @LalatenduMohanty I was seeing some surprising behavior of the timeout in PollImmediate. That method does not seem to interact well with ConditionFuncs that take a long time to execute (as in our case, where the client-go retry logic can make the GET take 30 seconds to execute). PollImmediate as implemented will always execute at least three iterations of ConditionFunc even if the timeout expires earlier, check out the testcase here: https://go.dev/play/p/frtOTVNGOHk. This means that our failure cases would cause CVO to shutdown in 90s in worst cases.

I solved this by implementing the timeout with a context instead, happy to hear your thoughts.

petr-muller · 2022-11-03T15:32:45Z

upgrade-out-of-change failure does not seem related:

alert TargetDown fired for 990 seconds with labels: {job="prometheus-k8s", namespace="openshift-monitoring", service="prometheus-k8s", severity="warning"}

/retest

petr-muller · 2022-11-04T12:09:26Z

[sig-node] nodes should not go unready after being upgraded and go unready only once expand_more :/
/retest

The client-go code retries a subset of network errors on GET for 30s, but we saw occurrences of other short disruptions, like DNS ones, that make us abort and restart unnecessarily soon. Make CVO retry all errors for 25s and only abort when we do not succeed in this time frame. This helps CVO survive short disruptions on startup, leading to less noise, mostly during installation.

petr-muller · 2022-11-07T20:19:45Z

TestCVO_UpgradeFailedPayloadLoadWithCapsChanges
/test unit

wking

/lgtm

openshift-ci · 2022-11-07T20:24:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, wking

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~~ [wking]

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

petr-muller · 2022-11-08T01:00:18Z

/retest

openshift-bot · 2022-11-08T11:10:16Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on main branch need to target OCP 4.12), recalculating validity.

openshift-ci · 2022-11-08T11:10:42Z

@openshift-bot: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Retaining the bugzilla/valid-bug label as it was manually added.

Details

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on main branch need to target OCP 4.12), recalculating validity.

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.

petr-muller · 2022-11-08T11:19:42Z

/retest

petr-muller · 2022-11-08T11:54:42Z

/jira refresh

openshift-ci-robot · 2022-11-08T11:54:44Z

@petr-muller: This pull request references Jira Issue OCPBUGS-1458, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shellyyang1989

Details

In response to this:

/jira 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.

petr-muller · 2022-11-08T14:16:51Z

/test e2e-agnostic-upgrade-into-change

petr-muller · 2022-11-09T02:11:58Z

/retest

petr-muller · 2022-11-09T10:11:26Z

/retest

petr-muller · 2022-11-09T12:44:22Z

/test e2e-agnostic-upgrade-out-of-change

petr-muller · 2022-11-09T13:23:42Z

/test e2e-agnostic-upgrade-out-of-change

petr-muller · 2022-11-09T16:48:25Z

/test e2e-agnostic-upgrade-out-of-change

openshift-ci · 2022-11-09T19:37:47Z

@petr-muller: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-ci-robot · 2022-11-09T19:42:38Z

@petr-muller: All pull requests linked via external trackers have merged:

openshift/cluster-version-operator#851

Jira Issue OCPBUGS-1458 has been moved to the MODIFIED state.

Details

In response to this:

The problem was identified to be a broken substitution of internal load balancer into KUBERNETES_SERVICE_HOST by Trevor and David (see my JIRA comment and related Slack thread).

CVO injects the LB hostname in the ModifyDeployment fine, but then the deployment gets applied in ApplyDeployment and the EnsureDeployment->ensurePodTemplateSpec->ensurePodSpec->ensureContainers->ensureContainer->ensureEnvVar chain stomps the updated value in required by the old value from existing and reverts the injection.

This behavior was added intentionally in #559 as a part of a fix for various hot-looping issues. The substitution apparently caused some hot-looping issues in the past (slack thread). I have tested removing the special handling KUBERNETES_SERVICE_HOST thoroughly, and saw no problematic behavior. After fixing other hot-looping problems in #855 to eliminate noise, no new hot-loops occurs with KUBERNETES_SERVICE_HOST handling removed.

Even with the substitution fix, I observed further network problems in CVO (example):
F1024 17:40:24.591942       1 start.go:29] error: Get "https://api-int.ci-op-02b7hikv-3302f.ci.azure.devcluster.openshift.com:6443/apis/config.openshift.io/v1/featuregates/cluster": dial tcp: lookup api-int.ci-op-02b7hikv-3302f.ci.azure.devcluster.openshift.com on 172.30.0.10:53: read udp 169.254.169.2:48475->172.30.0.10:53: i/o timeout
To get rid of these, I added a client-side poll to CVO, but I currently fail to reproduce the issue in #859, so maybe the i/o timeout issues were just some co-indidental disruption bug that we happened to fix in the meantime? Not sure.

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.

petr-muller · 2022-11-16T14:46:33Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-11-16T14:47:15Z

@petr-muller: new pull request created: #866

Details

In response to this:

/cherry-pick release-4.12

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 the jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. label Oct 13, 2022

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Oct 13, 2022

openshift-ci bot requested review from LalatenduMohanty and wking October 13, 2022 16:45

petr-muller force-pushed the ocpbugs-1458-bootstrap-crashloop branch 2 times, most recently from 406dac8 to afcb8c8 Compare October 21, 2022 13:46

openshift-ci bot requested a review from shellyyang1989 October 24, 2022 13:07

petr-muller changed the title ~~OCPBUGS-1458: Reproduce KUBERNETES_SERVICE_HOST failing to reconcile~~ OCPBUGS-1458: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address Oct 24, 2022

petr-muller changed the title ~~OCPBUGS-1458: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address~~ WIP: OCPBUGS-1458: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address Oct 24, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2022

petr-muller force-pushed the ocpbugs-1458-bootstrap-crashloop branch 2 times, most recently from f8be0cd to 088b109 Compare October 24, 2022 14:27

petr-muller force-pushed the ocpbugs-1458-bootstrap-crashloop branch from 296ae5f to 6b84006 Compare November 7, 2022 19:57

petr-muller force-pushed the ocpbugs-1458-bootstrap-crashloop branch from 6b84006 to 542520d Compare November 7, 2022 20:09

wking approved these changes Nov 7, 2022

View reviewed changes

openshift-ci bot assigned wking Nov 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

openshift-merge-robot merged commit dfcfbaa into openshift:master Nov 9, 2022

openshift-cherrypick-robot mentioned this pull request Nov 16, 2022

[release-4.12] OCPBUGS-3770: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address #866

Merged

OCPBUGS-1458: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address #851

OCPBUGS-1458: Allow CVO to update KUBERNETES_SERVICE_HOST with LB address #851

Uh oh!

Conversation

petr-muller commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 13, 2022

Uh oh!

openshift-ci-robot commented Oct 13, 2022

Uh oh!

petr-muller commented Oct 14, 2022

Uh oh!

petr-muller commented Oct 14, 2022

Uh oh!

petr-muller commented Oct 16, 2022

Uh oh!

petr-muller commented Oct 17, 2022

Uh oh!

petr-muller commented Oct 17, 2022

Uh oh!

petr-muller commented Oct 24, 2022

Uh oh!

openshift-ci-robot commented Oct 24, 2022

Uh oh!

openshift-ci-robot commented Oct 24, 2022

Uh oh!

petr-muller commented Oct 24, 2022

Uh oh!

petr-muller commented Oct 24, 2022

Uh oh!

petr-muller commented Oct 24, 2022

Uh oh!

petr-muller commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented Nov 4, 2022

Uh oh!

petr-muller commented Nov 7, 2022

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 7, 2022

Uh oh!

petr-muller commented Nov 8, 2022

Uh oh!

openshift-bot commented Nov 8, 2022

Uh oh!

openshift-ci bot commented Nov 8, 2022

Uh oh!

petr-muller commented Nov 8, 2022

Uh oh!

petr-muller commented Nov 8, 2022

Uh oh!

openshift-ci-robot commented Nov 8, 2022

Uh oh!

petr-muller commented Nov 8, 2022

Uh oh!

petr-muller commented Nov 9, 2022

Uh oh!

petr-muller commented Nov 9, 2022

Uh oh!

petr-muller commented Nov 9, 2022

Uh oh!

petr-muller commented Nov 9, 2022

Uh oh!

petr-muller commented Nov 9, 2022

Uh oh!

openshift-ci bot commented Nov 9, 2022

Uh oh!

openshift-ci-robot commented Nov 9, 2022

Uh oh!

petr-muller commented Nov 16, 2022

Uh oh!

openshift-cherrypick-robot commented Nov 16, 2022

Uh oh!

OCPBUGS-1458: Allow CVO to update `KUBERNETES_SERVICE_HOST` with LB address #851

OCPBUGS-1458: Allow CVO to update `KUBERNETES_SERVICE_HOST` with LB address #851

petr-muller commented Oct 13, 2022 •

edited

Loading

petr-muller commented Nov 2, 2022 •

edited

Loading

petr-muller commented Nov 3, 2022 •

edited

Loading