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

Product-Config 2.0 #362

Open
nightkr opened this issue Apr 6, 2022 · 0 comments
Open

Product-Config 2.0 #362

nightkr opened this issue Apr 6, 2022 · 0 comments

Comments

@nightkr
Copy link
Member

nightkr commented Apr 6, 2022

The current product-config architecture has a few issues that we keep bumping into:

  • It's confusing to understand what values come from where
  • Validation error messages are produced in the context of the product's configuration format, which is confusing to users who configure it in term of our ProductCluster abstraction
  • PC only works on unstructured key/value maps that correspond directly to the product's configuration structure, making it difficult to feed config back from PC to the operator itself (for example: port values need to match between the configuration's listen option and the generated discovery configmaps)
    • This often leads us to bypass P-C for these options
    • We have no solid way to get back typed Rust values when using the Configuration mechanism to allow fields to be overridden across the ProductCluster/Role/RoleGroup hierarchy (without implementing the same precedence mechanism in the operator itself)
  • There is a lot of repetitive boilerplate when linking Rust structs up to the PC machinery

To work this out I propose reorient PC so that the Rust structs are the source of truth for the structure, and so that precedence rules and validations are applied while still working with typed Rust values. We would still need to serialize to the product's native configuration format (and apply overrides to that) eventually, but this would be delayed until it is required.

From the definition side, the API is currently expected to look something like this:

#[derive(ProductConfig)]
struct ZookeeperConfig {
    #[pc(default = 1000)]
    #[pc(file("hdfs-site.xml", "tick.limit.ms"))]
    tick_limit_ms: i32,
}

This would expand to something like the following:

struct ZookeeperConfigFragment {
    tick_limit_ms: Option<i32>,
}

enum ZookeeperConfigValidationError {
    NoTickLimitMs,
}

impl ProductConfigFragment for ZookeeperConfigFragment {
    type Validated = ZookeeperConfig;
    type ValidationError = ZookeeperConfigValidationError;

    fn merge(self, other: &Self) -> Self {
        Self {
            tick_limit_ms: self.tick_limit_ms.or_else(other.tick_limit_ms),
            ...
        }
    }

    fn default() -> Self {
        Self {
            tick_limit_ms: Some(1000),
        }
    }

    fn validate(self) -> Result<ZookeeperConfig, ZookeeperConfigValidationError> {
        match self {
            ZookeeperConfigFragment { tick_limit_ms: Some(tick_limit_ms) } => Ok(ZookeeperConfig {tick_limit_ms}),
            ZookeeperConfigFragment { tick_limit_ms: None, .. } => Err(ZookeeperConfigValidationError::NoTickLimitMs),
        }
    }
}

This would allow the reconciler to process already fully merged and validated ZookeeperConfig objects, while still preserving the flexibility of the override hierarchy.

Product-config YAMLs would be relegated to serving as progressive enhancements and overrides to the metadata. For example, this could be used to improve backwards compatibility with older product versions, or to supply more thorough documentation. The current proposal for these looks like this, but this part is still fairly up in the air:

mixins:
- appliesTo:
    product:
    - kafka
    versionRange:
    - product: ">=1.0.0"
    properties:
    - tick_limit_ms
  apply:
    description: |
      The maximum allowed clock skew between cluster members
    default: foo

This is a part of stackabletech/issues#198.

bors bot pushed a commit that referenced this issue May 5, 2022
## Description

This is a starting point for #362, but is intended to be usable outside of that context (in this case, the primary motivation for getting this done now is #365).



Co-authored-by: Teo Klestrup Röijezon <[email protected]>
Co-authored-by: Felix Hennig <[email protected]>
Co-authored-by: Malte Sander <[email protected]>
Co-authored-by: Sebastian Bernauer <[email protected]>
Co-authored-by: Sönke Liebau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant