iter8-tools / iter8

Kubernetes release optimizer
https://iter8.tools
Apache License 2.0
254 stars 34 forks source link

Remove and auto compute testing pattern #863

Closed sriumcp closed 3 years ago

sriumcp commented 3 years ago

Is your feature request related to a problem? Please describe the problem. Testing pattern (conformance, canary, A/B, A/B/n, ...) is explicitly specified in the experiment CR (required at present), and also validated based on num versions and criteria. This is needlessly complex for the user.

Describe the feature/solution you'd like

  1. Remove testingPattern from spec.strategy
  2. Auto compute testing pattern based on num versions and criteria and populate status.strategy.testingPattern with this instead.
  3. Collapse the types conformance and canary into a single type, and call it SLOValidation.

Will this feature/solution bring new benefits for Iter8 users? What are they? Simplifies experimentation experience both conceptually and in terms of define the experiment.

Does this issue require a design doc/discussion? If there is a link to the design document/discussions, please provide it below. This will be part of v2alpha3 CRD change (which includes #862). Yes, another, "implementation-level" design discussion is needed for this feature as it is a CRD change that will affect every Iter8 component.

How will this feature be tested?

How will this feature be documented?

kalantar commented 3 years ago

Suggest making this optional instead of removing it. Users do think in concepts such as "canary" "validation" "a/b". It doesn't hurt to allow them to express what they think they are asking for and validating if the rest does not match. Agree with computation otherwise and that it should go to status where it can be shown to the user via get.

kalantar commented 3 years ago

Wonder about rename of canary. Users use this term. We want to be sure that the terms we have match to user concepts. Granted. This term is used differently by different users so the change may be appropriate.

sriumcp commented 3 years ago

Top links that show up on google when I typed "canary testing"...

  1. https://launchdarkly.com/blog/what-is-a-canary-deployment/?utm_source=google&utm_medium=cpc&utm_campaign=&utm_terms=&matchtype=b&utm_content=529046860681&gclid=CjwKCAjwruSHBhAtEiwA_qCppncKGBqfztTFIkrqBdYwLihli1M9gMDVnk9zpz20sjJgdnugNnxX7hoC_DIQAvD_BwE
  2. https://www.optimizely.com/optimization-glossary/canary-testing/
  3. https://featureflags.io/canary-testing/
  4. https://whatis.techtarget.com/definition/canary-canary-testing

One thing all of them seem to agree on is that canary is about releasing software to a small group of users.

sriumcp commented 3 years ago

So... how about?

  1. Make testingPattern optional in spec and in status. Behavior ...

    • If user supplied the correct info, Iter8 controller will validate and then copy it over to status (assuming experiment is valid).
    • If user supplied incorrect info, Iter8 controller will validate, experiment will be found to be invalid, and there will be failure.
    • If user omitted this field, Iter8 controller will compute this field based on criteria and number of versions and copy the computed value to status (no validation since there's nothing in spec).
  2. Definitions...

    • Criteria has no reward: testingPattern = SLO validation.
    • Criteria has reward and no objectives: number of versions = 2. Then, testingPattern = A/B
    • Criteria has reward and no objectives: number of versions > 2. Then, testingPattern = A/B/n
    • Criteria has reward and objectives: number of versions = 2. Then, testingPattern = Hybrid-A/B
    • Criteria has reward and objectives: number of versions > 2. Then, testingPattern = Hybrid-A/B/n
sriumcp commented 3 years ago

@kalantar Given the simplification streak we're on, my preference would be that we leave out the testingPattern entirely from the spec section from now. The amount of stuff we need to explain, when we try to explain the Iter8 experiment CRD -- is enormous. Dropping the field would simply make the number of fields to be explained, one less.

Given that the idea is anyway to make the field optional, we will always have the option of reintroducing this field as optional, if we really feel the need for it.

Note that the field is still going to be present -- in status.

kalantar commented 3 years ago

I don't suppose there is much value in keeping it.

sriumcp commented 3 years ago

Testing pattern in status is still valid (because it is based on criteria). However, removal is now superseded by #941

sriumcp commented 3 years ago

Note: Please see #942

kalantar commented 3 years ago

Some questions:

sriumcp commented 3 years ago

Did we decide to add patterns Hybrid-A/B and Hybrid-A/B/n

Yes, Hybrid-A/B (reward, objectives, 2 versions) and Hybrid-A/B/n (reward, objectives, 2 or more versions) are patterns we will now support, in addition to SLO validation (no reward), A/B (reward + 2 versions) and A/B/n (reward + 3 or more versions).

According to rules above, if 3 versions and no reward, it is SLOValidation. This is not restricted to 1 or 2 versions?

Yes. I don't think the analytics service should declare any winner in this case though. In any case, testing pattern should be SLOValidation in this case.

Does SLOValidation require objectives?

Practically yes. Technically no. If there are no objectives and no rewards, we should still call it SLO validation (just happens to be the degenerate case with zero SLOs). So, all versions will pass the criteria.