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

start with a clean slate for future multicluster work #4805

Merged
merged 4 commits into from Jun 17, 2022

Conversation

steeling
Copy link
Contributor

@steeling steeling commented Jun 9, 2022

This removes all multicluster references so that we can start from scratch.

It's a lot of deletions, and I can peel this out into multiple PR's if that makes it more readable, just let me know!

@@ -88,7 +88,6 @@ The following table lists the configurable parameters of the osm chart and their
| osm.featureFlags.enableEgressPolicy | bool | `true` | Enable OSM's Egress policy API. When enabled, fine grained control over Egress (external) traffic is enforced |
| osm.featureFlags.enableEnvoyActiveHealthChecks | bool | `false` | Enable Envoy active health checks |
| osm.featureFlags.enableIngressBackendPolicy | bool | `true` | Enables OSM's IngressBackend policy API. When enabled, OSM will use the IngressBackend API allow ingress traffic to mesh backends |
| osm.featureFlags.enableMulticlusterMode | bool | `false` | Enable Multicluster mode. When enabled, multicluster mode will be enabled in OSM |
Copy link
Contributor

Choose a reason for hiding this comment

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

One thought: does removing these options violate our semver support guarantees? I believe all the multi-cluster stuff was alpha, but I do think this does break installation for users

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'll let others comment on
@trstringer @shashankram

Copy link
Member

Choose a reason for hiding this comment

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

@keithmattix how does this break installation?
We can always leave this feature flag behind as a no-op to be extra cautious.

Copy link
Contributor

@jaellio jaellio Jun 9, 2022

Choose a reason for hiding this comment

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

I think we could just add this to the release_notes.md as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I was thinking more generally about removing options/feature flags from meshconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya he's right, removing from a CRD warrants a version change. Leaving the field in for now, and marking as deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the field is kept in the CRD, removing the value here will break users of the chart if they either have existing scripts setting it or they upgrade with helm upgrade --reuse-values, either of which they would likely expect to succeed if the chart's major version is the same.

I think it's likely though that the number of people we'd break is literally zero with this change, but it's something to consider whenever making a change like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! Maybe we can say that support between versions is non-breaking when using the CLI directly, but if they play around with the underlying helm chart we don't offer guaranteed compatibility. That way we can add additional logic to the CLI.

I also think it could make sense to provide additional steps for upgrades. That is, an upgrade is backwards compatible, but you may have to follow some small set of steps. We should probably also say you can only upgrade from 1 version to +2 minor versions at a time, or something along those lines.

@steeling steeling force-pushed the feature/remove-mc branch 6 times, most recently from 9d4b32c to 4c81654 Compare June 10, 2022 13:06
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

It looks like we can remove KindGateway in addition since that's no longer referred to. And at that point it might be worth removing ProxyKind altogether.

osm/pkg/envoy/types.go

Lines 99 to 108 in 4c81654

// ProxyKind is the type used to define the proxy's kind
type ProxyKind string
const (
// KindSidecar implies the proxy is a sidecar
KindSidecar ProxyKind = "sidecar"
// KindGateway implies the proxy is a gateway
KindGateway ProxyKind = "gateway"
)

@@ -88,7 +88,6 @@ The following table lists the configurable parameters of the osm chart and their
| osm.featureFlags.enableEgressPolicy | bool | `true` | Enable OSM's Egress policy API. When enabled, fine grained control over Egress (external) traffic is enforced |
| osm.featureFlags.enableEnvoyActiveHealthChecks | bool | `false` | Enable Envoy active health checks |
| osm.featureFlags.enableIngressBackendPolicy | bool | `true` | Enables OSM's IngressBackend policy API. When enabled, OSM will use the IngressBackend API allow ingress traffic to mesh backends |
| osm.featureFlags.enableMulticlusterMode | bool | `false` | Enable Multicluster mode. When enabled, multicluster mode will be enabled in OSM |
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the field is kept in the CRD, removing the value here will break users of the chart if they either have existing scripts setting it or they upgrade with helm upgrade --reuse-values, either of which they would likely expect to succeed if the chart's major version is the same.

I think it's likely though that the number of people we'd break is literally zero with this change, but it's something to consider whenever making a change like this.

demo/run-osm-demo.sh Outdated Show resolved Hide resolved
@steeling
Copy link
Contributor Author

@nojnhuh tagging you since it won't let me quote reply directly..

Even if the field is kept in the CRD, removing the value here will break users of the chart if they either have existing scripts setting it or they upgrade with helm upgrade --reuse-values, either of which they would likely expect to succeed if the chart's major version is the same.

I think it's likely though that the number of people we'd break is literally zero with this change, but it's something to consider whenever making a change like this.

Agreed. Maybe we should consider if direct helm upgrades (as opposed to upgrades via the CLI) are supported by our upgrade policy. I would say no, and we should provide a single upgrade path.

Even then, upgrades might not be a magic one command, they may require a few steps.

@steeling
Copy link
Contributor Author

It looks like we can remove KindGateway in addition since that's no longer referred to. And at that point it might be worth removing ProxyKind altogether.

done! I'll save removing ProxyKind for the future (if we decide to do so)

Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
@shashankram shashankram merged commit e3700d6 into openservicemesh:main Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants