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.

Sign up for GitHub

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

Jump to bottom

🐛 ClusterResourceSet: continue applying when apply for a single cluster failed #8611

Merged
merged 1 commit into from
May 9, 2023
Merged

🐛 ClusterResourceSet: continue applying when apply for a single cluster failed #8611

merged 1 commit into from
May 9, 2023

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented May 5, 2023 •
edited
Loading

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:
Previously as soon as apply for a single cluster failed the Reconcile func returned and other clusters were not reconciled.

With this PR we try to apply for all clusters and then requeue if one of them returned an ErrClusterLocked error.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 5, 2023
@sbueringer sbueringer mentioned this pull request May 5, 2023
Merged
@sbueringer
Copy link
Member Author

/assign @fabriziopandini

@sbueringer
Copy link
Member Author

/assign @killianmuldoon

@sbueringer
Copy link
Member Author

/hold
I think we might not need this

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2023
@sbueringer
Copy link
Member Author

Looks like we don't need this fix once this CR fix is merged: kubernetes-sigs/controller-runtime#2303

Basically with the old CR version ConfigMap/Secret were not cached, with the new version they were cached. So we now were hitting stale cache issues with ConfigMap/Secret.

@sbueringer
Copy link
Member Author

/close

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Closed this PR.

In response to this:

/close

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.

@killianmuldoon
Copy link
Contributor

Should we fix this anyway? I think this PR was all upside / improvements in CRS.

@sbueringer
Copy link
Member Author

Do you think we should requeue if the object actually does not exist?

@sbueringer
Copy link
Member Author

/reopen

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Reopened this PR.

In response to this:

/reopen

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.

@k8s-ci-robot k8s-ci-robot reopened this May 5, 2023
@sbueringer
Copy link
Member Author

Do you think we should requeue if the object actually does not exist?

The part that we try all clusters before we return the errors is definitely an improvement we should merge

@sbueringer
Copy link
Member Author

I'll update the PR as discussed.

Independent of that I want to merge this 1-2 weeks after the CR bump to ensure the CR bump by itself is not introducing a regression

/hold

@sbueringer sbueringer changed the title 🐛 ClusterResourceSet: fix reconcile behavior if objects don't exist 🐛 ClusterResourceSet: continue applying if apply for a single cluster failed May 8, 2023
@sbueringer sbueringer changed the title 🐛 ClusterResourceSet: continue applying if apply for a single cluster failed 🐛 ClusterResourceSet: continue applying when apply for a single cluster failed May 8, 2023
@sbueringer
Copy link
Member Author

@fabriziopandini Reframed the PR (explanation here: #8611 (comment)).

I think now we can also merge this PR independent of the CR bump. I don't see a relevant overlap anymore

/assign @fabriziopandini
/assign @g-gaston

@sbueringer
ClusterResourceSet: continue aplying when apply for a single cluster …
506c570 
…failed

Signed-off-by: Stefan Büringer buringerst@vmware.com
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 8, 2023
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

g-gaston
g-gaston reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@g-gaston g-gaston left a comment

Choose a reason for hiding this comment

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

thanks for this @sbueringer
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d2cb933b109bc76bbb0a8e7d10cdc9fa914dc4e4

vincepri
vincepri approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2023
@fabriziopandini
Copy link
Member

/lgtm
/approve

ok to hold as you proposed

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [fabriziopandini,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sbueringer
Copy link
Member Author

@fabriziopandini I would merge now directly because of #8611 (comment) .

Basically this PR doesn't change the reqeue behavior for a CRS with missing resources anymore.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit a4f6608 into kubernetes-sigs:main May 9, 2023
10 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.5 milestone May 9, 2023
@sbueringer sbueringer deleted the pr-fix-crs branch May 9, 2023 09:15
@johannesfrey
Copy link
Contributor

/area clusterresourceset

@k8s-ci-robot k8s-ci-robot added the area/clusterresourceset Issues or PRs related to clusterresourcesets label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@g-gaston g-gaston g-gaston left review comments

@vincepri vincepri vincepri approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@fabriziopandini fabriziopandini

@g-gaston g-gaston

@killianmuldoon killianmuldoon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterresourceset Issues or PRs related to clusterresourcesets cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sbueringer @k8s-ci-robot @killianmuldoon @fabriziopandini @johannesfrey @vincepri @g-gaston