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

feat: add name & labels mapping store #1973

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

FabianKramm
Copy link
Member

What issue type does this pull request address? (keep at least one, remove the others)
/kind feature

Please provide a short message that should be published in the vcluster release notes
This PR adds a mapping store to vCluster that saves name and label mappings on the fly in a mapping store backend. This can be used to translate back resources including their metadata and simplifies importing as well as customising mappings.

@FabianKramm FabianKramm marked this pull request as draft July 23, 2024 10:11
Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for vcluster-docs canceled.

Name Link
🔨 Latest commit 9bea0d4
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/66a93f85fc86040008191676

@FabianKramm FabianKramm force-pushed the mappings-store branch 12 times, most recently from 86f4e38 to c55f5d5 Compare July 27, 2024 08:07
@FabianKramm FabianKramm force-pushed the mappings-store branch 10 times, most recently from e3726f4 to d199c06 Compare July 30, 2024 07:04
@FabianKramm FabianKramm marked this pull request as ready for review July 30, 2024 07:05
@FabianKramm FabianKramm force-pushed the mappings-store branch 2 times, most recently from 94e8583 to fc137d5 Compare July 30, 2024 10:01
@@ -143,6 +142,10 @@ func (s *importer) Syncer() syncertypes.Sync[client.Object] {
return syncer.ToGenericSyncer[*unstructured.Unstructured](s)
}

func (s *importer) Migrate(_ *synccontext.RegisterContext, _ synccontext.Mapper) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Migrate replaces basically the index we had before. Since we startup without "knowing" anymore what objects were how translated, we have the migrate function that initially populates the store with the virtual <-> host mappings

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can call the function MigrateMappings to make this more clear

@@ -16,8 +16,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

func New(ctx *synccontext.RegisterContext) (syncertypes.Object, error) {
mapper, err := ctx.Mappings.ByGVK(mappings.CSIDrivers())
func New(_ *synccontext.RegisterContext) (syncertypes.Object, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just remove the parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

this needs to adhere to the create syncer type function that uses this parameter for other syncers

PermitWithoutStream: true,
Endpoints: endpoints,
Context: ctx,
DialTimeout: 5 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This causes issues if you just want to query etcd for values as its trying to reach all peers

Comment on lines +38 to +41
if len(watches) == 0 {
return
}

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't that redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its avoiding a goroutine creation so I guess the small check is worth it

@FabianKramm FabianKramm force-pushed the mappings-store branch 9 times, most recently from 7a3005b to 774dc77 Compare July 30, 2024 19:13
@FabianKramm FabianKramm merged commit 8cea580 into loft-sh:main Jul 31, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants