Bug 1832923: Forget bootstrap etcd member IP after bootstrap #367

ironcladlou · 2020-06-01T15:32:52Z

After bootstrap completes, forget the bootstrap etcd member IP by clearing
the annotation from the endpoint configmap. Otherwise the etcd client will
continue failing to connect to the dead bootstrap node forever.

ironcladlou · 2020-06-01T15:39:19Z

/test e2e-aws-upgrade

openshift-ci-robot · 2020-06-01T15:39:33Z

@ironcladlou: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test e2e-aws
/test e2e-aws-disruptive
/test e2e-azure
/test e2e-gcp
/test e2e-gcp-upgrade
/test e2e-metal-ipi
/test images
/test unit
/test verify
/test verify-deps

Use /test all to run all jobs.

In response to this:

/test e2e-aws-upgrade

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.

ironcladlou · 2020-06-01T18:41:03Z

Latest failures are I think unrelated, and I have a successful 4.4 to 4.5 upgrade job. I want to do a little refactoring and add test coverage for this controller.

openshift-ci-robot · 2020-06-01T18:41:32Z

@ironcladlou: This pull request references Bugzilla bug 1832923, 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.

3 validation(s) were run on this bug

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

In response to this:

Bug 1832923: Forget bootstrap etcd member IP after bootstrap

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.

ironcladlou · 2020-06-02T00:18:16Z

@hexfusion @retroflexer @deads2k refactored, added a bunch of tests, re-running the Y upgrade test. Tomorrow I might go ahead and continue expanding the unit test while I'm in here to cover node=>configmap translation scenarios.

ironcladlou · 2020-06-02T13:11:19Z

/retest

ironcladlou · 2020-06-02T13:43:51Z

There's a problem here. In a real cluster the annotation is never cleaned up because the revision checks falsely detect a rollout because nodestates.targetrevision is empty. So, the unit tests are inaccurate. I need clarification on the semantics around these fields:

https://github.com/openshift/api/blob/master/operator/v1/types.go#L208

What does an empty TargetRevision mean?
What does an empty LatestAvailableRevision mean?

ironcladlou · 2020-06-02T14:44:48Z

Now I'm relying on latestAvailableRevision instead of nodestatus.targetRevision.

ironcladlou · 2020-06-02T17:28:07Z

https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-etcd-operator/367/pull-ci-openshift-cluster-etcd-operator-master-e2e-gcp/1530

Unrelated flakes; annotation was removed and etcd related things are reporting available.

ironcladlou · 2020-06-02T19:00:53Z

AWS failures look unrelated:

[sig-network-edge][Conformance][Area:Networking][Feature:Router] The HAProxy router should pass the gRPC interoperability tests [Suite:openshift/conformance/parallel/minimal]
[sig-network][Feature:Router] The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel]

Failing tests:

[sig-network] Services should be rejected when no endpoints exist [Skipped:ibmcloud] [Suite:openshift/conformance/parallel] [Suite:k8s]

https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-etcd-operator/367/pull-ci-openshift-cluster-etcd-operator-master-e2e-azure/1604

Dug into the artifacts and nothing seems amiss with etcd or the apiserver.

/retest

ironcladlou · 2020-06-02T19:06:19Z

@hexfusion @retroflexer this is ready for review

ironcladlou · 2020-06-02T21:24:09Z

Successful 4.4 -> 4.5 run w/ the latest commit: https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-launch-aws/12426

ironcladlou · 2020-06-03T12:41:17Z

/retest

ironcladlou · 2020-06-03T19:17:53Z

/retest

hexfusion

Looks good overall, a few questions.

hexfusion · 2020-06-04T12:06:32Z

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go

 		configmapClient: kubeClient.CoreV1(),
 	}
 	return factory.New().ResyncEvery(time.Minute).WithInformers(
 		operatorClient.Informer(),
-		configmapsInformer.Informer(),
+		kubeInformers.InformersFor(operatorclient.TargetNamespace).Core().V1().ConfigMaps().Informer(),


Curious what is the difference between the former and the latter?

functionally equivalent, both cause sync to trigger on configmap change in the openshift-etcd namespace

OK so why change :)

