[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil #495

ewolinetz · 2020-09-23T21:15:18Z

To address https://bugzilla.redhat.com/show_bug.cgi?id=1881957
Manual cherry-pick of #471

This addresses cases where there is or was an existing cluster created (deployments, pods, pvcs, etc) who's cluster name is the same as the CR name, however the new elasticsearch CR is missing UUIDs.

As part of the recovery/adoption process, it will be required that the PVCs to be picked back up have the label logging-cluster: . It will also be validated against the name of the cluster that the PVC name is based on.

Recovery/adoption will be triggered upon the processing of a CR that is missing UUIDs. It will only try to recover UUIDs for nodes that do not already have UUIDs defined.

Further documentation will need to be developed and publish as part of how to recover data that from another PVC. This PR does not seek to resolve that but rather address cases where an elasticsearch CR may have been removed on accident and then recreated (without UUIDs).

Please note: as part of this change EO will be creating PVCs with the required labels now, so to ensure keeping previously used PVCs they should be labeled per above.

openshift-ci-robot · 2020-09-23T21:15:27Z

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

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.z) matches configured target release for branch (4.5.z)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1868300 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA))
dependent Bugzilla bug 1868300 targets the "4.6.0" release, which is one of the valid target releases: 4.6.0, 4.6.z
bug has dependents

In response to this:

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil

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.

based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.

periklis

Looks good to me in general, however I might be wrong but the merge for persistentvolumeclaims blurred some inconsistencies in.

periklis · 2020-09-24T06:52:25Z

pkg/k8shandler/persistentvolumeclaims.go

+	if retryErr != nil {
+		return retryErr


I suggest to simplify the return statement here to retrun retryErr to enhance readbility

periklis · 2020-09-24T06:55:48Z

pkg/k8shandler/persistentvolumeclaims.go

-				if !errors.IsAlreadyExists(err) {
-					return fmt.Errorf("Unable to create PVC: %v", err)
-				}
+	claim := createPersistentVolumeClaim(newName, namespace, clusterName, pvc)


It looks like the method is using the wrong prefix create instead of new. Former indicates normally a POST call to the apiserver while latter only a factory method for the resource we would like to persist later.

periklis · 2020-09-24T06:58:04Z

pkg/k8shandler/persistentvolumeclaims.go

+		return fmt.Errorf("unable to create PVC: %w", err)
+	}
+
+	return updatePersistentVolumeClaim(claim, client)


Why updating the same PVC resource after initial creation? Looks like duplicate work here.

to ensure its in its correct state as we leave this function

periklis · 2020-09-24T06:58:59Z

pkg/k8shandler/persistentvolumeclaims.go

