Issue 1:
Hey,

I've noticed that on 1.3.0-beta.1 when petset is deleted its pods stay alive:

core@master ~ $ kubectl get petsets | grep mariadb

mariadb-galera 3 3 7s

core@master ~ $ kubectl get pods | grep mariadb-galera

mariadb-galera-0 1/1 Running 0 5s

mariadb-galera-1 1/1 Running 0 4s

mariadb-galera-2 1/1 Running 0 3s

core@master ~ $ kubectl delete petset mariadb-galera

petset "mariadb-galera" deleted

core@master ~ $ kubectl get pods | grep mariadb-galera

mariadb-galera-0 1/1 Running 0 18s

mariadb-galera-1 1/1 Running 0 17s

mariadb-galera-2 1/1 Running 0 16s

core@master ~ $ kubectl get petsets | grep mariadb-galera

core@master ~ $

Cheers

Issue 2:
As part of #1702, we would like to use declarative configuration to maintain the state of a Kubernetes cluster. We do this by issuing the local objects to the server as JSON HTTP PATCH requests. As per the JSON merge patch spec, all maps are merged together, keys that are omitted from the patch are left untouched in the document, and if you want to delete a key you simply use key: null. This is exactly what we want in config reconciliation. However, lists present a special challenge in that the spec says they should always be replaced. Often that isn't what we want.

Let's say we start with the following Pod:

spec:
containers:
- name: nginx
image: nginx-1.0

...and we POST that to the server. Then a controller comes along and adds another container to this pod before it starts up. (Not possible today but likely a scenario we will want to support.)

spec:
containers:
- name: nginx
image: nginx-1.0
- name: log-tailer
image: log-tailer-1.0

If you try to apply the local config as a JSON PATCH to the remote config, you should get a no-op: the container that we've specified (nginx) still exists in the server, and extra containers inside the containers field are not of concern to us (and we should not clobber them). However, there is no way to programatically see if this is a list that should be merged versus a list that should not be merged. Here is an example of a list that should not be merged (the ExecAction API object), and instead should be fully replaced when it is modified:

command:
- nginx
- -t
- -c
- /etc/nginx.conf

If the remote server were to look like this...

command:
- httpd
- -X
- -f
- /etc/httpd.conf

...then if I were to patch the remote server with the local config, it should NOT merge these lists together - it should actually replace the command field wholesale with the one on disk.

The semantic knowledge of which lists should be merged like maps versus which lists should be replaced completely belongs in the API server. There are two ways to accomplish this.

Change mergeable lists to maps

If a list is mergeable, we should make it a map in the API server of a key to the objects. See a thorough discussion of this alternative to lists in #2004 (comment). The above example with pod spec would transform to this:

spec:
containers:
nginx:
image: nginx-1.0

And the remote side would look like this:

spec:
containers:
nginx:
image: nginx-1.0
log-tailer:
image: log-tailer-1.0

This enables us to merge the maps properly. If you wanted to actually delete the nginx container, you would put nginx: null in the local patch. (We have a design which will automatically generate the correct nulls of things you've deleted based on an annotation containing your previously submitted config - this design will be written into #1702.)

The big advantage of this method though is that it's really obvious what gets merged and what doesn't (lists are always replaced, maps are always merged) and we can use a standard JSON PATCH library to accept patches on the server and we can directly conform to the JSON merge patch spec. A side benefit is that the list is readily accessible in a meaningful way (indexed by primary key) which avoids manually indexing lists all the time.

The downside (as outlined in @jbeda's comment) is that the field to use as the key for the map is not always obvious. We can implement it as a swagger annotation to mitigate that. The bigger issue is that if a struct has no other required fields besides the map field, it can get awkward. For example, the PublicIP's field of the service spec is likely something we would want to merge. It's a []string which looks like this:

spec:
publicIPs:
- 67.1.1.10
- 67.1.1.11

If we made it a mergeable map (since you may want someone to inject a public IP into a service), then we'd have to change it to look like something like this:

spec:
publicIPs:
67.1.1.10:
ip: 67.1.1.10
67.1.1.11:
ip: 67.1.1.11

There is an optimization we could do where IP would be automatically filled by API server as a default, which would make it look like this:

spec:
publicIPs:
67.1.1.10: {}
67.1.1.11: {}

But in any case it's still a bit ugly. There may be other ways to beautify it and/or solve this problem - very open to suggestions here.

Swagger annotation to tell API server which lists should be merged

The alternative is to do the merge determination more dynamically. When API server gets a PATCH, it introspects into the lists and checks against a Swagger annotation to see where the list should be replaced or merged. If it's one that should be merged, it will need a mergeKey which tells it what key to use to do the merging. In the above case of containers, it would be name, but in the publicIP's case it would just uniqify and merge the values themselves. While this lets us not change the API, this requires us to implement and maintain a custom JSON PATCH implementation which will not be fun. It also goes against POLA when your PATCH request will not behave as the spec defines.

What should we do?

Neither of these options are fantastic, but I think we need to implement this distinction as part of the API. Please discuss and detail other options if you can think of any.

@bgrant0607 @smarterclayton @derekwaynecarr @mikedanese