Report the lowest running version in the status of each resource #3489

sebgl · 2020-07-20T15:37:43Z

Report the currently running version in the status.version field of
each resource. In case a version upgrade is in progress, report the
lowest running version.

For Elasticsearch, we check both the Pods and the StatefulSets to report
the lowest version. A StatefulSet might report a higher version whereas
the Pods haven't been upgraded yet. It may also report a lower version
and no Pods are running any more with that version.
For other resources, we just check the Pods.

Parts of the Status is refactored into a common DeploymentStatus for
Kibana, ApmServer and EnterpriseSearch. Elasticsearch and Beat have
different requirements which cannot fit this common place.

This is a pre-req for strict stack version upgrade ordering
(#2600).

Report the currently running version in the `status.version` field of each resource. In case a version upgrade is in progress for that resource, report the lowest running version. For Elasticsearch, we check both the Pods and the StatefulSets to report the lowest version. A StatefulSet might report a higher version whereas the Pods haven't been upgraded yet. It may also report a lower version and no Pods are running any more with that version. For other resources, we just check the Pods. Parts of the Status is refactored into a common `DeploymentStatus` for Kibana, ApmServer and EnterpriseSearch. Elasticsearch and Beat have different requirements which cannot fit this common place. This is a pre-req for strict stack version upgrade ordering (elastic#2600).

sebgl · 2020-07-20T15:46:18Z

pkg/controller/elasticsearch/reconcile/resources_state.go

@@ -25,6 +27,8 @@ type ResourcesState struct {
 	CurrentPodsByPhase map[corev1.PodPhase][]corev1.Pod
 	// DeletingPods are all deleted Elasticsearch pods.
 	DeletingPods []corev1.Pod
+	// StatefulSets are all existing StatefulSets for the cluster.
+	StatefulSets sset.StatefulSetList


It may feel a bit strange to retrieve StatefulSets here for the only purpose of reporting the version in the status, but I think it's still way cleaner than propagating the k8s client to the function that updates the status data, that did not really need it.
I'm also thinking we should invest more in this struct, instead of retrieving the Pods and StatefulSets several times in a single reconciliation. See #3490.

david-kow

Looks good overall.

For Elasticsearch, we check both the Pods and the StatefulSets to report
the lowest version. A StatefulSet might report a higher version whereas
the Pods haven't been upgraded yet. It may also report a lower version
and no Pods are running any more with that version.

Isn't this the case for Deployments/DaemonSets too?

david-kow · 2020-07-21T05:38:45Z

hack/manifest-gen/assets/charts/eck/crds/all-crds.yaml

+              description: Health of the deployment.
+              type: string
+            version:
+              description: 'Version of the stack resource currently running. During


Why this has quotes around it while other descriptions don't?

I think because it is spread over more than 1 line. I tested with a single line in the comment: quotes are removed.
So I think it's fine.

pkg/controller/common/version/version.go

david-kow · 2020-07-21T06:22:29Z

docs/reference/api-docs.asciidoc

@@ -265,6 +265,8 @@ ConfigSource references configuration settings.
 |===


+
+


nit: remove?

I don't know where it comes from, it is auto-generated this way 😕
I see other places in this file with a lot of empty lines.
@charith-elastic any idea?

They don't get rendered IIRC so I'm not sure it's worth spending too much time on

sebgl · 2020-07-22T07:51:47Z

For Elasticsearch, we check both the Pods and the StatefulSets to report
the lowest version. A StatefulSet might report a higher version whereas
the Pods haven't been upgraded yet. It may also report a lower version
and no Pods are running any more with that version.

Isn't this the case for Deployments/DaemonSets too?

I think it is slightly different because we reconcile a single DaemonSet/Deployment per resource, whereas we have multiple StatefulSets per Elasticsearch resource:

We may be in a situation where a Deployment reports version N+1 and no Pods are running. In this case we keep the last observed version (eg. N). Once the Pods are running again they should be created in version N+1 so we can just use it at this point in time. I don't see a case where inspecting the Pods only would cause a version mismatch.
When dealing with StatefulSets we may have 2/3 StatefulSets updated to version N+1. If Pods of the third StatefulSet are temporarily not running, all Pods may report version N+1, but Pods of that 3rd StatefulSet may be recreated in version N anytime soon. So we should not report version N+1 until all Pods and StatefulSets report N+1.

pebrc

LGTM

pkg/controller/common/version/version.go

pebrc · 2020-07-22T07:39:23Z

pkg/controller/common/version/version_test.go

+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {


Nice, very easy to read. We should probably clean up the verbose ones I wrote above at some point (not necessarily this PR)

Do you have one (or several) in mind specifically?

The TestMin one that uses a lot of vertical space for the Version declarations which could be expressed in a more compact form for readability. But it is really not that important.

thbkrkr

Nice!

pkg/utils/k8s/k8sutils.go

pkg/apis/common/v1/common.go

sebgl added >enhancement Enhancement of existing functionality v1.3.0 labels Jul 20, 2020

sebgl mentioned this pull request Jul 20, 2020

Refactor how the Elasticsearch controller retrieves resources (rely on ResourcesState?) #3490

Open

sebgl commented Jul 20, 2020

View reviewed changes

david-kow reviewed Jul 21, 2020

View reviewed changes

Fix function comment

aa06927

pebrc approved these changes Jul 22, 2020

View reviewed changes

Remove elasticsearch label.MinVersion()

3bb74a8

thbkrkr approved these changes Jul 22, 2020

View reviewed changes

pkg/utils/k8s/k8sutils.go Outdated Show resolved Hide resolved

pkg/apis/common/v1/common.go Outdated Show resolved Hide resolved

sebgl added 2 commits July 22, 2020 11:49

Fix typo

ff13ff1

Improve comment wording

31ca231

sebgl merged commit 2e06a7c into elastic:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report the lowest running version in the status of each resource #3489

Report the lowest running version in the status of each resource #3489

sebgl commented Jul 20, 2020 •

edited

Loading

sebgl Jul 20, 2020

david-kow left a comment

david-kow Jul 21, 2020

sebgl Jul 21, 2020

david-kow Jul 21, 2020

sebgl Jul 21, 2020 •

edited

Loading

anyasabo Jul 21, 2020

sebgl commented Jul 22, 2020

pebrc left a comment

pebrc Jul 22, 2020

sebgl Jul 22, 2020

pebrc Jul 22, 2020

thbkrkr left a comment

		@@ -265,6 +265,8 @@ ConfigSource references configuration settings.
		\|===

Report the lowest running version in the status of each resource #3489

Report the lowest running version in the status of each resource #3489

Conversation

sebgl commented Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

david-kow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebgl Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebgl commented Jul 22, 2020

pebrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thbkrkr left a comment

Choose a reason for hiding this comment

sebgl commented Jul 20, 2020 •

edited

Loading

sebgl Jul 21, 2020 •

edited

Loading