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

Unified selector #15

Merged
merged 4 commits into from
Jun 9, 2021
Merged

Unified selector #15

merged 4 commits into from
Jun 9, 2021

Conversation

YangKeao
Copy link
Member

It's the very first step of chaos-mesh/chaos-mesh#1520

Please comment if you have better idea :)

@STRRL @Hexilee

Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: Yang Keao <keao.yang@yahoo.com>
Signed-off-by: Yang Keao <keao.yang@yahoo.com>
The controller would construct the `Selector` first, and then use the `Selector`
to select the chaos targets. However, as the target may have multiple type (it
could be a pod, a container, a volume or an AWS machine), we can only return an
`interface{}`, and the implementation of chaos would assert the type by
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the controller does not actually know the type of the target? I wonder how it leads to

the controller framework should know the concrete status of every selected targets

Maybe not a interface{}, but some interface Target{}?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. It needs to know whether these targets are injected or not (or fault), which can be judged according to the return value of Apply/Recover. (If Recover success, then it must have been in "not injected" status 😸 )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the granularity of success/failure would be at a per-chaos level. What if some of the targets succeeded but the other failed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop. In controller, the "success"/"failure" will be per-"target" level, so that the error handling (like "retry") would be simpler.

However, to avoid modifying dashboard, I don't want to modify the specification too rudely. So the per-chaos level will be keeped as the "AND" of every target status, and will only be used to show in dashboard.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get it. How can you get a pre-target status if you do not even know what the target is? (Since it is interface{})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "status" is only about whether this chaos has been executed successfully, which we can get from the following return value of Apply or Recover function. Why should I know what the target is?

But yes. I have realized that interface{} is not enough. It has to provide an ID to record the status/running result.

Signed-off-by: Yang Keao <keao.yang@yahoo.com>
Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cwen0 cwen0 merged commit 1b7e90c into chaos-mesh:main Jun 9, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants