Add "revision" to mesh config/operator spec #1260

howardjohn · 2020-01-29T00:48:42Z

See design doc for more context:
https://docs.google.com/document/d/1d1z5PC8wvh9QiR3NesAaPm6w2HnE4-KIfjB7GB-_Wf8/edit#heading=h.qlmnvhzb2ib2

The intent here is to get this merged into 1.5 so we can support forward
compatibility for when the feature is fully completed in 1.6

rshriram · 2020-01-29T01:31:45Z

I do not want to call this environment! It’s not indicative and will lead to a host of confusion. Please figure out a better name. Also, since you are adding this, add cluster to the mesh config as well as it is high time we had a proper external api for the cluster useful in variety of multi cluster scenarios. (right now it’s in kube secrets yuk!)

howardjohn · 2020-01-29T01:39:02Z

The cluster stuff is far outside the scope of this PR.

I dug up when we last discussed this and apparently TOC thought "install" was a better choice. I disagree but if others think that's the right choice we can go with it.

rshriram · 2020-01-29T02:42:51Z

The cluster stuff is relatively easy to add. and I think we have a helm chart variable for that already :(. So we are not introducing a new one. Its actually defaulting to Kubernetes!

s/env/project/ ? i think thats the openshift term. How will citadel work? will there be cross mesh traffic? I dont even think it can work if each citadel has self signed randomly generated ca. auto mtls will break when traffic transits from one istio to another.

howardjohn · 2020-01-29T02:50:48Z

This is different than what openshift is doing. They have multi-tenancy, where the two things are isolation. In the istio case, this means like two different teams running two different meshes. What we are doing here is not multi-tenancy. There are multiple control planes, but they may not be owned by different teams, so I don't think project works - it implies that they are not really associated with eachother. I wouldn't expect a workload to canary between projects?

There will be cross mesh traffic. CA functionality works the same way as it does with multiple replicas, they are not all randomly generating, they will do it once and share that between all instances.

We have been testing multiple control plane for many months prior to this and it has proven to work very well

rshriram · 2020-01-29T02:57:30Z

then call it control plane version or control plane id.

howardjohn · 2020-01-29T03:03:07Z

So what is the label? Either istio.io/version or istio.io/controlPlaneVersion?

istio.io/version won't work because its an overload term in multiple ways (ie version 1.5.0 vs version "my-other-control-plane"), and its really confusing to have a istio.io/version label on a CR I think - is it the version of the CR or the control plane?

controlPlaneVersion again has the same problem with overloading - you can have multiple replicas of the same version of Istio, and is really long (this will be on namespaces, so its a pretty common thing people would be typing - we cannot get much shorter than istio.io/env)

smawson · 2020-01-29T17:56:57Z

controlPlaneReplica?
controlPlaneInstall?

env seems very wrong and overloaded, please don't use that. Environments to this point in Istio has meant VMs vs. K8s vs. Mobile etc.

control.istio.io/replica
installation.istio.io/replica

One of those would probably be good enough for now, at least better than environment.

howardjohn · 2020-01-29T18:08:02Z

I am not dead set on environment, but I think all the alternatives are far more confusing. Replica means "an exact copy" which is not what this is, and seems like there would be confusion between replicas of a pod vs the different control planes.

install I think is the next best option, but its a bit awkward to me. For example:

apiVersion: networking.istio.io/v1alpha3
kind: Sidecar
metadata:
  name: sidecar
  labels:
    istio.io/install: canary
spec:

is really confusing to me.

Environment has a lot of prior art being used in this way: https://en.wikipedia.org/wiki/Deployment_environment

smawson · 2020-01-29T20:33:18Z

I am 100% opposed to using environment :) These are not different environments. Deployment environments should be different meshes, not different control planes within the same mesh. So I really really dislike environment as the name for these.

istio.io/installation: canary

seems fine to me. So does any of

install.istio.io/version: canary
install.istio.io/revision: canary
install.istio.io/replica: canary

I used replica because that's how you referred to this in your text comment, different replicas of the same version, but any of those work for me.

My favorite looking at these is probably install.istio.io/version: canary, that seems very clear IMO.

howardjohn · 2020-01-29T21:03:41Z

Alright I will shift my preference to istio.io/revision: canary. I am not sure why we need an install. prefix on it? this will be a pretty common label so I am sure people will appreciate it being short. One benefit of revision is that is what Knative uses to describe a similar concept, so it has prior art.

I don't like version much because I think that is even more overloaded - if someone asks me "what version of Istio are you running" is the answer 1.5.0 or "canary".

So is istio.io/revision a reasonable choice?

smawson · 2020-01-29T21:28:39Z

The reason I suggested an "install" prefix was to allow disambiguation, we've talked about having a "canonical revision" label as part of the canonical service work, that one is something longer like service.istio.io/canonical-revision, so won't conflict with this, but I wouldn't want people to think they did that one when they really did this one. So being clear as part of the label what revision we're talking about seems useful?

howardjohn · 2020-01-29T21:45:22Z

I think the decision between the prefix vs no prefix is mostly about convenience/length vs ambiguity. I don't have a strong opinion there. But if we do have a prefix, now we need to come up with 2 names which is an even harder problem 🙂 .

If we do go with a prefix, I can't come up with a better one than install.istio.io/revision but I don't particularly like it because I don't want to tie multiple control planes too much to "installation". Installation is a one time event, where as the lifetime of multiple control planes is intentionally infinite. Definitely not a deal breaker for me though, I would be fine with that.

If no one else has opinions i'll switch this to install.istio.io/revision

howardjohn · 2020-01-29T22:07:55Z

