-
Notifications
You must be signed in to change notification settings - Fork 852
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
fix: Validate replicaSchedulingType and replicaDivisionPreference #3014
fix: Validate replicaSchedulingType and replicaDivisionPreference #3014
Conversation
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
name: nginx-propagation
spec:
resourceSelectors:
- apiVersion: apps/v1
kind: Deployment
name: nginx
placement:
clusterAffinity:
clusterNames:
- member1
- member2
replicaScheduling:
replicaDivisionPreference: Weighted apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
name: nginx-propagation
spec:
resourceSelectors:
- apiVersion: apps/v1
kind: Deployment
name: nginx
placement:
clusterAffinity:
clusterNames:
- member1
- member2
replicaScheduling:
replicaSchedulingType: Divided |
Codecov Report
@@ Coverage Diff @@
## master #3014 +/- ##
==========================================
+ Coverage 38.61% 38.74% +0.12%
==========================================
Files 206 206
Lines 18808 18919 +111
==========================================
+ Hits 7263 7330 +67
- Misses 11115 11155 +40
- Partials 430 434 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
87f2cd9
to
99fa354
Compare
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.
BTW, policyv1 really needs to be promoted and collated because some fields are not decent enough🤔.
/assign |
99fa354
to
faf0840
Compare
@@ -255,6 +255,7 @@ type ReplicaSchedulingStrategy struct { | |||
// "Divided" divides replicas into parts according to number of valid candidate member | |||
// clusters, and exact replicas for each cluster are determined by ReplicaDivisionPreference. | |||
// +kubebuilder:validation:Enum=Duplicated;Divided | |||
// +kubebuilder:default=Divided |
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.
Emmm🤔, I think this solution does not have an effect on #3013 because the empty field is ReplicaDivisionPreference
.
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.
Yes, It looks good for this instruction.
But it can't fix #3013.
For the ReplicaDivisionPreference
, maybe we can't just set a default value by kubebuilder instruction, because it has a precondition that is when replicaSchedulingType is Divided
.
How about deal replicaSchedulingType
with the mutating webhook?
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.
+1. Meanwhile, I suggest we gather our own default function of all API objects in zz_generated.defaults.go
which generated by code-generator
and fill up with the default value in mutating webhook.
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'm interested in the defaults thing, is there an example?
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.
Take corev1.Pod
as an example. AFAIK, the code-generator
helps us generate zz_generated.defaults.go and we only need to implement the customized default function in defaults.go. Once the scheme is added, we could use these codes to dump all default values into an object.
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 code-generator helps us generate zz_generated.defaults.go
https://pkg.go.dev/k8s.io/code-generator/cmd/defaulter-gen
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.
Right, I have used this tool before.
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
faf0840
to
7a3207c
Compare
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.
/approve
Please @Garrybest take a look?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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:
Approvers can indicate their approval by writing |
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
name: nginx-propagation3
spec:
resourceSelectors:
- apiVersion: apps/v1
kind: Deployment
name: nginx
placement:
clusterAffinity:
clusterNames:
- member1
- member2
replicaScheduling:
weightPreference:
staticWeightList:
- targetCluster:
clusterNames:
- member1
weight: 1
- targetCluster:
clusterNames:
- member2
weight: 1
|
/lgtm Thanks😄. |
Signed-off-by: chaunceyjiang chaunceyjiang@gmail.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
Validate replicaSchedulingType and replicaDivisionPreference
Which issue(s) this PR fixes:
Fixes #3013
Special notes for your reviewer:
Does this PR introduce a user-facing change?: