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

🐛 controller: use the global informer to get Shards #2660

Conversation

p0lyn0mial
Copy link
Contributor

@p0lyn0mial p0lyn0mial commented Jan 20, 2023

Summary

since Shard resources are only on the root shard all controllers must use the global (cached) informer otherwise the list will always be empty on a non-root shard.

Related issue(s)

xref: #2596

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023
@hardys
Copy link

hardys commented Jan 23, 2023

Looks like something happened last week and a few PRs got stuck In merge pool - I had to close and reopen #2659 to get it to merge, so we may need to do the same here

@p0lyn0mial
Copy link
Contributor Author

Looks like something happened last week and a few PRs got stuck In merge pool - I had to close and reopen #2659 to get it to merge, so we may need to do the same here

I think the PR hasn't been merged because of failing e2e-sharded. Although tide is saying In merge pool. So maybe this will be my next issue (once the failing job is green). Thanks for letting me know!

FYI #2651 should deflake e2e-sharded.

@p0lyn0mial
Copy link
Contributor Author

/close

@openshift-ci openshift-ci bot closed this Jan 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

@p0lyn0mial: 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.

@p0lyn0mial
Copy link
Contributor Author

/reopen

@openshift-ci openshift-ci bot reopened this Jan 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 23, 2023

@p0lyn0mial: 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.

@p0lyn0mial p0lyn0mial force-pushed the controllers_global_inf_for_shards branch from a536d53 to f77a94f Compare January 23, 2023 13:37
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@p0lyn0mial
Copy link
Contributor Author

I rebased against the master and pushed because I wasn't sure if e2e-sharded pulled #2651

@p0lyn0mial
Copy link
Contributor Author

e2e-sharded is green, re-running just to make sure

@p0lyn0mial
Copy link
Contributor Author

e2e-sharded failed on TestCRDCrossLogicalClusterListPartialObjectMetadata for which I have created #2666

@@ -1274,7 +1274,7 @@ func (s *Server) installSyncTargetController(ctx context.Context, config *rest.C
c := synctargetcontroller.NewController(
kcpClusterClient,
s.KcpSharedInformerFactory.Workload().V1alpha1().SyncTargets(),
s.KcpSharedInformerFactory.Core().V1alpha1().Shards(),
s.CacheKcpSharedInformerFactory.Core().V1alpha1().Shards(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

still seeing some tests failing in test/e2e/syncer pkg. I will revert the change to synctargetcontroller controller for now.

since Shard resources are only on the root shard the apiexport
controller must use the global (cached) informer othweriwse
the list will always be empty on a non-root shard.
@p0lyn0mial p0lyn0mial force-pushed the controllers_global_inf_for_shards branch from f77a94f to 01d4f40 Compare January 23, 2023 14:38
@p0lyn0mial
Copy link
Contributor Author

/retest

@p0lyn0mial
Copy link
Contributor Author

/test e2e-sharded

1 similar comment
@p0lyn0mial
Copy link
Contributor Author

/test e2e-sharded

@sttts
Copy link
Member

sttts commented Jan 23, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@openshift-merge-robot openshift-merge-robot merged commit 7b3126c into kcp-dev:main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants