New Insights config API proposal

[tremes]

This enhancement is proposing a new Insights config API including a way how to trigger the Insights data gathering on demand.

[bparees]

you're going to want feedback from @deads2k on your api specification

[tremes]

I updated the confg API proposal to make the API more lightweight. There are still some things to be decided (and then described):

• do we need to introduce unsupportedConfigOverrides field?
• this is related to the question above, but would it make sense to keep the original support secret way of config available since we will need to keep it for the backward compatible reasons (it's described in the enhancement but there is this option of basic auth for the ingress service, which means to store/config username, password somewhere)

[bparees]

would it make sense to keep the original support secret way of config available since we will need to keep it for the backward compatible reasons

i would think you have no choice but to keep it, as you say, for backwards compatible reasons.

The real question will be what to do if someone then updates the secret, after you have migrated the content to the new api. Do you re-migrate? do you ignore the change?

my inclination would be that as long as the secret exists, it is the source of truth, and if someone wants to fully migrate to the new mechanism, they need to remove the secret/stop editing the secret.

but if they create the secret in the future, that becomes the source of truth for the config again.

[soltysh]

Same question as above, what happens with ForceGatheringReason field after a successful run?

[soltysh]

I compared this with ForceRedeploymentReason in our operators, and they leave it set, so for consistency I'm fine leaving it set. You'll easily know when the field changes to initiate a new run from informers.

[soltysh]

I didn't notice that earlier, but since each gatherer has its own status GathererStatus, and within that you have an array of conditions GathererConditions you could probably change this Messages array to be just Message string I don't expect that much of data in it, based on my previous experience.

[soltysh]

One final nit, I'll let @deads2k or @mfojtik have final saying

[deads2k]

This seems transient and the rest seem permanent. I'd make two separate values under spect for them.

[soltysh]

@deads2k are you referring to forced gathering (transient) and the disablement of gatherers (permanent)? If so, that's already handled separately. This field is only for the former, the latter is done through DisabledGatherers below. Or are you talking about something else?

[deads2k]

discussed in slack. Also, this could get worked out in an API PR. It's important, but the fitting impact here is relatively minor

[deads2k]

This seems backwards. If someone is using this, the old should be retired and perhaps force-assigned to reflect these values.

[bparees]

If someone is using this, the old should be retired and perhaps force-assigned to reflect these values.

i might not be following what "force reassigned to reflect these values means" (forced in which direction), but fundamentally if someone has config in a secret, that config should take precedence over any default values implied by the introduction of this new api resorce with empty values, no?

and if they have existing tooling that's ignorant of this new api and continues to update the secret, i'd expect those config values to be respected, until such time as we actually remove support for the "secret-based" config api (which, being that it's an api in my mind, we can't do without some sort of deprecation process)

[tremes]

Another point is that the current proposal doesn't provide all the config options available via the support secret so we can't simply deprecate it IMO.

[soltysh]

@deads2k based on my understanding after talking with @tremes there is no clear goal yet, to fully replace these config secret, that's why the secret precedence here makes sense. Until there's a clear desire to move out of the config we can switch the priorities, but for now, given only limited set of functionality is supported there I'm ok with this approach.

[deads2k]

I like the direction of the API to introduce a real operator resource in operator.openshift.io and I suggest starting the implementation there. The API for insights.operator.openshift.io seems close enough to debate finer details in an API PR.

[tremes]

Based on yesterday's discussion, I removed the wording regarding the clearing of the "ForceGatherReason" attribute. The API PR is openshift/api#1193.

[soltysh]

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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 [soltysh]

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

[openshift-ci]

@tremes: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.