-
Notifications
You must be signed in to change notification settings - Fork 859
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 that the InterpretDependency operation is absent in the validatin… #3052
fix that the InterpretDependency operation is absent in the validatin… #3052
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
==========================================
+ Coverage 38.96% 38.99% +0.02%
==========================================
Files 207 207
Lines 19365 19365
==========================================
+ Hits 7546 7551 +5
+ Misses 11362 11358 -4
+ Partials 457 456 -1
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. |
768874b
to
0eea3b0
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
Leave approve to @XiShanYongYe-Chang
[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 |
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.
Thanks a lot~
test/e2e/resourceinterpreter_test.go
Outdated
gomega.Eventually(func(g gomega.Gomega) (int, error) { | ||
resourceBinding, err := karmadaClient.WorkV1alpha2().ResourceBindings(configMapNamespace).Get(context.TODO(), resourceBindingName, metav1.GetOptions{}) | ||
g.Expect(err).NotTo(gomega.HaveOccurred()) | ||
|
||
klog.Infof("Attached ResourceBinding(%s/%s)'s schedule result is %v, existing schedule result is expected.", | ||
resourceBinding.Namespace, resourceBinding.Name, resourceBinding.Spec.RequiredBy) | ||
return len(resourceBinding.Spec.RequiredBy), nil | ||
}, pollTimeout, pollInterval).ShouldNot(gomega.BeZero()) | ||
}) | ||
}) | ||
}) |
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.
Maybe we can directly determine whether the cm is distributed to the target member 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.
Strictly speaking, we only need to check whether the dependent configmap is correctly interpreted, as for whether it is distributed to the relevant member clusters, I don't think it does care because the configmap is distributed is not the responsibility of the InterpretDependency
operation. Maybe there is wrong with the execution_controller
or binding_controller
?
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.
For other operations, propagating cm is unnecessary because it consumes extra resources and may affect our analysis of e2e when it goes wrong.
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 should clearly judge the resources that follow the distribution.
Maybe we can directly determine whether the cm is distributed to the target member cluster.
It's just a suggestion. The feature dependency distribute
is dependent on the two controllers you mentioned, so I think it's reasonable to test the entire process.
@@ -271,6 +280,23 @@ var _ = ginkgo.Describe("Resource interpreter webhook testing", func() { | |||
}) | |||
}) | |||
}) | |||
|
|||
ginkgo.Context("InterpreterOperation InterpretDependency testing", func() { |
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 set the relevant cm only for operation InterpretDependency, such as:
@@ -30,7 +30,6 @@ import (
var _ = ginkgo.Describe("Resource interpreter webhook testing", func() {
var policyNamespace, policyName string
var workloadNamespace, workloadName string
- var configMapNamespace, configMapName string
var workload *workloadv1alpha1.Workload
var policy *policyv1alpha1.PropagationPolicy
var cm *corev1.ConfigMap
@@ -40,10 +39,7 @@ var _ = ginkgo.Describe("Resource interpreter webhook testing", func() {
policyName = workloadNamePrefix + rand.String(RandomStrLength)
workloadNamespace = testNamespace
workloadName = policyName
- configMapNamespace = testNamespace
- configMapName = configMapNamePrefix + rand.String(RandomStrLength)
-
- workload = testhelper.NewWorkload(workloadNamespace, workloadName, configMapName)
+ workload = testhelper.NewWorkload(workloadNamespace, workloadName)
policy = testhelper.NewPropagationPolicy(policyNamespace, policyName, []policyv1alpha1.ResourceSelector{
{
APIVersion: workload.APIVersion,
@@ -55,19 +51,14 @@ var _ = ginkgo.Describe("Resource interpreter webhook testing", func() {
ClusterNames: framework.ClusterNames(),
},
})
- // configmaps should be propagated automatically.
- policy.Spec.PropagateDeps = true
- cm = testhelper.NewConfigMap(configMapNamespace, configMapName, map[string]string{"RUN_ENV": "test"})
})
ginkgo.JustBeforeEach(func() {
framework.CreatePropagationPolicy(karmadaClient, policy)
framework.CreateWorkload(dynamicClient, workload)
- framework.CreateConfigMap(kubeClient, cm)
ginkgo.DeferCleanup(func() {
framework.RemoveWorkload(dynamicClient, workload.Namespace, workload.Name)
framework.RemovePropagationPolicy(karmadaClient, policy.Namespace, policy.Name)
- framework.RemoveConfigMap(kubeClient, configMapNamespace, configMapName)
})
})
@@ -282,6 +273,28 @@ var _ = ginkgo.Describe("Resource interpreter webhook testing", func() {
})
ginkgo.Context("InterpreterOperation InterpretDependency testing", func() {
+ var configMapNamespace, configMapName string
+
+ ginkgo.BeforeEach(func() {
+ configMapNamespace = testNamespace
+ configMapName = configMapNamePrefix + rand.String(RandomStrLength)
+
+ workload.Spec.Template.Spec.Containers[0].EnvFrom = []corev1.EnvFromSource{
+ {ConfigMapRef: &corev1.ConfigMapEnvSource{
+ LocalObjectReference: corev1.LocalObjectReference{Name: configMapName},
+ Optional: pointer.Bool(true),
+ }}}
+
+ // configmaps should be propagated automatically.
+ policy.Spec.PropagateDeps = true
+ cm = testhelper.NewConfigMap(configMapNamespace, configMapName, map[string]string{"RUN_ENV": "test"})
+
+ framework.CreateConfigMap(kubeClient, cm)
+ ginkgo.DeferCleanup(func() {
+ framework.RemoveConfigMap(kubeClient, configMapNamespace, configMapName)
+ })
+ })
+
ginkgo.It("InterpretDependency testing", func() {
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.
You're right, but is it really necessary to tweak? maybe it will be used in the future for additional test cases, I think.
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.
You're right, but is it really necessary to tweak? maybe it will be used in the future for additional test cases, I think.
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 sorry I don't get 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.
Sorry, I typo the wrong words, what I want to say is:
For other operations, propagating comfigmap is unnecessary because it consumes extra resources and may affect our analysis of e2e when it goes wrong.
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.
Thanks for your suggestions. I'll improve the test case.
fbf8123
to
4bce472
Compare
@XiShanYongYe-Chang |
@whitewindmills Could you please squash your commits? |
…g webhook Signed-off-by: hejunhua <jayfantasyhjh@gmail.com> fix e2e problem Signed-off-by: hejunhua <jayfantasyhjh@gmail.com> fix test case Signed-off-by: hejunhua <jayfantasyhjh@gmail.com>
4bce472
to
f229d97
Compare
Ok, I will check it later. |
Thanks a lot~ |
Hi @whitewindmills, I think we need to cherry-pick it to the previous release, can you help do it? |
…k-of-#3052-upstream-release-1.4 Automated cherry pick of #3052: fix that the InterpretDependency operation is absent in the
…k-of-#3052-upstream-release-1.3 Automated cherry pick of #3052: fix that the InterpretDependency operation is absent in the
…k-of-#3052-upstream-release-1.2 Automated cherry pick of #3052: fix that the InterpretDependency operation is absent in the
The following error occurred when registering the InterpretDependency operation.
Signed-off-by: hejunhua jayfantasyhjh@gmail.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: