search

canonical / charmed-kubeflow-chisme

Shared Utilities used across Charmed Kubeflow
Apache License 2.0
1 stars 4 forks source link

Expose optional `force` parameter in `apply()` in kubernetes resource handler #35

Closed i-chvets closed 1 year ago

i-chvets commented 1 year ago

Description

Expose optional force parameter in apply() in kubernetes resource handler.

In some cases force parameter is required in order to apply resources in the cluster.

DnPlas commented 1 year ago

Thanks @i-chvets, this sounds interesting. Have you explored the --force-conflicts option? It seems a little bit less aggressive than --force. Looking at the docs:

It sounds to me likeforce may cause issues if the resource gets deleted suddenly to be then re-applied, whereas force-conflicts will try to just force the changes against conflicts. Either option must be used carefully, though, I think we should only use force* iff the conflict is something we are sure won't represent any trouble with existing CRs. For instance, I'd only use it in an upgrade event handler after doing some checks.

i-chvets commented 1 year ago

Answer in 4 parts:

  1. In server-side apply functionality --force-conflicts only exists in K8S API, not in lightkube. This is the API we are using.
  2. Looks like force=True in lightkube API is the same as using --force-conflicts in K8S API.
  3. Interesting point about doing force=True on upgrade only (discussed below).
  4. This issue requests change in API to expose force parameter. Its usage can be discussed in appropriate issues where we are trying to solve conflicts during upgrade.

Forcing conflicts during upgrade only (really should be discussed in the charm specific issues): Can customer change those CRDs? They are owned by the charm and should not be touched, right? What if customer did some changes to those CRDs? All other events (config-change, add-relation, etc.) will fail on K8S resource apply if force is not used. Upgrade will be the only one that will succeed and will overwrite customer changes.

i-chvets commented 1 year ago

Fix is merged. Closing. Moving discussion of use of force parameter to charm's issues/