Elastic Agent: Set status.ObservedGeneration from metadata.Generation #5383

naemono · 2022-02-15T16:14:58Z

This change is Elastic Agent specific at the moment

related to #3392
related to #5331

This change allows status.ObservedGeneration to be kept in sync with metadata.Generation according to the rules described in the above issue. This change adds a set of unit tests which show more details into when observedGeneration will, and will not be updated according to any errors that may occur during reconciliation. An end-to-end test is also added to show how the change is intended to function in a true cluster.

pkg/controller/agent/controller.go

test/e2e/agent/observed_generation_test.go

pkg/controller/agent/controller_test.go

Add tests for agent observedGeneration. modify agent reconcile behavior to ensure status updates for certain situations. adjust common comparison to allow additional options Update some documentation. Make disabling of k8s client's errors simpler. Make the status update more readable. Configure initial state for Elastic Agent properly from current state. Add missing comments for some exported data. Make errors consistent with capitalization, and namespace+name. Rename e2e test name to be more descriptive. Update pod labels in e2e test to properly trigger generation update. Remove failing client tests. used keyed structs in tests.

…nge.

removed erroneous cmp.AllowUnexperted()

… test steps. remove WithDefaultESValidation from agent generation e2e test.

Remove unneeded struct in tests. Move Generation e2e tests to it's own package, and use it in all Agent mutation tests.

… observedGeneration.

barkbay · 2022-02-23T06:30:02Z

pkg/controller/agent/controller.go

@@ -153,25 +153,44 @@ func (r *ReconcileAgent) Reconcile(ctx context.Context, request reconcile.Reques
 func (r *ReconcileAgent) doReconcile(ctx context.Context, agent agentv1alpha1.Agent) *reconciler.Results {
 	defer tracing.Span(&ctx)()
 	results := reconciler.NewResult(ctx)
+	status := newStatus(agent)
 	if !association.AreConfiguredIfSet(agent.GetAssociations(), r.recorder) {


I think observedGeneration should be updated even if associations are not correctly set. One reason is that misconfigured associations are not immediately detected. Here is an example:

Deploy config/recipes/elastic-agent/fleet-kubernetes-integration.yaml

observerdGeneration is eventually updated:

k get agent.agent.k8s.elastic.co/fleet-server -o go-template='{{ printf "\nGeneration: "}}{{.metadata.generation}}{{printf "\n\nStatus:"}}{{.status}}' Generation: 2 Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Established observedGeneration:2 version:8.0.0]

⬆️ generation and observedGeneration are both set to 2

Break one of the association, for example Kibana, observedGeneration is still updated:

Generation: 3 Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Pending observedGeneration:3 version:8.0.0

⬆️ generation and observedGeneration are both set to 3

Update the Agent resource while keeping the Kibana association broken, observedGeneration is no more updated, which seems a bit confusing to me and does not match the description "If the generation observed in status diverges from the generation in metadata, the Elastic Agent controller has not yet processed the changes contained in the Elastic Agent specification."

k get agent.agent.k8s.elastic.co/fleet-server -o go-template='{{ printf "\nGeneration: "}}{{.metadata.generation}}{{printf "\n\nStatus:"}}{{.status}}' Generation: 4 Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Pending observedGeneration:3 version:8.0.0]

⬆️ generation has been updated to 4, but observedGeneration is still 3

This logic has been updated, and I'll get these steps tested.

pkg/controller/agent/reconcile.go

test/e2e/test/generation/generation.go

Updating E2E tests to have similar design as Kibana, and ES E2E tests for generation bits.

…392-agent-observedGeneration

…unction properly.

naemono · 2022-03-10T21:42:41Z

Last update resolves this issue with associations @barkbay

initial

❯ kc get agent.agent.k8s.elastic.co/fleet-server -o go-template='{{ printf "\nGeneration: "}}{{.metadata.generation}}{{printf "\n\nStatus:"}}{{.status}}'

Generation: 2

Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Established observedGeneration:2 version:8.0.0]

broken kibana assocciation

❯ kc get agent.agent.k8s.elastic.co/fleet-server -o go-template='{{ printf "\nGeneration: "}}{{.metadata.generation}}{{printf "\n\nStatus:"}}{{.status}}'

Generation: 3

Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Pending observedGeneration:3 version:8.0.0]

spec updated, observedGeneration updated:

❯ kc get agent.agent.k8s.elastic.co/fleet-server -o go-template='{{ printf "\nGeneration: "}}{{.metadata.generation}}{{printf "\n\nStatus:"}}{{.status}}'

Generation: 4

Status:map[availableNodes:1 elasticsearchAssociationsStatus:map[default/elasticsearch:Established] expectedNodes:1 health:green kibanaAssociationStatus:Pending observedGeneration:4 version:8.0.0]

pkg/controller/agent/reconcile.go

pebrc · 2022-03-15T14:12:36Z

pkg/controller/agent/controller.go

+		if agent == nil {
+			return reconcile.Result{}, err
+		}
+		return updateStatus(ctx, *agent, r.Client, newStatus(*agent)).WithError(err).Aggregate()


Which case are we covering here? If we cannot fetch the Agent resource we cannot update. The case where we can fetch the Agent resource but the JSON in the association config is invalid is the only one I can think of. We do not handle this case for the Kibana resource as far as I can tell. I think it would be good to be somewhat consistent here.

So assuming I am not missing an obvious case (totally possible), I am not fully decided which way we should go here. Either we consider this case too much of an edge case to warrant updating the status in two places (which I would be OK with at this point) or we try to bring all the status update functionality into the same place.

I've just removed updating the status on association failures, as it seems to be causing more issues than it's worth.

pkg/controller/agent/controller.go

pkg/controller/agent/controller_test.go

pkg/controller/agent/controller.go

pkg/controller/agent/reconcile.go

Don't worry about status update when FetchWithASsociations fails. Just return errors from updateStatus

pkg/controller/agent/reconcile.go

pkg/controller/agent/controller.go

pebrc · 2022-03-31T12:44:09Z

pkg/controller/agent/reconcile.go

@@ -27,7 +27,7 @@ import (
 	"github.com/elastic/cloud-on-k8s/pkg/utils/pointer"
 )

-func reconcilePodVehicle(params Params, podTemplate corev1.PodTemplateSpec) *reconciler.Results {
+func reconcilePodVehicle(params Params, podTemplate corev1.PodTemplateSpec, status *agentv1alpha1.AgentStatus) *reconciler.Results {


What's a bit messy here is that on the one hand we have a more functional approach where results are returned and not pointers manipulated:

reconcilePodVehicle(...) *reconcilerResults

and now we are introducing status here that is following another idiom where a pointer to a struct is passed and the underlying struct is manipulated: sort of a hidden return value if you will.

reconcilePodVehicle(&status)

I see your point here. I have refactored this to return both a status and a results pointer throughout the reconciliation. Lmk if that is what you were intending.

Update status logic to update health properly. Add unit test for updating of health in agent status.

pebrc

Almost LGTM!

pkg/controller/agent/controller.go

pebrc · 2022-04-04T12:09:25Z

pkg/controller/association/reconciler.go

+			"name", associatedKey.Name)
+		return results.WithResult(reconcile.Result{Requeue: true}).Aggregate()
+	} else if err != nil {
+		log.Error(


^ this is still open

pkg/controller/agent/reconcile.go

pkg/controller/agent/controller.go

pebrc

LGTM!

elastic#5383) * Add agent observed generation. Add tests for agent observedGeneration. modify agent reconcile behavior to ensure status updates for certain situations. adjust common comparison to allow additional options Update some documentation. Make disabling of k8s client's errors simpler. Make the status update more readable. Configure initial state for Elastic Agent properly from current state. Add missing comments for some exported data. Make errors consistent with capitalization, and namespace+name. Rename e2e test name to be more descriptive. Update pod labels in e2e test to properly trigger generation update. Remove failing client tests. used keyed structs in tests. * Run generation and update yaml for observedGeneration description change. * Properly order imports * capitalize Agent * remove erroneous debugging line * renamed agent e2e test file. removed erroneous cmp.AllowUnexperted() * Ignore generation when checking if agent status is updated during k8s test steps. remove WithDefaultESValidation from agent generation e2e test. * remove unneeded nil in return from rebase. * Add missing header to e2e test. * Remove unused args struct * move imports around properly * Defer agent validation to reduce repetitive code. Remove unneeded struct in tests. Move Generation e2e tests to it's own package, and use it in all Agent mutation tests. * Updating metav1.Object -> client.Object, and using type switch to get observedGeneration. * Allow status to be updated even if associations are broken. * Allow error to be returned from doReconcile during defer. Updating E2E tests to have similar design as Kibana, and ES E2E tests for generation bits. * ensure withmutatedfrom is used in e2e agent tests. * Ensure we're not returning nil results * Fixing comparison issues from rebase/merge * Add nolint for the naked return, which is required for the defer to function properly. * Handle updating status properly when associations are broken. * Debugging * Debugging, and moving the defer * Go back to previous way of handling fetch association errors * remove extraneous logging statement * Move status update to calling function. Don't worry about status update when FetchWithASsociations fails. Just return errors from updateStatus * Remove unused context * Update the func's comments to be consistent * remove unneeded return * move common.LowestVersionFromPods closer to where pods are retrieved. Update status logic to update health properly. Add unit test for updating of health in agent status. * changes to return status instead of modifying a pointer. * rename variable * Updating how status is handled for consistency

botelastic bot added the triage label Feb 15, 2022

naemono changed the title ~~Add agent observed generation.~~ Elastic Agent: Set status.ObservedGeneration from metadata.Generation Feb 15, 2022

barkbay added the >enhancement Enhancement of existing functionality label Feb 16, 2022

botelastic bot removed the triage label Feb 16, 2022

naemono force-pushed the 3392-agent-observedGeneration branch from 818892f to cbf499e Compare February 16, 2022 15:27

naemono marked this pull request as ready for review February 16, 2022 20:11

pebrc reviewed Feb 21, 2022

View reviewed changes

naemono added 13 commits February 22, 2022 08:03

Run generation and update yaml for observedGeneration description cha…

5bc9db5

…nge.

Properly order imports

8b7e14a

capitalize Agent

9de32ab

remove erroneous debugging line

f77b470

renamed agent e2e test file.

30f6c8f

removed erroneous cmp.AllowUnexperted()

Ignore generation when checking if agent status is updated during k8s…

c6661bb

… test steps. remove WithDefaultESValidation from agent generation e2e test.

remove unneeded nil in return from rebase.

58ed712

Add missing header to e2e test.

b6045d0

Remove unused args struct

1374e88

move imports around properly

5e03689

Defer agent validation to reduce repetitive code.

b96cd85

Remove unneeded struct in tests. Move Generation e2e tests to it's own package, and use it in all Agent mutation tests.

Updating metav1.Object -> client.Object, and using type switch to get…

2bc4fd9

… observedGeneration.

naemono force-pushed the 3392-agent-observedGeneration branch from b595041 to 2bc4fd9 Compare February 22, 2022 14:04

barkbay reviewed Feb 23, 2022

View reviewed changes

test/e2e/test/generation/generation.go Outdated Show resolved Hide resolved

pebrc mentioned this pull request Feb 24, 2022

Kibana: Set status.ObservedGeneration from metadata.Generation #5409

Merged

naemono added 6 commits March 9, 2022 16:32

Allow status to be updated even if associations are broken.

a58c2da

Allow error to be returned from doReconcile during defer.

250fcb5

Updating E2E tests to have similar design as Kibana, and ES E2E tests for generation bits.

Merge branch 'main' into 3392-agent-observedGeneration

a78d66d

Merge branch 'main' of https://github.com/elastic/cloud-on-k8s into 3…

27027d0

…392-agent-observedGeneration

ensure withmutatedfrom is used in e2e agent tests.

8b549f8

Ensure we're not returning nil results

28731f7

naemono added 6 commits March 10, 2022 14:28

Fixing comparison issues from rebase/merge

ea5935e

Add nolint for the naked return, which is required for the defer to f…

c10da1b

…unction properly.

Handle updating status properly when associations are broken.

3d9975d

Debugging

bc4f919

Debugging, and moving the defer

ecad6df

Go back to previous way of handling fetch association errors

4c8d2ec

remove extraneous logging statement

d7844d7

pebrc requested changes Mar 15, 2022

View reviewed changes

naemono added 4 commits March 17, 2022 09:32

Move status update to calling function.

205b1cf

Don't worry about status update when FetchWithASsociations fails. Just return errors from updateStatus

Remove unused context

5177545

Update the func's comments to be consistent

b850b4f

remove unneeded return

4e6f801

pebrc mentioned this pull request Mar 24, 2022

Elastic APM Server: Set status.ObservedGeneration from metadata.Generation #5470

Merged

Merge branch 'main' into 3392-agent-observedGeneration

f782303

naemono added the >feature Adds or discusses adding a feature to the product label Mar 30, 2022

pebrc reviewed Mar 31, 2022

View reviewed changes

naemono added 3 commits March 31, 2022 12:30

move common.LowestVersionFromPods closer to where pods are retrieved.

0dea707

Update status logic to update health properly. Add unit test for updating of health in agent status.

changes to return status instead of modifying a pointer.

074933c

rename variable

2894c06

pebrc reviewed Apr 4, 2022

View reviewed changes

Updating how status is handled for consistency

052fb3f

pebrc approved these changes Apr 5, 2022

View reviewed changes

naemono merged commit c4af92c into elastic:main Apr 5, 2022

naemono deleted the 3392-agent-observedGeneration branch April 5, 2022 12:51

thbkrkr mentioned this pull request Jun 26, 2023

TestFleetAgentWithoutTLS/ES_data_should_pass_validations flaky in 8-0-1 #6956

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elastic Agent: Set status.ObservedGeneration from metadata.Generation #5383

Elastic Agent: Set status.ObservedGeneration from metadata.Generation #5383

naemono commented Feb 15, 2022

barkbay Feb 23, 2022

naemono Mar 9, 2022

naemono commented Mar 10, 2022

pebrc Mar 15, 2022

naemono Mar 17, 2022

pebrc Mar 31, 2022

naemono Mar 31, 2022

pebrc left a comment

pebrc Apr 4, 2022

pebrc left a comment

Elastic Agent: Set status.ObservedGeneration from metadata.Generation #5383

Elastic Agent: Set status.ObservedGeneration from metadata.Generation #5383

Conversation

naemono commented Feb 15, 2022

barkbay Feb 23, 2022

Choose a reason for hiding this comment

naemono Mar 9, 2022

Choose a reason for hiding this comment

naemono commented Mar 10, 2022

pebrc Mar 15, 2022

Choose a reason for hiding this comment

naemono Mar 17, 2022

Choose a reason for hiding this comment

pebrc Mar 31, 2022

Choose a reason for hiding this comment

naemono Mar 31, 2022

Choose a reason for hiding this comment

pebrc left a comment

Choose a reason for hiding this comment

pebrc Apr 4, 2022

Choose a reason for hiding this comment

pebrc left a comment

Choose a reason for hiding this comment