Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Allow users to remove a cluster from an existing multicluster ingress #58

Closed
nikhiljindal opened this issue Nov 9, 2017 · 10 comments
Closed
Milestone

Comments

@nikhiljindal
Copy link
Contributor

nikhiljindal commented Nov 9, 2017

Scenario:
User creates a multicluster ingress in clusters A, B and C. Now user wants to remove cluster C so that the multicluster ingress should be restricted to clusters A and B only.
If user runs kubemci create command again with clusters.yaml containing clusters A and B only, then GCLB will be updated correctly to send traffic only to clusters A and B, but the ingress resource will still exist in cluster C.

Current solution:
Delete the multicluster ingress all together (from all clusters) and recreate it in clusters A and B.

This is bad since this leads to downtime. Service will be unavailable while the multicluster ingress is being deleted and recreated.
We need a better solution.

cc @csbell @G-Harmon @madhusudancs @mdelio

@nikhiljindal
Copy link
Contributor Author

Possible solutions:

Add a --old-clusters flag to kubemci create

When this flag is present, kubemci will also delete the ingress from clusters that are in old-clusters list but not in --clusters list.

Add a kubemci remove-clusters command

We can add a specific remove-clusters command to remove clusters from existing multicluster ingresses.
(+) Provides guarantee that only the list of clusters that the multicluster ingress is spread to is changed. Nothing else should change.
(-) This will add extra complexity for consumers if they are using kubemci in automatic watch triggered mode. They will need to understand that a cluster was removed from the list and hence call kubemci remove-clusters with only the list of clusters from which it needs to be deleted.

@csbell
Copy link
Contributor

csbell commented Nov 9, 2017

This issue is particularly thorny. The CUJ is poor with either approach. Can you think of a way to leave breadcrumbs somewhere as part of the creation of the ingresses so that we can detect changes to the cluster list without requiring explicit user input? For example, can we somehow fit cluster info in the description field of a label on the global forwarding rule?

@nikhiljindal
Copy link
Contributor Author

Yes we do that for cluster names already. We store cluster names on the description field of forwarding rule (we use them for get-status output). So we can detect that cluster list changed.

But we do not (and do not want to) store credentials for those clusters anywhere. Unless we store credentials, we will need user input.

@madhusudancs
Copy link
Contributor

madhusudancs commented Nov 9, 2017

There are challenges in storing cluster info and using it later, esp. in kubemci's current form. Currently the tool uses local kubecontext to derive cluster information. These contexts are local to the machine where the create command is run. A different user might have different context and cluster names. There is no easy way to uniquely identify a cluster from the client side across machines. And this problem is not unique to multi-user scenarios. Even a single user can have different context/cluster names on different machines or they can rename context/cluster names on the same machine. We either want something akin to cluster identity or a centralized store such as cluster registry where clusters can be uniquely identified if we want to associate LB resources with cluster info.

@G-Harmon
Copy link
Contributor

G-Harmon commented Nov 9, 2017

Forgive the newbie question- What happens today if you do "kubemci create --force" with a pruned cluster list? Why does that cause downtime?

@nikhiljindal
Copy link
Contributor Author

Forgive the newbie question- What happens today if you do "kubemci create --force" with a pruned cluster list? Why does that cause downtime?

It does not cause downtime. But the ingress will not be deleted from the cluster which got pruned. kubemci will just ignore that cluster. So if ingress was there before, it will still remain there. All GCP resources will be updated though to not send traffic to that pruned cluster.

@nikhiljindal nikhiljindal added this to the v0.2 milestone Nov 29, 2017
@nikhiljindal nikhiljindal modified the milestones: v0.2, v0.4 Feb 21, 2018
@nikhiljindal
Copy link
Contributor Author

cc @mdelio Can you chime in on what you think we should do to provide a good user experience.

@mdelio
Copy link

mdelio commented Feb 28, 2018

I like the remove-clusters approach; it's most intuitive. A few thoughts from offline conversation with @nikhiljindal:

  • we should eventually add an "add-clusters" cmd to match "remove-clusters"
  • if the user attempts to remove all clusters of an ingress, then we should have a warning that asks them to use "delete" instead (and we will require "--force" to continue with just removing the clusters but not the GCLB instance)
  • if the user deletes the underlying cluster before running "remove-clusters", we should allow them to specify a "--force" flag to remove whatever we can

@nikhiljindal
Copy link
Contributor Author

#146 is adding the remove-clusters command

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Apr 4, 2018
Automatic merge from submit-queue (batch tested with PRs 62049, 62085). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Adding a test for kubemci remove-clusters

Ref GoogleCloudPlatform/k8s-multicluster-ingress#58

Adding an e2e test for `kubemci remove-clusters` command. 
The test creates an ingress and then removes it from all clusters.

cc @G-Harmon

```release-note
NONE
```
@nikhiljindal
Copy link
Contributor Author

This is now fixed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants