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(kuma-cp) store VIP per Mesh #1114

Merged
merged 28 commits into from
Nov 16, 2020
Merged

feat(kuma-cp) store VIP per Mesh #1114

merged 28 commits into from
Nov 16, 2020

Conversation

lobkovilya
Copy link
Contributor

Summary

This PR allows getting rid of inefficient DataplaneToSameMeshDataplanesMapper and replace it with watcher of VIP state at the current Mesh.

Documentation

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

# Conflicts:
#	app/kuma-cp/cmd/run.go
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya requested a review from a team as a code owner October 27, 2020 06:42
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@@ -48,7 +48,7 @@ var _ = Describe("ClusterLoadAssignment Cache", func() {
var claCache *cla.Cache
var metrics core_metrics.Metrics

expiration := 500 * time.Millisecond
expiration := 2 * 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.

Interesting, why is this?

Base automatically changed from feat/k8s-perf-opt to master November 3, 2020 11:23
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

# Conflicts:
#	app/kuma-cp/cmd/run.go
#	pkg/core/resources/model/resource.go
#	pkg/plugins/bootstrap/k8s/cache/doc.go
#	pkg/plugins/bootstrap/k8s/cache/internal/cache_reader.go
#	pkg/plugins/bootstrap/k8s/plugin.go
#	pkg/plugins/extensions/k8s/context.go
#	pkg/plugins/resources/k8s/caching_converter.go
#	pkg/plugins/resources/k8s/converter.go
#	pkg/plugins/resources/k8s/store.go
#	pkg/plugins/runtime/k8s/controllers/mesh_controller.go
#	pkg/plugins/runtime/k8s/controllers/outbound_converter.go
#	pkg/plugins/runtime/k8s/controllers/pod_controller.go
#	pkg/plugins/runtime/k8s/controllers/pod_converter.go
#	pkg/plugins/runtime/k8s/controllers/pod_converter_test.go
#	pkg/plugins/runtime/k8s/plugin.go
#	pkg/plugins/runtime/k8s/webhooks/defaulter.go
#	pkg/plugins/runtime/k8s/webhooks/defaulter_test.go
#	pkg/plugins/runtime/k8s/webhooks/injector/injector.go
#	pkg/plugins/runtime/k8s/webhooks/mesh_validator.go
#	pkg/plugins/runtime/k8s/webhooks/validation.go
#	pkg/plugins/runtime/k8s/webhooks/validation_test.go
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
pkg/dns/persistence/meshed.go Outdated Show resolved Hide resolved
pkg/dns/persistence/meshed_test.go Outdated Show resolved Hide resolved
pkg/plugins/runtime/k8s/controllers/mesh_controller.go Outdated Show resolved Hide resolved
if err := r.ConfigManager.Create(ctx, &system.ConfigResource{}, store.CreateByKey(cmName, "")); err != nil {
if !kube_apierrs.IsAlreadyExists(err) {
return kube_ctrl.Result{}, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this with multicluster? I think this should be only created on the remote and standalone.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this would actually work great with the defaults/mesh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted logic from this place, now Persistence.Set create or update config:

func (m *Persistence) Set(mesh string, vips List) error {
	ctx := context.Background()
	name := fmt.Sprintf(template, mesh)
	resource := &config_model.ConfigResource{}
	create := false
	if err := m.configManager.Get(ctx, resource, store.GetByKey(name, model.NoMesh)); err != nil {
		if !store.IsResourceNotFound(err) {
			return err
		}
		create = true
	}

	jsonBytes, err := json.Marshal(vips)
	if err != nil {
		return errors.Wrap(err, "unable to marshall VIP list")
	}
	resource.Spec.Config = string(jsonBytes)

	if create {
		meshRes := &mesh_core.MeshResource{}
		if err := m.resourceManager.Get(ctx, meshRes, store.GetByKey(mesh, model.NoMesh)); err != nil {
			return err
		}
		return m.configManager.Create(ctx, resource, store.CreateByKey(name, model.NoMesh), store.CreateWithOwner(meshRes))
	} else {
		return m.configManager.Update(ctx, resource)
	}
}

to be the same for Universal and Kubernetes

}
if err := rt.Add(server, vipsSync); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it now the same with Universal? Can we moved it back to components?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see why this is a case. I think it's not a good idea to carry all the internals of DNS to the reconciler. What you really want to do is to Instruct VipAllocator to do the update on the mesh X. So my proposal is

  1. Include VipAllocator into Runtime
  2. Instead of having
func CreateOrUpdateVIPConfig(p *MeshedPersistence, rm manager.ReadOnlyResourceManager, r DNSResolver, ipam IPAM, mesh string) error {

This should be

func (d *vipsAllocator) CreateOrUpdateVIPConfig(mesh string) error {
  1. Pass VIP allocator to this reconciller and execute this method
  2. Move back all DNS component preparation into common components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to put VipAllocator into Runtime. I made it stateless and dependent only on runtime components, so now it's easy to create it from any place:

vipsAllocator, err := dns.NewVIPsAllocator(
	rt.ReadOnlyResourceManager(),
	rt.ConfigManager(),
	rt.Config().DNSServer.CIDR,
	rt.DNSResolver(),
)

if err != nil {
return err
}
global, err := p.Get()
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need global at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need it to have the same VIP for services across Meshes

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya merged commit 9b1857d into master Nov 16, 2020
@lobkovilya lobkovilya deleted the feat/vips-per-mesh branch November 16, 2020 19:56
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.

None yet

3 participants