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

Use explicit ModuleTemplate .spec.manager info to determine the module manager #1831

Closed
6 tasks
c-pius opened this issue Sep 5, 2024 · 3 comments
Closed
6 tasks
Labels
area/quality Related to all activites around quality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@c-pius
Copy link
Contributor

c-pius commented Sep 5, 2024

Description

We determine the manifest state based on the state of its manager here: https://github.com/kyma-project/lifecycle-manager/blob/main/internal/manifest/statecheck/state_check.go

Right now, we determine the manager implicitly, i.e., the first deployment or stateful set that we find as part of the module resources. With #1840 we will introduce explicit information what the manager of the module is. We should use this to refactor this implicit determination of the manager.

Reasons

More explicit code, less prone to errors (not only manager deployment in raw manifest)

Acceptance Criteria

  • manager explicitly looked up by the GVK info from the ModuleTemplate
    • if not present in the manifest => throw an error (may already be validated long before the state check)
    • if not found in the SKR
      • within 1 min of creation of the ManifestCR => return state.Processing
      • after 1 min of creation of the ManifestCR => return state.Error
  • create e2e tests covering all three cases above

Feature Testing

No response

Testing approach

No response

Attachments

No response

@c-pius c-pius added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 5, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Sep 5, 2024

Blocked right now until ADR is completed and API change is implemented.

@c-pius c-pius added the area/quality Related to all activites around quality label Sep 5, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Sep 11, 2024

When rolling out the current state check without the explicit manager, we experienced problems with Cloud Manager. The key assumption that every module has a manager deployed to the cluster doesn't hold, as Cloud Manager is a KCP component.

For Cloud Manager, we only deploy the CRD to the SKR. If this fails, we already error out before determining the manager state. We could allow the CRD to be specified as manager (field name is then questionable, but I would say it is okay given that this should be an exceptional case). With that, we would have 3 supported state checks from KLM perspective:

type Manager struct {
	group,
        version,
        kind,
        namespace,
        name
}

type ManagerStateCheck struct {
       // holds 
       // "deployment"=>DeploymentStateChecker: checks based on deployment resources
       // "statefulset"=>StatefulSetStateChecker: checks based on statefulset resources
       // "crd"=>CRDStateChecker: checks based on the CRD conditions (should have Established, NamesAccepted and NOT Terminating)
       checkers map[string]StateChecker
}

func (m *ManagerStateCheck) GetState(ctx context.Context, mngr Manager, resources []*resource.Info) (shared.State, error) {
  if checker, hit := m.checkers[mngr.kind]; !hit {
    return shared.StateError, ErrNoCheckForGivenKind
  }
  return checker.GetState(ctx, mngr, resources)
}

For Kyma dashboard / CLI not much changes. They take the manager info and check if the resource is there in the SKR. If so, it is assumed that the module is installed.

@c-pius c-pius added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Sep 11, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Dec 5, 2024

Closing this issue as we are not aiming to use this info in lifecycle-manager. We should rather have a follow-up where it is clarified what states the end user needs exactly, and who is responsible for providing what, e.g.:

  • Manifest.Status => only tells if KLM was able to apply the manifest
  • DefaultCR.Status => tells if the module is functional
  • Manager.Status => tells if the module's manager is up and running, affects the DefaultCR.Status

@c-pius c-pius closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/quality Related to all activites around quality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

No branches or pull requests

1 participant