new Insights 'DataGather' CRD to allow on-demand data gathering #1365

tremes · 2022-12-01T14:26:16Z

API part for the enhancement proposal openshift/enhancements#1279

openshift-ci · 2022-12-01T14:28:22Z

Hello @tremes! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

tremes · 2022-12-01T14:29:58Z

insights/v1alpha1/0000_10_01_datagather.crd.yaml

+    listKind: DataGatherList
+    plural: datagathers
+    singular: datagather
+  scope: Namespaced


I am not sure whether this would be better as namespaced or clusterscoped.

Good question, what do we expect the usage pattern of this to be? If you have it namespaced, will you reconcile objects across all namespaces or just a single pre-defined namespace?

CC @deads2k interested to see what you think about this

One thought, given we are discussing this API owning pods/jobs, having it namespaced then pairs nicely with it being a job semantic. The pod would be created in the same namespace i guess

Let me put it this way. I think the periodic gathering will create the CR as well as the job in the openshift-insights namespace. The question is more about the on-demand gathering where the CR will be created by the user. I think it would make sense to watch only CRs created in one namespace (openshift-insights), because the jobs will be created there anyways...but I don't have so much experience in this area.

There are definitely other APIs with similar patterns, for example the machine API only operates on machines in the machine API namespace

So I could see that restriction as in keeping

JoelSpeed

Not done a thorough review of the API yet but a few initial comments

insights/v1alpha1/0000_10_01_datagather.crd.yaml

insights/v1alpha1/doc.go

JoelSpeed · 2022-12-01T14:52:19Z

insights/v1alpha1/0000_10_01_datagather.crd.yaml

+    listKind: DataGatherList
+    plural: datagathers
+    singular: datagather
+  scope: Namespaced


Good question, what do we expect the usage pattern of this to be? If you have it namespaced, will you reconcile objects across all namespaces or just a single pre-defined namespace?

CC @deads2k interested to see what you think about this

One thought, given we are discussing this API owning pods/jobs, having it namespaced then pairs nicely with it being a job semantic. The pod would be created in the same namespace i guess

insights/v1alpha1/techpreview.datagather.testsuite.yaml

insights/v1alpha1/types_insights.go

insights/v1alpha1/0000_10_01_datagather.crd.yaml

insights/v1alpha1/types_insights.go

JoelSpeed

I think this is pretty much good to go, few nits/suggestions for improvement

insights/v1alpha1/types_insights.go

JoelSpeed · 2022-12-16T14:21:19Z

insights/v1alpha1/types_insights.go

+
+type DataGatherStatus struct {
+	// conditions provide details on the status of the gatherer job.
+	// +listType=atomic


Conditions will definitely have a single writer? Normally conditions is a list type of map with the map key as the type field within the condition, which allows multiple actors to apply their own conditions, using an atomic list means only a single actor owns the whole list, that may be what you want, just want to duble check (same for gather status below)

Yes. I think so. Only the related job can update the corresponding CR.

insights/v1alpha1/types_insights.go

JoelSpeed · 2023-02-09T17:45:04Z

Did we ever make a conclusion on namespaced vs cluster scoped?

tremes · 2023-02-10T07:25:35Z

@JoelSpeed I think we didn't:

I am for the namespaced approach, but @deads2k opinion (as well as others opinions of course) is welcome :)

JoelSpeed

One small error on the list map key but otherwise I think we are pretty much ready to go on this.

A few things to consider:

@deads2k asked what a future where configuration for individual gatherers is needed, what that would look like and where disabled gatherers fits into that, I don't know that we resolved that conversation
Currently, you haven't included any CEL validation rules, is this something we are interested in? For example, since users can now set status themselves, it's good to set fields in status so that they can't be modified, eg, the insightsRequestID should never change, right? So we could make that immutable once set, likewise with start and finish times. You could also validate that the state of a DataGather never goes backwards, imagine a user trying to "restart" the gather, if they cleared the state, what would your operator do?
Are there any validations that you think need tests, we can add new tests to the integration suite to validate the transitions between states and check things like the regex values where this might be helpful

insights/v1alpha1/types_insights.go

JoelSpeed

Can we please add integration tests for the CEL validations you've added, I'd like to see at a minimum:

status is present in initial and has no startTime and startTime is added
status is present in initial and has no requestID and requestID is added
attempt to remove startTime/finishTime/insightsRequestID
Some validation of DataGatherState transitions

JoelSpeed · 2023-03-10T14:12:52Z

insights/v1alpha1/types_insights.go

+	RelatedObjects []ObjectReference `json:"relatedObjects,omitempty"`
+	// insightsRequestID is an Insights request ID to track the status of the
+	// Insights analysis (in console.redhat.com processing pipeline) for the corresponding Insights data archive.
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="insightsRequestID is immutable once set"


This might break, you probably want

Suggested change

// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="insightsRequestID is immutable once set"

// +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="insightsRequestID is immutable once set"

Hm let me experiment it with that more, but it's the example from https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/#immutablility-after-first-modification

OK this works for the string attribute, however it's not clear to me whether it's required or not (because it seems working fine without it)

as above, I suspect this may break, but I'm willing to loosen once you see it.

JoelSpeed · 2023-03-10T14:14:12Z

insights/v1alpha1/types_insights.go

+	Gatherers []GathererStatus `json:"gatherers,omitempty"`
+	// startTime is the time when Insights data gathering started.
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="startTime is immutable once set"


What's the zero value here, do you want to be able to change this form the zero value? Typically with a string we would include oldSelf == '' || at the beginning of this

Yes I think we want to change this from the zero value. Adding oldSelf == '' || doesn't work though. I am getting:

The CustomResourceDefinition "datagathers.insights.openshift.io" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[finishTime].x-kubernetes-validations[0].rule: Invalid value: apiextensions.ValidationRule{Rule:"oldSelf == '' || self == oldSelf", Message:"finishTime is immutable once set"}: compilation failed: ERROR: <input>:1:9: found no matching overload for '_==_' applied to '(timestamp, string)' | oldSelf == '' || self == oldSelf | ........^

If this doesn't work for you, I'm willing to adjust afterwards. I suspect you need a cast of some kind.

JoelSpeed · 2023-03-10T14:14:42Z

insights/v1alpha1/types_insights.go

+	StartTime metav1.Time `json:"startTime,omitempty"`
+	// finishTime is the time when Insights data gathering finished.
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="finishTime is immutable once set"


As above, not sure if we want to allow transition from the zero value

insights/v1alpha1/types_insights.go

deads2k · 2023-03-20T14:20:19Z

Very nice API.

I suspect Joel is correct and you'll hit some bumps where you cannot mutate as you need to. But these particular fields (not in general, but these specific ones), we could loosen for you if you hit a problem.

/approve
/lgtm

openshift-ci · 2023-03-20T14:23:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tremes

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

openshift-ci · 2023-03-20T14:24:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tremes

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

openshift-ci · 2023-03-20T14:39:43Z

@tremes: all tests passed!

Full PR test history. Your PR dashboard.

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022

tremes force-pushed the on-demand-insights branch from 108ccf3 to f9f4c00 Compare December 1, 2022 14:29

tremes commented Dec 1, 2022

View reviewed changes

tremes mentioned this pull request Dec 1, 2022

on demand Insights data gathering openshift/enhancements#1279

Merged

openshift-ci bot requested review from derekwaynecarr and jwforres December 1, 2022 14:32

JoelSpeed reviewed Dec 1, 2022

View reviewed changes

tremes force-pushed the on-demand-insights branch from f9f4c00 to 1712c6f Compare December 5, 2022 08:27

tremes changed the title ~~WIP new Insights 'DataGather' CRD to allow on-demand data gathering~~ new Insights 'DataGather' CRD to allow on-demand data gathering Dec 5, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2022

JoelSpeed reviewed Dec 5, 2022

View reviewed changes

JoelSpeed reviewed Dec 8, 2022

View reviewed changes

tremes force-pushed the on-demand-insights branch from 1d11d87 to 7ecc00b Compare December 12, 2022 14:24

JoelSpeed reviewed Dec 16, 2022

View reviewed changes

JoelSpeed reviewed Jan 27, 2023

View reviewed changes

insights/v1alpha1/types_insights.go Outdated Show resolved Hide resolved

tremes force-pushed the on-demand-insights branch from 2e53d63 to 73d6a01 Compare February 6, 2023 14:31

JoelSpeed reviewed Feb 9, 2023

View reviewed changes

insights/v1alpha1/types_insights.go Outdated Show resolved Hide resolved

tremes force-pushed the on-demand-insights branch from 2e74c2c to e5ab208 Compare February 13, 2023 13:07

JoelSpeed reviewed Feb 14, 2023

View reviewed changes

insights/v1alpha1/types_insights.go Outdated Show resolved Hide resolved

tremes force-pushed the on-demand-insights branch from dfcf20c to ba54f25 Compare February 17, 2023 09:11

JoelSpeed reviewed Mar 10, 2023

View reviewed changes

tremes force-pushed the on-demand-insights branch from 483e3ad to e34c952 Compare March 14, 2023 07:48

new Insights 'DataGather' CRD to allow on-demand data gathering

ba793e0

tremes force-pushed the on-demand-insights branch from a0c7946 to ba793e0 Compare March 20, 2023 14:20

deads2k added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2023

openshift-ci bot assigned deads2k Mar 20, 2023

openshift-merge-robot merged commit 0eafde6 into openshift:master Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new Insights 'DataGather' CRD to allow on-demand data gathering #1365

new Insights 'DataGather' CRD to allow on-demand data gathering #1365

tremes commented Dec 1, 2022 •

edited

openshift-ci bot commented Dec 1, 2022

tremes Dec 1, 2022

JoelSpeed Dec 1, 2022

tremes Dec 1, 2022

JoelSpeed Dec 1, 2022

JoelSpeed left a comment

JoelSpeed Dec 1, 2022

JoelSpeed left a comment

JoelSpeed Dec 16, 2022

tremes Jan 2, 2023

JoelSpeed commented Feb 9, 2023

tremes commented Feb 10, 2023

JoelSpeed left a comment

JoelSpeed left a comment

JoelSpeed Mar 10, 2023

tremes Mar 13, 2023

tremes Mar 14, 2023

deads2k Mar 20, 2023

JoelSpeed Mar 10, 2023

tremes Mar 14, 2023

deads2k Mar 20, 2023

JoelSpeed Mar 10, 2023

deads2k commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="insightsRequestID is immutable once set"
	// +kubebuilder:validation:XValidation:rule="oldSelf == '' \|\| self == oldSelf",message="insightsRequestID is immutable once set"

new Insights 'DataGather' CRD to allow on-demand data gathering #1365

new Insights 'DataGather' CRD to allow on-demand data gathering #1365

Conversation

tremes commented Dec 1, 2022 • edited

openshift-ci bot commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Feb 9, 2023

tremes commented Feb 10, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

openshift-ci bot commented Mar 20, 2023

tremes commented Dec 1, 2022 •

edited