I swapped the current PR to "revision" and also replaced resource_suffix which is un-implemented and will be replaced by this, so that we can have the API set in 1.5 and don't need to deprecate a field, etc etc. cc @ostromart

smawson

LGTM

I agree on the install thing since these live long, but couldn't come up with something better. Operator? Control?

I could sort of convince myself that each of these represents an "install" of Istio control plane, so more of a noun than a verb, but yeah its not perfect.

costinm · 2020-01-29T23:58:51Z

I like 'revision' because it is also the term used by KNative, which follows a similar model for their deployments. However 'install' doesn't seem right - there is no reason for each 'revision' to actually be associated with a new install of anything. Some 'revisions' may consist only on settings ( injection template, address of the XDS server, etc ) - and may actually be implemented by shared installations. In particular we can support multiple injector 'revisions' with a single webhook. And in some cases the 'revision' may consist of managed services that don't get installed, it just indicates that the namespace is using a particular (remote) XDS server.

…

On Wed, Jan 29, 2020 at 2:30 PM Sven Mawson ***@***.***> wrote: ***@***.**** approved this pull request. LGTM I agree on the install thing since these live long, but couldn't come up with something better. Operator? Control? I could sort of convince myself that each of these represents an "install" of Istio control plane, so more of a noun than a verb, but yeah its not perfect. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1260?email_source=notifications&email_token=AAAUR2XIBFIQK3A27KK33XTRAH7RNA5CNFSM4KM42E22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTR4I7I#pullrequestreview-350471293>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2RPNLXVLTKIEW6GXT3RAH7RNANCNFSM4KM42E2Q> .

costinm · 2020-02-01T00:04:32Z

mesh/v1alpha1/config.proto

+  // The name of the revision for the control plane. If this is set, only Istio configuration that is associated
+  // with this revision (by the `istio.io/rev` label) or configuration that is global (missing
+  // a `istio.io/rev` label) will be processed.
+  string revision = 50;


As discussed, this is not really needed and hard to set at least with kustomize.

The discovery address will be different for each revision - and used on injection, but we already have that.

costinm

Approved for the operator part.

For mesh config - see comment, I think we can drop it.

howardjohn · 2020-02-01T00:05:31Z

/hold

howardjohn · 2020-02-01T00:05:48Z

dropping mesh config

howardjohn · 2020-02-03T18:02:00Z

/cherrypick release-1.5

istio-testing · 2020-02-03T18:02:06Z

@howardjohn: #1260 failed to apply on top of branch "release-1.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	operator/v1alpha1/istio.operator.v1alpha1.pb.html
M	operator/v1alpha1/operator.pb.go
M	operator/v1alpha1/operator.proto
M	python/istio_api/operator/v1alpha1/operator_pb2.py
Falling back to patching base and 3-way merge...
Auto-merging python/istio_api/operator/v1alpha1/operator_pb2.py
CONFLICT (content): Merge conflict in python/istio_api/operator/v1alpha1/operator_pb2.py
Auto-merging operator/v1alpha1/operator.proto
CONFLICT (content): Merge conflict in operator/v1alpha1/operator.proto
Auto-merging operator/v1alpha1/operator.pb.go
CONFLICT (content): Merge conflict in operator/v1alpha1/operator.pb.go
Auto-merging operator/v1alpha1/istio.operator.v1alpha1.pb.html
CONFLICT (content): Merge conflict in operator/v1alpha1/istio.operator.v1alpha1.pb.html
Patch failed at 0001 Add istio.io/rev

In response to this:

/cherrypick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

(cherry picked from commit 7b905a0)

howardjohn requested review from dcberg, geeknoid, linsun, louiscryan, rshriram and smawson as code owners January 29, 2020 00:48

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 29, 2020

howardjohn assigned costinm Jan 29, 2020

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 29, 2020

smawson approved these changes Jan 29, 2020

View reviewed changes

howardjohn force-pushed the env/mesh branch from 2cba715 to d0fa6de Compare January 31, 2020 22:48

howardjohn changed the title ~~Add "environment" to mesh config~~ Add "revision" to mesh config/operator spec Jan 31, 2020

costinm reviewed Feb 1, 2020

View reviewed changes

costinm approved these changes Feb 1, 2020

View reviewed changes

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Feb 1, 2020

Add istio.io/rev

9a65f5b

howardjohn force-pushed the env/mesh branch from d0fa6de to 9a65f5b Compare February 1, 2020 00:08

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Feb 1, 2020

istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 1, 2020

istio-testing merged commit 7b905a0 into istio:master Feb 1, 2020

howardjohn added a commit to howardjohn/api that referenced this pull request Feb 3, 2020

Add istio.io/rev (istio#1260)

0a92e7c

(cherry picked from commit 7b905a0)

istio-testing pushed a commit that referenced this pull request Feb 4, 2020

Add istio.io/rev (#1260) (#1268)

d937755

(cherry picked from commit 7b905a0)

shamsher31 mentioned this pull request Mar 17, 2021

Revision is no longer experimental #1918

Closed

Add "revision" to mesh config/operator spec #1260

Add "revision" to mesh config/operator spec #1260

Uh oh!

Conversation

howardjohn commented Jan 29, 2020

Uh oh!

rshriram commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

rshriram commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

rshriram commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

smawson commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

smawson commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

smawson commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

howardjohn commented Jan 29, 2020

Uh oh!

smawson left a comment

Choose a reason for hiding this comment

Uh oh!

costinm commented Jan 29, 2020 via email

Uh oh!

costinm Feb 1, 2020

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Feb 1, 2020

Uh oh!

howardjohn commented Feb 1, 2020

Uh oh!

howardjohn commented Feb 3, 2020

Uh oh!

istio-testing commented Feb 3, 2020

Uh oh!

Uh oh!