+		if !reflect.DeepEqual(current.ObjectMeta.Labels, claim.ObjectMeta.Labels) {
+			current.ObjectMeta.Labels = claim.ObjectMeta.Labels


Why does this function only update if the labels are not equal?

if nothing is different (that we can change) there isn't a point to making an update call

ewolinetz · 2020-09-24T14:43:56Z

/retest

periklis · 2020-09-24T15:02:09Z

/lgtm

openshift-ci-robot · 2020-09-24T15:02:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ewolinetz, periklis

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

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

markmc · 2020-09-25T10:01:57Z

(patch manager) affecting real-world users

openshift-bot · 2020-09-25T13:09:47Z

/retest

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

openshift-ci-robot · 2020-09-25T14:37:23Z

@ewolinetz: All pull requests linked via external trackers have merged:

openshift/elasticsearch-operator#495

Bugzilla bug 1881957 has been moved to the MODIFIED state.

In response to this:

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil

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.

ewolinetz · 2020-09-28T21:16:16Z

/cherry-pick release-4.4
this was originally reported on 4.4 and likely to still be an issue for customers who haven't upgraded to 4.5 yet

openshift-cherrypick-robot · 2020-09-28T21:16:26Z

@ewolinetz: #495 failed to apply on top of branch "release-4.4":

Applying: updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.
Using index info to reconstruct a base tree...
M	pkg/k8shandler/cluster.go
M	pkg/k8shandler/common.go
A	pkg/k8shandler/migrations/kibana5to6.go
A	pkg/k8shandler/podtemplate.go
A	pkg/k8shandler/podtemplate_test.go
M	pkg/k8shandler/util.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/k8shandler/util.go
CONFLICT (content): Merge conflict in pkg/k8shandler/util.go
CONFLICT (modify/delete): pkg/k8shandler/podtemplate_test.go deleted in HEAD and modified in updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.. Version updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations. of pkg/k8shandler/podtemplate_test.go left in tree.
CONFLICT (modify/delete): pkg/k8shandler/podtemplate.go deleted in HEAD and modified in updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.. Version updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations. of pkg/k8shandler/podtemplate.go left in tree.
CONFLICT (modify/delete): pkg/k8shandler/migrations/kibana5to6.go deleted in HEAD and modified in updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.. Version updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations. of pkg/k8shandler/migrations/kibana5to6.go left in tree.
Auto-merging pkg/k8shandler/common.go
Auto-merging pkg/k8shandler/cluster.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 updating EO to adopt an existing cluster if cr nodes uuid is nil based on PVC names if PVCs currently exist or deployment|statefulset names if PVCs do not exist. if there are no matched UUIDs ones are auto generated. Attempt to best match UUID to a node based on how naming would match up based on role and node count. Also match based on resources, node selectors, and tolerations.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.4
this was originally reported on 4.4 and likely to still be an issue for customers who haven't upgraded to 4.5 yet

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 bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Sep 23, 2020

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

openshift-ci-robot requested review from alanconway and periklis September 23, 2020 21:15

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

ewolinetz force-pushed the 45_reuse_orphaned_uuids branch from e76e234 to 896a559 Compare September 23, 2020 21:52

periklis suggested changes Sep 24, 2020

View reviewed changes

openshift-ci-robot assigned periklis Sep 24, 2020

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

markmc added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 25, 2020

openshift-merge-robot merged commit fb37b53 into openshift:release-4.5 Sep 25, 2020

ewolinetz mentioned this pull request Sep 28, 2020

[release-4.4] Bug 1883355: updating EO to adopt an existing cluster if cr nodes uuid is nil #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil #495

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil #495

ewolinetz commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

periklis left a comment

periklis Sep 24, 2020

periklis Sep 24, 2020

periklis Sep 24, 2020

ewolinetz Sep 24, 2020 •

edited

periklis Sep 24, 2020

ewolinetz Sep 24, 2020

ewolinetz commented Sep 24, 2020

periklis commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

markmc commented Sep 25, 2020 •

edited

openshift-bot commented Sep 25, 2020

openshift-ci-robot commented Sep 25, 2020

ewolinetz commented Sep 28, 2020

openshift-cherrypick-robot commented Sep 28, 2020

		if !reflect.DeepEqual(current.ObjectMeta.Labels, claim.ObjectMeta.Labels) {
		current.ObjectMeta.Labels = claim.ObjectMeta.Labels

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil #495

[release-4.5] Bug 1881957: updating EO to adopt an existing cluster if cr nodes uuid is nil #495

Conversation

ewolinetz commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

periklis left a comment

Choose a reason for hiding this comment

periklis Sep 24, 2020

Choose a reason for hiding this comment

periklis Sep 24, 2020

Choose a reason for hiding this comment

periklis Sep 24, 2020

Choose a reason for hiding this comment

ewolinetz Sep 24, 2020 • edited

Choose a reason for hiding this comment

periklis Sep 24, 2020

Choose a reason for hiding this comment

ewolinetz Sep 24, 2020

Choose a reason for hiding this comment

ewolinetz commented Sep 24, 2020

periklis commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

markmc commented Sep 25, 2020 • edited

openshift-bot commented Sep 25, 2020

openshift-ci-robot commented Sep 25, 2020

ewolinetz commented Sep 28, 2020

openshift-cherrypick-robot commented Sep 28, 2020

ewolinetz Sep 24, 2020 •

edited

markmc commented Sep 25, 2020 •

edited