-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Recommendation API #4
Conversation
recommendation/v1alpha1/types.go
Outdated
|
||
// RecommendationSpec describes the recommendation type and what the recommendation is for. | ||
type RecommendationSpec struct { | ||
TargetRef autoscalingv2.CrossVersionObjectReference `json:"targetRef"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing omitempty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a mandatory field
recommendation/v1alpha1/types.go
Outdated
// RecommendationSpec describes the recommendation type and what the recommendation is for. | ||
type RecommendationSpec struct { | ||
TargetRef autoscalingv2.CrossVersionObjectReference `json:"targetRef"` | ||
Namespace string `json:"namespace"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to have cross namespace support for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
RecommendationSpec should refer to a specific workload, which is uniquely identified by 'namespace/apiversion/kind/name'
recommendation/v1alpha1/types.go
Outdated
|
||
type AdvancedHorizontalPodAutoscalerRecommendation struct { | ||
// optional | ||
MinReplicas *int32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about other properties in AHPA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this is a place holder, and @qmhu can provide more in future:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i will finish the recommendation for AHPA sooner
recommendation/v1alpha1/types.go
Outdated
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
// Analytics represents the configuration of an analytics. | ||
type Analytics struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be singlary format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analytics is uncountable noun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but kubernetes api need singular and plural format, then it is Analytics and Analytics, I am not sure if crdgen can handle such case, it might become Analytics and Analyticss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question here.
recommendation/v1alpha1/types.go
Outdated
|
||
// +genclient | ||
// +genclient:nonNamespaced | ||
// +kubebuilder:resource:scope=Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we define this object scope as Cluster, it means generally only admin can scan the app, but should we also consider app owner?
maybe:
set this object as namespaced, and if use create the object in crane-root, then it applies everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same comment to analytics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is SRE or admin's job to perform any resource optimization action,s and app owner should only care the APM rather than the underlying resource consumption.
recommendation/v1alpha1/types.go
Outdated
} | ||
|
||
// AnalyticsSpec describes the analytics type and what the analysis is for. | ||
type AnalyticsSpec struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the relation of analytics and recommendation?
users define analytics and analytics controller creates recommendation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addition, is recommendation and analytic is very generic words, should we consider if the reflected the real intension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analytics "generates" and "updates" Recommendations, which is similar to the relationship between CronJob and Job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about ResourceRecommendation & ResourceAnalytics?
|
||
go run ${KUBE_OPENAPI_PKG}/cmd/openapi-gen/openapi-gen.go --logtostderr \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openapi-gen and zz_generated.openapi is used now in metric-adapter, please keep it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
recommendation/v1alpha1/register.go
Outdated
package v1alpha1 | ||
|
||
import ( | ||
"github.com/gocrane/api/recommendation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please reformat this imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
recommendation/v1alpha1/types.go
Outdated
|
||
// Recommendation represents the configuration of a single recommendation. | ||
type Recommendation struct { | ||
metav1.TypeMeta `json:",inline"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is nicer to add blank lines here for spec and status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
recommendation/v1alpha1/types.go
Outdated
|
||
type AdvancedHorizontalPodAutoscalerRecommendation struct { | ||
// optional | ||
MinReplicas *int32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i will finish the recommendation for AHPA sooner
recommendation/v1alpha1/types.go
Outdated
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
// Analytics represents the configuration of an analytics. | ||
type Analytics struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question here.
recommendation/v1alpha1/types.go
Outdated
// Recommendations is a list of pointers to recommendations that are updated by this analytics. | ||
// +optional | ||
// +listType=atomic | ||
Recommendations []corev1.ObjectReference `json:"active,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analystics n <-> n Recommendation
Seems no need to add owner reference between them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just select them from label selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
recommendation/v1alpha1/types.go
Outdated
|
||
type AnalyticsType string | ||
|
||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about combine AnalyticsType and RecommendationType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…e.io" 2. Make Recommendation and Analytics namespace scoped. 3. Generate crd schemas for analysis.
analysis/v1alpha1/types.go
Outdated
|
||
const ( | ||
AnalyticsTypeAdvancedHPA AnalyticsType = "HPA" | ||
AnalyticsTypeResource AnalyticsType = "Resource" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Type/AnalysisType/g
/LGTM |
No description provided.