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

✨ server: wire cache informers instead of root informers #2559

Merged
merged 9 commits into from
Jan 9, 2023
Merged

✨ server: wire cache informers instead of root informers #2559

merged 9 commits into from
Jan 9, 2023

Conversation

sttts
Copy link
Member

@sttts sttts commented Jan 6, 2023 •
edited
Loading

  • wire the cache informers into the relevant controllers that are needed to create and initialize a workspace.
  • add workspace types to replication to the cache server
  • fix workspace initialization VW on a non-root shard
  • add --location-selector flag to kubectl ws create plugin

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2023
@fgiloux
Copy link
Contributor

fgiloux commented Jan 6, 2023

Suggestion: do we want to use something like "global" instead of "cache" as a prefix for informers and like bound to the cache server? I am just concerned that "cache" is also used for the local (client-go) cache and it may not be obvious at first sight which cache is referred to.

fgiloux
fgiloux reviewed Jan 6, 2023
View reviewed changes
pkg/server/controllers.go
if err := s.waitForOptionalSync(hookContext.StopCh); err != nil {
logger.Error(err, "failed to finish post-start-hook")
return nil // don't klog.Fatal. This only happens when context is cancelled.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What guarantees that shard informers for the cache server have synced before APIExportEndpointSliceController is started?

Copy link
Member Author

Choose a reason for hiding this comment

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

does it matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

in general we are waiting for all informers now with exceptions of exports and logical clusters (we need these for bootstrapping).

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@sttts sttts changed the title WIP: ✨ server: wire cache informers instead of root informers ✨ server: wire cache informers instead of root informers Jan 6, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2023
@sttts sttts force-pushed the sttts-remote-to-cache branch from c7f5eb1 to d6fa7ba Compare January 6, 2023 12:28
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@sttts sttts force-pushed the sttts-remote-to-cache branch from d6fa7ba to 175f279 Compare January 6, 2023 12:52
@sttts sttts force-pushed the sttts-remote-to-cache branch from 175f279 to d4870c0 Compare January 6, 2023 12:56
p0lyn0mial
p0lyn0mial reviewed Jan 9, 2023
View reviewed changes
cmd/sharded-test-server/cache.go
@@ -40,11 +40,11 @@ import (
)

func startCacheServer(ctx context.Context, logDirPath, workingDir string) (<-chan error, string, error) {
red := color.New(color.BgHiRed, color.FgHiWhite).SprintFunc()
inverse := color.New(color.BgHiWhite, color.FgHiRed).SprintFunc()
cyan := color.New(color.BgHiCyan, color.FgHiWhite).SprintFunc()
Copy link
Contributor

Choose a reason for hiding this comment

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

why BgHiCyan is better than color.BgHiRed :)

p0lyn0mial
p0lyn0mial reviewed Jan 9, 2023
View reviewed changes
pkg/cliplugins/workspace/plugin/kubeconfig.go
@@ -486,6 +488,10 @@ func (o *CreateWorkspaceOptions) Complete(args []string) error {

// Validate validates the CreateWorkspaceOptions are complete and usable.
func (o *CreateWorkspaceOptions) Validate() error {
if _, err := metav1.ParseToLabelSelector(o.LocationSelector); 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.

is an empty LocationSelector a valid label ?

p0lyn0mial
p0lyn0mial reviewed Jan 9, 2023
View reviewed changes
pkg/reconciler/cache/replication/replication_reconcile.go
case tenancyv1alpha1.SchemeGroupVersion.WithResource("workspacetypes").String():
return c.reconcileObject(ctx,
keyParts[1],
tenancyv1alpha1.SchemeGroupVersion.WithResource("workspacetypes"),
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we could also extend our e2e tests to cover the new type

Copy link
Contributor

Choose a reason for hiding this comment

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

can be done in a follow-up

p0lyn0mial
p0lyn0mial reviewed Jan 9, 2023
View reviewed changes
pkg/server/controllers.go
@@ -683,8 +683,11 @@ func (s *Server) installAPIBindingController(ctx context.Context, config *rest.C
if err := wait.PollImmediateInfiniteWithContext(goContext(hookContext), time.Millisecond*100, func(ctx context.Context) (bool, error) {
crdsSynced := s.ApiExtensionsSharedInformerFactory.Apiextensions().V1().CustomResourceDefinitions().Informer().HasSynced()
exportsSynced := s.KcpSharedInformerFactory.Apis().V1alpha1().APIExports().Informer().HasSynced()
cacheExportsSynced := s.CacheKcpSharedInformerFactory.Apis().V1alpha1().APIExports().Informer().HasSynced()
schemasSynced := s.KcpSharedInformerFactory.Apis().V1alpha1().APIExports().Informer().HasSynced()
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be APIResourceSchemas

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in follow-up PR

@p0lyn0mial
Copy link
Contributor

/lgtm

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

sttts commented Jan 9, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit c591547 into kcp-dev:main Jan 9, 2023
@sttts sttts deleted the sttts-remote-to-cache branch January 9, 2023 11:59
@p0lyn0mial p0lyn0mial mentioned this pull request Jan 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgiloux fgiloux fgiloux left review comments

@p0lyn0mial p0lyn0mial p0lyn0mial left review comments

@davidfestal davidfestal Awaiting requested review from davidfestal

@stevekuznetsov stevekuznetsov Awaiting requested review from stevekuznetsov

Assignees

@p0lyn0mial p0lyn0mial

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sttts @fgiloux @p0lyn0mial @openshift-merge-robot