-
Notifications
You must be signed in to change notification settings - Fork 804
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: inject IOChaos with multiple containers #4332
base: master
Are you sure you want to change the base?
feat: inject IOChaos with multiple containers #4332
Conversation
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4332 +/- ##
==========================================
+ Coverage 38.68% 38.70% +0.01%
==========================================
Files 169 169
Lines 13950 13965 +15
==========================================
+ Hits 5397 5405 +8
- Misses 8105 8113 +8
+ Partials 448 447 -1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
/hold The e2e tests failed. I need to investigate the reason. |
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
07b4e16
to
0027797
Compare
I have several points about the current design:
|
The first container. This behavior is the same as the other type of chaos. The default container selector will select the first container if
@STRRL Yes. I actually thought for hours to choose which way to implement but I finally chose the second way. My considerations are based on the following points:
|
e2e tests keep failing because this assertion does not get acquired: chaos-mesh/e2e-test/e2e/chaos/iochaos/io_delay.go Lines 229 to 248 in 373785d
other test cases does not check this, so they could get passed, which is not expected. I am still profiling the reason why the status of IOChaos is not updated to With the debugger, I noticed these lines of code does not get called by reconcile, I guess that it does not get the latest status of PodIOChaos, or one iteration of reconciliation does not get triggered properly. chaos-mesh/controllers/chaosimpl/iochaos/impl.go Lines 87 to 89 in 4a85452
|
@STRRL 👍🏼 Good catch! To correct this behavior, should I add a condition to check if namespacedName, containerName, err := controller.ParseNamespacedNameContainer(record.Id)
// ...
namespacedNameContainer := k8sTypes.NamespacedName{
Namespace: namespacedName.Namespace,
Name: namespacedName.Name + "-" + containerName,
}
if namespacedName == objName || namespacedNameContainer == objName {
//...
} |
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
After discussing with @STRRL, we decided to abandon this implementation (an Currently, a record only matches a Pod, for example, if you inject a So the next step, we will do:
For anyone who wants to inject IOChaos with multiple containers now, please compile the images based on this PR. The current code passes all the necessary tests, and it's working in my local test environment. |
Hi,
Thanks and br |
What problem does this PR solve?
For a long time, we didn't support injecting IOChaos with multiple containers even if we have implemented new unified selectors with the
containerNames
field.chaos-mesh/api/v1alpha1/podiochaos_types.go
Line 28 in b4dcfa3
What's changed and how it works?
To support injecting IOChaos with multiple containers, I followed below steps:
curlpod
, and it has two containers namedcurl1
andcurl2
, the generated podiochaos will become:curlpod-curl1
curlpod-curl2
Key
toPodKey
, and added aChaosKey
to find related podiochaos.Related changes
UI interface
Cherry-pick to release branches (optional)
Checklist
CHANGELOG
CHANGELOG.md
Tests
Side effects
DCO
If you find the DCO check fails, please run commands like below (Depends on the actual situations. For example, if the failed commit isn't the most recent) to fix it: