feat(kuma-cp) delete zone #1348

lobkovilya · 2020-12-22T13:17:40Z

Summary

Right now if someone is trying to delete ZoneResource in Global, all corresponding resources won't be gone. So Dataplanes and DataplaneInsights will stuck in the cluster until you explicitly wipe them out.

Also deleting ZoneResource while Remote Kuma CP in that zone is still alive, probably not a great idea. Since we create zone automatically there is the method createOrUpdateZone, which will actually be triggered if someone deletes ZoneResource. So the user's intention to delete a zone should ally with the actual Remote CP state. But at the same time, if we have networking issues between Remote CP and Global CP we shouldn't delete ZoneResource automatically, we should show this zone as offline.

So, in order to support these requirements current PR introduces new scenarios:

Delete zone

The user can't delete ZoneResource in Global while Remote CP is alive and connected. Current PR introduces validation for that:

zone: unable to delete Zone, Remote CP is still connected, please shut it down first

So the user has to shutdown Remote CP at first, and only then go and delete ZoneResource.
Also current PR adds ownership relation between synced resources and ZoneResource that allows to cleanup Dataplanes and DataplaneInsights automatically.

Disable zone

There are might be situations when the user doesn't have access to Remote CP but there is a need to stop routing traffic to some specific zone. Current PR introduces a new field in ZoneResources:

spec:
  enabled: true

Changing enabled: false will allow the user to exclude the zone's ingress from all other zones, so traffic won't be directed to the disabled zone. The disabled zone is considered as offline.

Documentation

TBD

Link to the website documentation PR

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

jakubdyszkiewicz · 2020-12-22T14:50:56Z

pkg/api-server/zones_ws.go

@@ -52,7 +52,7 @@ func toZones(rlist system.ZoneOverviewResourceList) Zones {
 	for _, overview := range rlist.Items {
 		zones = append(zones, Zone{
 			Name:   overview.GetMeta().GetName(),
-			Active: overview.Spec.ZoneInsight.IsOnline(),
+			Active: overview.Spec.GetZoneInsight().IsOnline() && overview.Spec.GetZone().GetEnabled(),
 		})
 	}
 	return zones


Is this API still used somewhere?

If I get it right it is used here:

Is it correct?

Checked the GUI code. Yeah, for some reason it is used. I was pretty sure we want to drop this in favor of insights.

I think we should have a column called enabled / disabled. Putting zone offline when we disable it is not exactly correct.

Let's keep it for now because I don't think it matters, but create a card for getting rid of this endpoint and changing the GUI

jakubdyszkiewicz · 2020-12-22T14:51:16Z

pkg/core/bootstrap/bootstrap.go

@@ -295,6 +296,10 @@ func initializeResourceManager(cfg kuma_cp.Config, builder *core_runtime.Builder
 	dpInsightManager := dataplaneinsight.NewDataplaneInsightManager(builder.ResourceStore(), builder.Config().Metrics.Dataplane)
 	customManagers[mesh.DataplaneInsightType] = dpInsightManager

+	zoneValidater := zone.Validator{Store: builder.ResourceStore()}


typo: validater

jakubdyszkiewicz · 2020-12-22T14:54:19Z

pkg/kds/global/components.go

+		if err := rm.Get(context.Background(), zone, store.GetByKey(util.ZoneTag(r), model.NoMesh)); err != nil {
+			kdsGlobalLog.Error(err, "failed to get zone", "zone", util.ZoneTag(r))
+			// since there is no explicit "enabled: false" sync anyway
+			return true


shouldn't this be false if we cannot find the zone?

Yeah, I think it can be false. I wasn't sure about ordering, what if there is no Zone when we build a Snapshot. But I see now that it's not cached or something and we build Snapshot from the scratch every time, so delay with Zone creation won't be a problem

Discussed and decided to go with return true to avoid unneeded deletions

jakubdyszkiewicz · 2020-12-22T14:57:19Z

pkg/kds/store/sync.go

@@ -116,13 +125,29 @@ func (s *syncResourceStore) Sync(upstream model.ResourceList, fs ...SyncOptionFu
 		}
 	}

+	zone := system.NewZoneResource()
+	if opts.Zone != "" {


in which case it can be ""?
Can we do the Get only when len(onCreate) > 0?

It will be "" if you didn't pass Zone("some_zone") option to the Sync.
Yes, good point with checking len(onCreate) > 0

jakubdyszkiewicz · 2020-12-22T15:00:12Z

api/system/v1alpha1/zone.proto

+message Zone {
+  // enable allows to turn the zone on/off and exclude the whole zone from
+  // balancing traffic on it
+  bool enabled = 1;


that's a breaking change. We could do a BoolWrapper and treat nil as true

Agree, we should!

Add a card that for 1.1 we should change this to true by default

Do you mean treat nil as false since 1.1?

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

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # api/go.sum # pkg/plugins/resources/k8s/native/go.sum

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

* feat(kuma-cp) delete and disable zones * fix(kuma-cp) display disabled zones as 'offline' * fix(kuma-cp) fix `kumactl inspect zone` tests Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> (cherry picked from commit 9d9ee96)

* feat(kuma-cp) delete and disable zones * fix(kuma-cp) display disabled zones as 'offline' * fix(kuma-cp) fix `kumactl inspect zone` tests Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> (cherry picked from commit 9d9ee96) Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya added 4 commits December 22, 2020 15:43

feat(kuma-cp) delete and disable zones

6d4c26e

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

feat(kuma-cp) make check

49f6595

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

feat(kuma-cp) make check

9f09436

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

fix(kuma-cp) display disabled zones as 'offline', fix tests

1e74014

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

lobkovilya requested a review from a team as a code owner December 22, 2020 13:17

nickolaev approved these changes Dec 22, 2020

View reviewed changes

fix(kuma-cp) fix kumactl inspect zone tests

5416887

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

jakubdyszkiewicz reviewed Dec 22, 2020

View reviewed changes

fix(kuma-cp) review

2fd1a9c

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

jakubdyszkiewicz approved these changes Dec 22, 2020

View reviewed changes

lobkovilya added 3 commits December 23, 2020 00:47

Merge branch 'master' into feat/delete-zone

058e9ed

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> # Conflicts: # api/go.sum # pkg/plugins/resources/k8s/native/go.sum

feat(kuma-cp) back to return true

2327884

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

feat(kuma-cp) replace 'do' with 'make'

0bd7429

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

nickolaev merged commit 9d9ee96 into master Dec 22, 2020

nickolaev deleted the feat/delete-zone branch December 22, 2020 18:55

nickolaev added the backport-to-stable label Dec 22, 2020

mergify bot mentioned this pull request Dec 22, 2020

feat(kuma-cp) delete zone (bp #1348) #1355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp) delete zone #1348

feat(kuma-cp) delete zone #1348

lobkovilya commented Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

lobkovilya Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

lobkovilya Dec 22, 2020

lobkovilya Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

lobkovilya Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

nickolaev Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

lobkovilya Dec 22, 2020

jakubdyszkiewicz Dec 22, 2020

feat(kuma-cp) delete zone #1348

feat(kuma-cp) delete zone #1348

Conversation

lobkovilya commented Dec 22, 2020

Summary

Delete zone

Disable zone

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment