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

feat: Separate business status from module status #1628

Merged
merged 33 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8766d15
Remove custom CR state check, only check for deployment state and map…
jeremyharisch Jun 12, 2024
249c173
Remove custom CR state check, only check for deployment state and map…
jeremyharisch Jun 12, 2024
5fbed0c
Remove arg from test func
jeremyharisch Jun 12, 2024
6e2a043
Fix integration test
jeremyharisch Jun 14, 2024
b90f7e3
Fix integration test
jeremyharisch Jun 14, 2024
157b1ab
Merge branch 'main' into newStateLogic
jeremyharisch Jun 14, 2024
d8a49c4
Refactor Status propagation
jeremyharisch Jun 20, 2024
af9df2c
Fix Status decoupling Test
jeremyharisch Jun 21, 2024
0618975
Merge branch 'main' into newStateLogic
jeremyharisch Jun 21, 2024
4cc4981
Fix Status decoupling Test
jeremyharisch Jun 24, 2024
c6b6243
Set deleting state if manifest has a deletion timestamp
jeremyharisch Jun 24, 2024
c607683
Adapt state decoupling test
jeremyharisch Jun 24, 2024
e166263
Merge branch 'main' into newStateLogic
jeremyharisch Jun 25, 2024
fcee820
Add new Testcase
jeremyharisch Jun 26, 2024
4a7a9bb
Use wrong config module for test
jeremyharisch Jun 26, 2024
675261f
Use wrong config module for test
jeremyharisch Jun 26, 2024
3d0ce71
Fix wrong config module for test
jeremyharisch Jun 26, 2024
f18cca2
Merge branch 'main' into newStateLogic
jeremyharisch Jun 27, 2024
05f5fe3
Fix go lint
jeremyharisch Jun 27, 2024
172219c
Add docs
jeremyharisch Jun 27, 2024
39966f2
Merge branch 'main' into newStateLogic
jeremyharisch Jun 27, 2024
ee7ba34
Adress review comments
jeremyharisch Jun 28, 2024
1f08df8
Merge branch 'newStateLogic' of github.com:jeremyharisch/lifecycle-ma…
jeremyharisch Jun 28, 2024
591121c
Gofumpt
jeremyharisch Jun 28, 2024
a699986
Adress second review comments
jeremyharisch Jun 28, 2024
d33dcc6
Introduce more unit tests
jeremyharisch Jun 28, 2024
ab5c26e
Remove duplicated check
jeremyharisch Jun 28, 2024
0c58ac4
Remove duplicated check
jeremyharisch Jun 28, 2024
8cea9de
Merge branch 'main' into newStateLogic
jeremyharisch Jun 28, 2024
b496ecd
Apply suggestions from code review
jeremyharisch Jul 1, 2024
c093a0b
Remove Stateinfo
jeremyharisch Jul 1, 2024
659b4af
Merge branch 'main' into newStateLogic
jeremyharisch Jul 1, 2024
5be43d5
Apply suggestions from code review
jeremyharisch Jul 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion .github/actions/deploy-template-operator/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ runs:
kubectl apply -f tests/moduletemplates/moduletemplate_template_operator_v2_fast.yaml
- name: Create Template Operator Module with final state and final deletion state as `Warning` and apply
working-directory: template-operator
if: ${{ matrix.e2e-test == 'module-status-propagation'}}
if: ${{ matrix.e2e-test == 'module-status-decoupling'}}
shell: bash
run: |
pushd config/default
Expand All @@ -66,6 +66,23 @@ runs:
sed -i 's/localhost:5111/k3d-kcp-registry.localhost:5000/g' ./template.yaml
kubectl get crds
kubectl apply -f template.yaml
- name: Create Template Operator Module with non-working image and apply
working-directory: template-operator
if: ${{ matrix.e2e-test == 'module-status-decoupling'}}
shell: bash
run: |
pushd config/default
echo \
"- op: replace
path: /spec/template/spec/containers/0/image
value: non-working-path" >> image_patch.yaml
cat image_patch.yaml
kustomize edit add patch --path image_patch.yaml --kind Deployment
popd
kyma alpha create module --kubebuilder-project --channel=regular --name kyma.project.io/module/template-operator-misconfigured --version 1.1.1 --path . --registry localhost:5111 --insecure --module-archive-version-overwrite
sed -i 's/localhost:5111/k3d-kcp-registry.localhost:5000/g' ./template.yaml
kubectl get crds
kubectl apply -f template.yaml
- name: Create Template Operator Module without default CR and apply
working-directory: template-operator
if: ${{ matrix.e2e-test == 'module-without-default-cr' }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
- watcher-enqueue
- kyma-deprovision-with-foreground-propagation
- kyma-deprovision-with-background-propagation
- module-status-propagation
- module-status-decoupling
- kyma-metrics
- module-without-default-cr
- module-consistency
Expand Down
9 changes: 8 additions & 1 deletion docs/technical-reference/api/manifest-cr.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,14 @@ The resource is the default data that should be initialized for the module and i

### **.status**

The Manifest CR status is an unmodified version of the [declarative status](../../../internal/declarative/README.md#resource-tracking), so the tracking process of the library applies. There is no custom API for this.
The Manifest CR status is set based on the following logic, managed by the manifest reconciler:

- If the Module defined in the Manifest CR is successfully applied and the deployed Module is up and running, the status of the Manifest CR is set to `Ready`.
jeremyharisch marked this conversation as resolved.
Show resolved Hide resolved
- While the Manifest is being applied and the Deployment is still starting, the status of the Manifest CR is set to `Processing`.
jeremyharisch marked this conversation as resolved.
Show resolved Hide resolved
- If the Deployment is unable to start (for example, due to an `ImagePullBackOff` error) or if the application of the Manifest fails, the status of the Manifest CR is set to `Error`.
jeremyharisch marked this conversation as resolved.
Show resolved Hide resolved
- If the Manifest CR is marked for deletion, the status of the Manifest CR is set to `Deleting`.

This status provides a reliable way to track the state of the Manifest CR and the associated Module, offering insights into the deployment process and any potential issues, while being decoupled of the Module business logic.
jeremyharisch marked this conversation as resolved.
Show resolved Hide resolved

### **.metadata.labels**

Expand Down
2 changes: 1 addition & 1 deletion internal/controller/manifest/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func NewReconciler(mgr manager.Manager,
declarativev2.WithSpecResolver(
manifest.NewSpecResolver(kcp, extractor),
),
declarativev2.WithCustomReadyCheck(manifest.NewCustomResourceReadyCheck()),
declarativev2.WithCustomReadyCheck(manifest.NewDeploymentReadyCheck()),
declarativev2.WithRemoteTargetCluster(lookup.ConfigResolver),
manifest.WithClientCacheKey(),
declarativev2.WithPostRun{manifest.PostRunCreateCR},
Expand Down
3 changes: 1 addition & 2 deletions internal/declarative/v2/ready_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type StateInfo struct {
}

type ReadyCheck interface {
Run(ctx context.Context, clnt Client, obj Object, resources []*resource.Info) (StateInfo, error)
Run(ctx context.Context, clnt Client, resources []*resource.Info) (StateInfo, error)
}

func NewExistsReadyCheck() *ExistsReadyCheck {
Expand All @@ -31,7 +31,6 @@ type ExistsReadyCheck struct{}
func (c *ExistsReadyCheck) Run(
ctx context.Context,
clnt Client,
_ Object,
resources []*resource.Info,
) (StateInfo, error) {
for i := range resources {
Expand Down
49 changes: 35 additions & 14 deletions internal/declarative/v2/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,12 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, obj Object,
}
}

return r.checkTargetReadiness(ctx, clnt, obj, target)
deploymentState, err := r.checkDeploymentState(ctx, clnt, obj, target)
if err != nil {
obj.SetStatus(status.WithState(shared.StateError).WithErr(err))
return err
}
return r.setManifestState(obj, deploymentState)
}

func hasDiff(oldResources []shared.Resource, newResources []shared.Resource) bool {
Expand All @@ -387,31 +392,47 @@ func hasDiff(oldResources []shared.Resource, newResources []shared.Resource) boo
return false
}

func (r *Reconciler) checkTargetReadiness(
ctx context.Context, clnt Client, manifest Object, target []*resource.Info,
) error {
status := manifest.GetStatus()

func (r *Reconciler) checkDeploymentState(
ctx context.Context, clnt Client, obj Object, target []*resource.Info,
) (shared.State, error) {
resourceReadyCheck := r.CustomReadyCheck

crStateInfo, err := resourceReadyCheck.Run(ctx, clnt, manifest, target)
deploymentStateInfo, err := resourceReadyCheck.Run(ctx, clnt, target)
if err != nil {
manifest.SetStatus(status.WithState(shared.StateError).WithErr(err))
return err
return shared.StateError, err
}

if deploymentStateInfo.State == shared.StateProcessing {
return shared.StateProcessing, nil
}

if crStateInfo.State == shared.StateProcessing {
waitingMsg := "waiting for resources to become ready: " + crStateInfo.Info
if !obj.GetDeletionTimestamp().IsZero() {
ruanxin marked this conversation as resolved.
Show resolved Hide resolved
return shared.StateDeleting, nil
}

return deploymentStateInfo.State, nil
}

func (r *Reconciler) setManifestState(manifest Object, state shared.State) error {
status := manifest.GetStatus()

if state == shared.StateProcessing {
waitingMsg := "waiting for resources to become ready"
manifest.SetStatus(status.WithState(shared.StateProcessing).WithOperation(waitingMsg))
return ErrInstallationConditionRequiresUpdate
}

if !manifest.GetDeletionTimestamp().IsZero() {
ruanxin marked this conversation as resolved.
Show resolved Hide resolved
state = shared.StateDeleting
}

installationCondition := newInstallationCondition(manifest)
if !meta.IsStatusConditionTrue(status.Conditions, installationCondition.Type) || status.State != crStateInfo.State {
if !meta.IsStatusConditionTrue(status.Conditions,
installationCondition.Type) || status.State != state {
installationCondition.Status = apimetav1.ConditionTrue
meta.SetStatusCondition(&status.Conditions, installationCondition)
manifest.SetStatus(status.WithState(crStateInfo.State).
WithOperation(generateOperationMessage(installationCondition, crStateInfo)))
manifest.SetStatus(status.WithState(state).
WithOperation(generateOperationMessage(installationCondition, StateInfo{State: state})))
ruanxin marked this conversation as resolved.
Show resolved Hide resolved
return ErrInstallationConditionRequiresUpdate
}

Expand Down
Loading
Loading