Skip to content

Commit

Permalink
update with option Override by GVK
Browse files Browse the repository at this point in the history 
Signed-off-by: haoqing0110 <[email protected]>
  • Loading branch information
@haoqing0110
haoqing0110 committed Jul 25, 2024
1 parent c11469c commit 92e2169
Showing 1 changed file with 89 additions and 74 deletions.
163 changes: 89 additions & 74 deletions enhancements/sig-architecture/123-addon-multiple-configs/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,40 @@ This proposal proposes an enhancement to the addon configurations, enabling user

## Motivation

Users have three locations to set [addon configurations](https://open-cluster-management.io/concepts/addon/#add-on-configurations):
Users have three locations to set [addon configurations](https://open-cluster-management.io/concepts/addon/#add-on-configurations) today:
1. In `ClusterManagementAddon` `spec.supportedConfigs`, user can set the `defaultConfig` where all the addons to have same configurations.
2. In `ClusterManagementAddOn`, `spec.installStrategy.placements[].configs`, user can set configs for addons on a group of clusters selected by placement.
3. In `ManagedClusterAddOn` `spec.configs`, user can set configs for addon on a specific cluster.

Currently, configs with the same GVK only support a single config. The config with the same GVK in place `3` will override the config in place `2`,
which will override the config in place `1`. The final effective addon configs are listed in the mca.status.
which will override the config in place `1`. The final effective addon configs are listed in the `mca.status`. Using `AddonDeploymentConfig` as an example,
user can define one in place `1` as the default config. User can also define one in place `2` for a group of clusters, which will
override the default config. Additionally, user can define one in place `3` for a specific cluster, which will override the `1` and `2`.

Now we are seeing the requirement to set multiple configs for the same GVK. For example, for `OpenTelemetryCollector`, the user may want to define 2 `OpenTelemetryCollector`
in place `2`, one CR is configured to collect traces and another to collect logs on the same cluster. This proposal aims to satisfy that requirement.

Now we are seeing the requirement to set multiple configs for the same GVK. For example, for `OpenTelemetryCollector`, one CR might be configured to
collect traces and another to collect logs on the same cluster. This proposal aims to satisfy that requirement.

### Goals

- Enhance `ClusterManagementAddon` and `ManagementClusterAddon` API to allow users to configure multiple addon configs with the same GVK.

### Non-Goals

- `ClusterManagementAddon` `spec.supportedConfigs` does not allow user to configure mutiple addon configs with same GVK in the `defaultConfig`.
- `ClusterManagementAddon` `spec.supportedConfigs` does not allow the user to configure multiple addon configs with the same GVK in the `defaultConfig`.

## Proposal

### User Stories

1. The admin can configure the `ClusterManagementAddon` API to set mutilple configs with same gvk per install strategy.
2. The admin can configure the `ManagementClusterAddon` API to set mutilple configs with same gvk for specific cluster.
1. The admin can configure the `ClusterManagementAddon` API to set multiple configs with the same GVK per install strategy.
2. The admin can configure the `ManagementClusterAddon` API to set multiple configs with the same GVK for a specific cluster.

## Design Details

### API changes

`cma.spec.installStrategy.placements[].configs` and `mca.spec.configs` already support mutiple configs.
No API changes are needed, `cma.spec.installStrategy.placements[].configs` and `mca.spec.configs` already support multiple configs.

```go
// Configs is the configuration of managedClusterAddon during installation.
Expand All @@ -57,14 +60,16 @@ collect traces and another to collect logs on the same cluster. This proposal ai

As the motivation mentioned, today there are 3 places to set addon configurations.

Currently, since each GVK supports only one config, the GVK is used as the identifier for configs. When configs with same GVK configured in multiple places, it follows the override sequence and
only one config is listed as the effective config in the mca.status.
Currently, since each GVK supports only one config, the GVK is used as the identifier for configs. When configs with the same GVK configured in
multiple places, it follows the override sequence and only one config is listed as the effective config in the `mca.status`.

With the support for multiple configs with the same GVK, `GVK + namespace + name` becomes the identifier.
We need to reconsider the logic for setting mca.status when configuring multiple configs in multiple places.
We need to reconsider the logic for setting `mca.status` when configuring multiple configs in multiple places.

In the initial discussion we listed three options, finally choose the below "Override by GVK" option, as it satisfies more on
the real use case than the other two. The other two are listed in the **Alternatives** for future needs.

We present three options using the example below (omitting the group and namespace for simplicity).
We do not expect to support all but want to start a discussion and eventually reach a consensus on which option should be adopted by the community.
The below example omits the group and namespace for simplicity.

```yaml
apiVersion: addon.open-cluster-management.io/v1alpha1
Expand Down Expand Up @@ -101,14 +106,13 @@ spec:
name: n3
```
#### Merge
With `GVK + namespace + name` as the identifier for each config, there is no override concept now actually, all the configured configs will be listed in the mca.status.
#### Override by GVK
Configs will be ordered, with the last configured on top. For example, configs in mca will be on top of cma configs, and mca configs with higher indices
will be on top of those with lower indices.
"Override by GVK" is a way try to keep consistent with current behavior. GVK is used as the identifier for a group of configs.
The addon's own controller will determine how to use the configs (e.g., choose one, read all, etc).
In the above example, config `r2` `n3` in `ManagedClusterAddOn` (place `3`) will override `r2` `n1` and `n2` in `installStrategy` (place `2`).
And for default config `r1` `n1`, since no other `r1` resource defined in place `2` and `3`, it will be kept in the effective addon configs.
The `mca.status` will be like:

```yaml
apiVersion: addon.open-cluster-management.io/v1alpha1
Expand All @@ -124,29 +128,70 @@ status:
specHash: <spec-hash>
name: n3
resource: r2
- desiredConfig:
name: n2
specHash: <spec-hash>
name: n2
resource: r2
- desiredConfig:
name: n1
specHash: <spec-hash>
name: n1
resource: r2
- desiredConfig:
name: n1
specHash: <spec-hash>
name: n1
resource: r1
```

#### Override by GVK
The limitation is if the `r2n1` and `r2n2` resources are some global configs, the user may not want it to be overridden by `r2n3`. In this case, the user need to define
both `r2n1`,`r2n2` and `r2n3` in `ManagedClusterAddOn`.

### Effective config values

`WithGetValuesFuncs()` depends on how the funcs handle multiple configs, for example:
[GetAddOnDeploymentConfigValues()](https://github.com/open-cluster-management-io/addon-framework/blob/main/pkg/addonfactory/addondeploymentconfig.go#L144) will only read the last object list in `mca.status`.

```go
// GetAddOnDeploymentConfigValues uses AddOnDeploymentConfigGetter to get the AddOnDeploymentConfig object, then
// uses AddOnDeploymentConfigToValuesFunc to transform the AddOnDeploymentConfig object to Values object
// If there are multiple AddOnDeploymentConfig objects in the AddOn ConfigReferences, the big index object will
// override the one from small index
func GetAddOnDeploymentConfigValues(
getter utils.AddOnDeploymentConfigGetter, toValuesFuncs ...AddOnDeploymentConfigToValuesFunc) GetValuesFunc {
...
}
```

For other addons, could write their own `WithGetValuesFuncs()` to determine how to use the listed configs in `mca.status`.

### How to trigger rollout

Below is the current rollout logic and step 2 needs to be enhanced since one GVK will have multiple configs.

1. The [buildConfigurationGraph](https://github.com/open-cluster-management-io/ocm/blob/release-0.13/pkg/addon/controllers/addonconfiguration/controller.go#L166)
will build and list the desired configs for each mca.
2. The [setRolloutStatus](https://github.com/open-cluster-management-io/ocm/blob/release-0.13/pkg/addon/controllers/addonconfiguration/graph.go#L54)
will compare the actual configs in `mca.Status.ConfigReferences` with desired configs, using GVK as key, compare the actual and desired name+namespace+hash, if not match, set status to ToApply.
3. The [generateRolloutResult](https://github.com/open-cluster-management-io/ocm/blob/release-0.13/pkg/addon/controllers/addonconfiguration/controller.go#L132)
will get clusters that need to rollout (with ToApply status) based on the rollout strategy.

### Test Plan

- e2e test on setting configs in `ClusterManagementAddon` `spec.supportedConfigs`.
- e2e test on setting multiple configs in `ClusterManagementAddOn` `spec.installStrategy.placements[].configs`.
- e2e test on setting multiple configs in `ManagedClusterAddOn` `spec.configs`.

### Graduation Criteria

TBD

### Upgrade / Downgrade Strategy

TBD

## Alternatives

"Override by GVK" is a way try to keep consistent with current behavior.
### Effective addon configs
#### Merge

The gaps is if the r2n1 and r2n2 resource are actually some global configs, you may not want it to be overriden by r2n3. Compared with "Merge" options,
this options can not cover such case and is lack of flexibility.
With `GVK + namespace + name` as the identifier for each config, there is no overriding concept now actually, all the configured configs will be listed in the `mca.status`.

Configs will be ordered, with the last configured on top. For example, configs in mca will be on top of cma configs, and mca configs with higher indices
will be on top of those with lower indices.

The addon's own controller will determine how to use the configs (e.g., choose one, read all, etc).

```yaml
apiVersion: addon.open-cluster-management.io/v1alpha1
Expand All @@ -162,6 +207,16 @@ status:
specHash: <spec-hash>
name: n3
resource: r2
- desiredConfig:
name: n2
specHash: <spec-hash>
name: n2
resource: r2
- desiredConfig:
name: n1
specHash: <spec-hash>
name: n1
resource: r2
- desiredConfig:
name: n1
specHash: <spec-hash>
Expand All @@ -187,44 +242,4 @@ status:
specHash: <spec-hash>
name: n3
resource: r2
```

### Effective config values

`WithGetValuesFuncs()` depends on how the funcs handle multiple configs, for example:
[GetAddOnDeploymentConfigValues()](https://github.com/open-cluster-management-io/addon-framework/blob/main/pkg/addonfactory/addondeploymentconfig.go#L144) will only read the last object list in mca.status.

```go
// GetAddOnDeploymentConfigValues uses AddOnDeploymentConfigGetter to get the AddOnDeploymentConfig object, then
// uses AddOnDeploymentConfigToValuesFunc to transform the AddOnDeploymentConfig object to Values object
// If there are multiple AddOnDeploymentConfig objects in the AddOn ConfigReferences, the big index object will
// override the one from small index
func GetAddOnDeploymentConfigValues(
getter utils.AddOnDeploymentConfigGetter, toValuesFuncs ...AddOnDeploymentConfigToValuesFunc) GetValuesFunc {
...
}
```

For other addons, could write their own `WithGetValuesFuncs()` to determine how to use the listed configs in mca.status.

### How to trigger rollout

TBD

### Test Plan

- e2e test on setting configs in `ClusterManagementAddon` `spec.supportedConfigs`.
- e2e test on setting multiple configs in `ClusterManagementAddOn` `spec.installStrategy.placements[].configs`.
- e2e test on setting multiple configs in `ManagedClusterAddOn` `spec.configs`.

### Graduation Criteria

TBD

### Upgrade / Downgrade Strategy

TBD

## Alternatives

N/A
```

0 comments on commit 92e2169

Please sign in to comment.