Precreate the Kafka ConfigMap, because the dispatcher will flap until it exists

[Harwayne]

Proposed Changes

• Precreate the Kafka ConfigMap, because the dispatcher will flap until it exists. And the controller doesn't create the ConfigMap until the first kafka Channel is reconciled.

Release Note

Applying the full YAML will cause the existing ConfigMap's data to be removed. It will be recreated the next time the Channel controller reconciles a Kafka Channel.

[Harwayne]

Should the ConfigMap be in a sibling bootstrap.yaml? It is only desired on the first install. We don't want it on subsequent upgrades (it will cause a short data plane outage, then again so will redeploying the StatefulSet which only has 1 replica).

[evankanderson]

What serving has been doing is to have default values for the configmap, and include only a commented template. It might be worth connecting with @mattmoor or @vagababov to see if we can learn anything from serving.

…

On Mon, Feb 25, 2019 at 11:33 AM Adam Harwayne ***@***.***> wrote: Should the ConfigMap be in a sibling bootstrap.yaml? It is only desired on the first install. We don't want it on subsequent upgrades (it will cause a short data plane outage, then again so will redeploying the StatefulSet which only has 1 replica). — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#831 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN_O4WM3v9p_gjzHnJZFZ_Z5NLTxlks5vRDqjgaJpZM4bIsdw> .

-- Evan Anderson <argent@google.com>

[vagababov]

The reason why we did this, was since if users override values in their YAML files and then new release is done, we'll rest those to our default values. So our suggested values were all moved to code as defaults and the ConfigMap YAMLs are basically examples of what keys we support.

[Harwayne]

The reason why we did this, was since if users override values in their YAML files and then new release is done, we'll rest those to our default values. So our suggested values were all moved to code as defaults and the ConfigMap YAMLs are basically examples of what keys we support.

We use knative/pkg's configmap.InformedWatcher, which will error on start up if the ConfigMap isn't present. So without replacing our reliance on configmap.InformedWatcher, changing the startup order, or changing configmap.InformedWatcher, I don't think we can be resilient to the ConfigMap not existing at startup.

@evankanderson How about I add a second file, bootstrap.yaml, which the instructions state to apply only during the initial install?

[vagababov]

You don't remove the config map, it's just empty.

…

On Tuesday, February 26, 2019, Adam Harwayne ***@***.***> wrote: The reason why we did this, was since if users override values in their YAML files and then new release is done, we'll rest those to our default values. So our suggested values were all moved to code as defaults and the ConfigMap YAMLs are basically examples of what keys we support. We use knative/pkg's configmap.InformedWatcher <https://github.com/knative/pkg/blob/41761b2c813c3e871ed02d1e137df6de8a97b68c/configmap/informed_watcher.go#L49>, which will error on start up if the ConfigMap isn't present <https://github.com/knative/pkg/blob/41761b2c813c3e871ed02d1e137df6de8a97b68c/configmap/informed_watcher.go#L112>. So without replacing our reliance on configmap.InformedWatcher, changing the startup order, or changing configmap.InformedWatcher, I don't think we can be resilient to the ConfigMap not existing at startup. @evankanderson <https://github.com/evankanderson> How about I add a second file, bootstrap.yaml, which the instructions state to apply only during the initial install? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#831 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAurXwmOQKs0oN5TSHEcU-HtaV4puWtLks5vRViPgaJpZM4bIsdw> .

[Harwayne]

You don't remove the config map, it's just empty.

How do we create an empty ConfigMap before first usage? Presumably either in the 'main' YAML, or a secondary bootstrap.yaml, right? The current PR puts it in the main YAML, which will cause overwrites if the YAML is applied again. Putting it into a bootstrap.yaml makes it less likely that mistake will occur.

[n3wscott]

Ping @vagababov

[vagababov]

Didn't think it dependend on me.
The config looks empty, so I guess it's alright. But here's the example: https://github.com/knative/serving/blob/master/config/config-domain.yaml.

[evankanderson]

/lgtm
/approve

/hold

Looks like this is the correct format; not sure if this is still needed.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, Harwayne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Harwayne,evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vaikas]

@evankanderson is there something that needs to be done here before merging?
@Harwayne still needed?

[Harwayne]

No, this is no longer needed. #1058 removed the need for this ConfigMap.