Because configmapsInformer was only used in one place (so what's the point of a local to begin with), and also had a misleading name (it isn't an informer for all configmaps, it's only for the openshift-etcd namespace).

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go

After bootstrap completes, forget the bootstrap etcd member IP by clearing the annotation from the endpoint configmap. Otherwise the etcd client will continue failing to connect to the dead bootstrap node forever.

hexfusion · 2020-06-04T13:20:42Z

/test images

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go

hexfusion · 2020-06-04T16:36:11Z

/retest

hexfusion · 2020-06-04T20:01:34Z

/test all

hexfusion · 2020-06-05T03:51:24Z

/refresh
/retest

hexfusion · 2020-06-05T13:49:19Z

/lgtm
/approve

openshift-ci-robot · 2020-06-05T13:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hexfusion, ironcladlou

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:

~~OWNERS~~ [hexfusion]

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

openshift-bot · 2020-06-05T13:49:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-06-05T15:34:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-06-05T15:47:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

deads2k · 2020-06-05T16:01:13Z

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go

+	// check to see if bootstrapping is complete
+	bootstrapFinishedConfigMap, err := configMapClient.ConfigMaps("kube-system").Get("bootstrap")
+	if err != nil {
+		if errors.IsNotFound(err) {


style: prefer

if errors.IsNotFound(){ return } if err != nil{ different return }

openshift-bot · 2020-06-05T16:26:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-06-05T16:40:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-06-05T17:18:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-06-05T17:44:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-06-05T18:25:03Z

@ironcladlou: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-disruptive	`c314c0b`	link	`/test e2e-aws-disruptive`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-bot · 2020-06-05T18:36:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-06-05T18:43:00Z

@ironcladlou: All pull requests linked via external trackers have merged: openshift/cluster-etcd-operator#367. Bugzilla bug 1832923 has been moved to the MODIFIED state.

In response to this:

Bug 1832923: Forget bootstrap etcd member IP after bootstrap

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 requested review from hexfusion and retroflexer June 1, 2020 15:33

ironcladlou changed the title ~~Forget bootstrap etcd member IP after bootstrap~~ Bug 1832923: Forget bootstrap etcd member IP after bootstrap Jun 1, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jun 1, 2020

ironcladlou force-pushed the cleanup-bootstrap-endpoint branch 2 times, most recently from 23cbbc0 to 1a23919 Compare June 2, 2020 00:15

ironcladlou force-pushed the cleanup-bootstrap-endpoint branch from 97189b9 to 10c3202 Compare June 2, 2020 16:08

hexfusion reviewed Jun 4, 2020

View reviewed changes

Forget bootstrap etcd member IP after bootstrap

c314c0b

After bootstrap completes, forget the bootstrap etcd member IP by clearing the annotation from the endpoint configmap. Otherwise the etcd client will continue failing to connect to the dead bootstrap node forever.

ironcladlou force-pushed the cleanup-bootstrap-endpoint branch from 10c3202 to c314c0b Compare June 4, 2020 13:14

hexfusion reviewed Jun 4, 2020

View reviewed changes

pkg/operator/etcdendpointscontroller/etcdendpointscontroller.go Show resolved Hide resolved

openshift-ci-robot assigned hexfusion Jun 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2020

deads2k reviewed Jun 5, 2020

View reviewed changes

openshift-merge-robot merged commit abe09ec into openshift:master Jun 5, 2020

Bug 1832923: Forget bootstrap etcd member IP after bootstrap #367

Bug 1832923: Forget bootstrap etcd member IP after bootstrap #367

Conversation

ironcladlou commented Jun 1, 2020

ironcladlou commented Jun 1, 2020

openshift-ci-robot commented Jun 1, 2020

ironcladlou commented Jun 1, 2020

openshift-ci-robot commented Jun 1, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 2, 2020

ironcladlou commented Jun 3, 2020

ironcladlou commented Jun 3, 2020

hexfusion left a comment

Choose a reason for hiding this comment

hexfusion Jun 4, 2020

Choose a reason for hiding this comment

ironcladlou Jun 4, 2020

Choose a reason for hiding this comment

hexfusion Jun 4, 2020

Choose a reason for hiding this comment

ironcladlou Jun 4, 2020

Choose a reason for hiding this comment

hexfusion commented Jun 4, 2020

hexfusion commented Jun 4, 2020

hexfusion commented Jun 4, 2020

hexfusion commented Jun 5, 2020

hexfusion commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

deads2k Jun 5, 2020

Choose a reason for hiding this comment

openshift-bot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020 • edited

openshift-bot commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020 •

edited