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

Sidecar manager #37

Merged
merged 69 commits into from
Dec 16, 2022
Merged

Sidecar manager #37

merged 69 commits into from
Dec 16, 2022

Conversation

werdes72
Copy link
Contributor

@werdes72 werdes72 commented Dec 2, 2022

See: #26

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 2, 2022
@kyma-bot
Copy link
Contributor

kyma-bot commented Dec 2, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@CLAassistant
Copy link

CLAassistant commented Dec 2, 2022

CLA assistant check
All committers have signed the CLA.

@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 2, 2022
@kyma-project kyma-project deleted a comment from CLAassistant Dec 5, 2022
@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2022
@kyma-bot kyma-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2022
@barchw barchw mentioned this pull request Dec 7, 2022
5 tasks
@barchw barchw linked an issue Dec 7, 2022 that may be closed by this pull request
5 tasks
@barchw
Copy link
Contributor

barchw commented Dec 14, 2022

/test all

// We want to avoid performing the same action multiple times for a parent if it contains multiple pods that need to be restarted.
if _, exists := processedActionObjects[action.object.getKey()]; !exists {
currentWarnings, err := action.run(ctx, c, action.object, logger)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

With the current implementation we fail fast the restarting of all pods once there is an error for any pod.
Do we want to be more resilient? I think we also didn't do it in the implementation in reconciler.
We can log the error and create a restart warning for the pod here or we could do it in each run function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added logging the error without breaking the loop.

@barchw
Copy link
Contributor

barchw commented Dec 15, 2022

/test all

@barchw barchw marked this pull request as ready for review December 15, 2022 13:38
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2022
@kyma-bot
Copy link
Contributor

kyma-bot commented Dec 15, 2022

@werdes72: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-istio-lint dbc1582 link false /test pull-istio-lint
barchw_test_of_prowjob_pull-istio-sidecars-component-test dbc1582 link false /test pull-istio-sidecars-component-test
barchw_test_of_prowjob_pull-istio-sidecars-lint dbc1582 link false /test pull-istio-sidecars-lint

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@barchw barchw left a comment

Choose a reason for hiding this comment

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

/bark

@kyma-bot
Copy link
Contributor

@barchw: dog image

In response to this:

/bark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Dec 16, 2022
@barchw
Copy link
Contributor

barchw commented Dec 16, 2022

/hold

@kyma-bot kyma-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@barchw
Copy link
Contributor

barchw commented Dec 16, 2022

@werdes72 @cnvergence Please approve and unhold if you don't need anything more to do

@werdes72 werdes72 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@kyma-bot kyma-bot merged commit 99f9b88 into kyma-project:main Dec 16, 2022
@werdes72 werdes72 deleted the sidecar-manager branch August 5, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library managing sidecars
6 participants