Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flapping state of module in kyma-CR #1080

Closed
k15r opened this issue Nov 23, 2023 · 3 comments · Fixed by #1302
Closed

Flapping state of module in kyma-CR #1080

k15r opened this issue Nov 23, 2023 · 3 comments · Fixed by #1302
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@k15r
Copy link

k15r commented Nov 23, 2023

Description

If a module goes into warning state during deletion the kyma-CR will report it alternating Is Deleting and Warning

Steps to reproduce

eg:
install nats module
create resources on nats:

  • port forward nats and use the nats cli
nats stream create foo

delete the nats module

it will go into warning state as the manager will not remove the reconciler (resources exist on the nats cluster)
the result can be seen in the attachment

Environment Type

Managed

Environment Info

Kubernetes Version: x.y.z
Docker Version: x.y.z
Kyma CLI Version: x.y.z
If used - k3d/k3s/minikube versions

Attachments

Screen.Recording.2023-11-23.at.11.54.07.mov
@k15r k15r added the kind/bug Categorizes issue or PR as related to a bug. label Nov 23, 2023
@lindnerby
Copy link
Member

We agreed on investigating the root cause of this behaviour estimate it to 1. Follow-up issue for the fix will be linked here as well.

@jeremyharisch jeremyharisch self-assigned this Jan 24, 2024
@jeremyharisch
Copy link
Contributor

jeremyharisch commented Jan 25, 2024

After investigating into this Issue I have the following conclusion:

  • The reason for this flickering is
    1. In the declarative pkg the status of the Manifest CR will be set to Deleting if the DeletionTimestamp has been set and the state is unequal to Deleting, then it triggers a new reconiliation
    2. Then if the DeletionTimestamp is set and the state is equal to Deleting it continues with the normal workflow of synchronising the resources, thus if the Module CR has the state Warning (even with set DeletionTimestamp), the state will be propagated to the Manifest CR -> Manifest has now Warning state
    3. Then it starts again with point 1.
  • The scenario was "covered" in the E2E test, but due to a wrong implementation of the code, the test was green -> Test needs to be adapted + We need a new feature in the Template-Operator to simulate this scenario

Acceptance Criterias:

  • Add new flag to Template-Operator which allows to set the State when Module is in Deleting; default Deleting -> Can be used to simulate more complex scenarios like the one describes in this ticket; I.e. Final-Deleting-State
  • Fix flapping state bug
    • In declarative/ 2/reconciler.go in the syncResoruces function it always sets the status of the manifest CR to Deleting if the DeletionTimestamp is unequal to zero, this needs to be changed to avoid the status flapping between Deleting and Warning
    • Could look like the following
                if obj.GetDeletionTimestamp().IsZero() {
			obj.SetStatus(status.WithState(shared.StateProcessing).WithOperation(ErrWarningResourceSyncStateDiff.Error()))
		} else if status.State != shared.StateWarning {
			obj.SetStatus(status.WithState(shared.StateDeleting).WithOperation("manifest should be deleted"))
		}
  • Adapt E2E Test in KLM
    • In warning_status_propagation_test.go:
      • Change "When Kyma Module is disabled with existing finalizer; Then there is no Module in KCP Kyma CR spec; And KCP Kyma CR is in "Warning" State; And Module Manifest CR is in a "Warning" State" to check CONSISTENTLY if Kyma and Manifest CR are in Warning state (First check with eventually condition and then with consistently; since this test should have been covered this scenaroi from the bug, but since it it flapping between the states the Eventually results to true
      • Maybe think of introducing a new util function to cover situations lwhere a condition first needs to be met (eventually) and then if it mets it should be consistent
      • I.e.
By("And Module Manifest CR is consistently in a \"Warning\" State")
			Eventually(CheckManifestIsInState).
				WithContext(ctx).
				WithArguments(kyma.GetName(), kyma.GetNamespace(), module.Name, controlPlaneClient,
					shared.StateWarning).
				Should(Succeed())
			Consistently(CheckManifestIsInState).
				WithContext(ctx).
				WithArguments(kyma.GetName(), kyma.GetNamespace(), module.Name, controlPlaneClient,
					shared.StateWarning).
				Should(Succeed())

@jeremyharisch
Copy link
Contributor

Template-Operator Changes will be done in kyma-project/template-operator#147
Unitl then this issue is blcoked.

@nesmabadr nesmabadr self-assigned this Feb 8, 2024
@jeremyharisch jeremyharisch removed their assignment Feb 8, 2024
kyma-bot pushed a commit to kyma-project/template-operator that referenced this issue Feb 12, 2024
<!-- Thank you for your contribution. Before you submit the pull
request:
1. Follow contributing guidelines, templates, the recommended Git
workflow, and any related documentation.
2. Read and submit the required Contributor Licence Agreements
(https://github.com/kyma-project/community/blob/main/CONTRIBUTING.md#agreements-and-licenses).
3. Test your changes and attach their results to the pull request.
4. Update the relevant documentation.

If the pull request requires a decision, follow the [decision-making
process](https://github.com/kyma-project/community/blob/main/governance.md)
and replace the PR template with the [decision record
template](https://github.com/kyma-project/community/blob/main/.github/ISSUE_TEMPLATE/decision-record.md).
-->

**Description**

Changes proposed in this pull request:

- Only add finalizer if the resource is not under deletion

**Related issue(s)**
kyma-project/lifecycle-manager#1080
@nesmabadr nesmabadr removed their assignment Feb 12, 2024
@c-pius c-pius self-assigned this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants