-
Notifications
You must be signed in to change notification settings - Fork 872
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
Feat: component replication #4449
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4449 +/- ##
==========================================
+ Coverage 61.58% 61.74% +0.15%
==========================================
Files 348 352 +4
Lines 34425 34773 +348
==========================================
+ Hits 21201 21470 +269
- Misses 10467 10501 +34
- Partials 2757 2802 +45
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Add e2e test Signed-off-by: qiaozp <qiaozhongpei.qzp@alibaba-inc.com>
pkg/oam/util/helper.go
Outdated
if replicaKey == "" { | ||
return componentName | ||
} | ||
return fmt.Sprintf("%s-%s", componentName, replicaKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if a user define workload name in cue template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be verified if the old style will overwrite the workload name as well.
Signed-off-by: Qiaozp <qiaozhongpei.qzp@alibaba-inc.com>
Signed-off-by: Qiaozp <qiaozhongpei.qzp@alibaba-inc.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job! LGTM
type ReplicationPolicySpec struct { | ||
Keys []string `json:"keys,omitempty"` | ||
// Selector is the subset of selected components which will be replicated. | ||
Selector []string `json:"selector,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the selector can only specify components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes. It works similar to the selector
field in override policy. We can make further extension in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what else? Trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe other component selection ways, like selecting components through regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a string, it lacks flexible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it lacks flexibility. The consideration here is:
- It aligns with the design of
override
policy, althoughoverride
policy also lacks flexibility, but it is easy to understand. - It satisfy the scenario we currently encountered by far.
- We could have more extension ways in the future. For now although it would be nice to make it more extensible, I do not have too much good points due to the lack of more advanced feature requests.
FYI, @chivalryq , a CUE file might need to be added to provide OpenAPI Schema for VelaUX and auto-generate documentations for kubevela.io. We can do it in another PR. |
@@ -82,6 +84,10 @@ func (executor *deployWorkflowStepExecutor) Deploy(ctx context.Context, policyNa | |||
if err != nil { | |||
return false, "", err | |||
} | |||
components, err = pkgpolicy.ReplicateComponents(policies, components) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will happen if the replicaComponent don't use the replicaKey in the definition template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If the definition template relies on replicaKey field, an error will be thrown for the incorrect use of that component/trait.
- If the definition template use the replicaKey field and has a fallback logic for handling the lack of replicaKey, it will be fine to use the definition with or without replicaKey.
- If the definition template does not use the replicaKey, nothing will be happened while using the replicaKey. (The component will be repeated dispatched but there is no side effect.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it depends on definitions. In definition it can check if context.replicaKey
is nil. Just like I did in replica-webservice.yaml
. But it's not recommended to mix component designed for replication and common ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if one component use the replica key in the definition, but it's used in a fixed workload name? It will apply many times with the last config? What about next time? Does it cause any infinitely loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a problem. We can optimize it later. Like scanning the definition used when replicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will apply many times but will not cause infinite loop. Replication does not mutate configurations. Multiple apply will be the same.
Got it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also add definition cue file with example markdown file (and some necessary explanation of the usage.)
It's really tricky when use it in definition.
ping @wonderflow definition cue and example md added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job
Description of your change
Fixes #4326
Added a replication policy. When using
deploy
workflow step, it could replicate components with keys. e.g.Apply Order of Policies
Now,
override
,topology
andreplication
can work withdeploy
workflow step. If they are used together, the apply order is:Replication Spec
replication
policy can NOT be used individually. Like topology, it need override to select target components to deploy.The
selector
field of replication policy is used to choose which components to replicate. For example, you can specify three components with override policy and only replicate one of them. "comp2" and "comp3" will be dispatched without any replicationSpecial Definition
Given that there aren't too many replication use case now. One want to use replication policy should modify the Component or Trait definition. A new context key
replicaKey
is provided when render the components. Here is an replica-webservice example intest/e2e-test/testdata/definition/replica-webservice.yaml
Fixes #
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Special notes for your reviewer