Skip to content
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

Graduate Schedule daemon set pods by default scheduler to beta and fix tests #67899

Merged
merged 2 commits into from Sep 12, 2018

Conversation

@ravisantoshgudimetla
Copy link
Contributor

ravisantoshgudimetla commented Aug 27, 2018

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

The main idea here is when ScheduleDSPods in enabled, even when there is a resource crunch, DSController would still create pod. Previously(before this feature), we would not allow that to happen.

As part of this PR,

  • Updating tests which does resource checks and fail to create/delete pods to run as before by unsetting ScheduleDSPod flag so that they can continue testing old code path. These tests are not relevant when ScheduleDSPods feature is enabled and would result in test failures.
  • Updating the create/delete pod events to reflect the new changes.

Release note:

Promote ScheduleDaemonSetPods by default scheduler to beta

/cc @bsalamat @aveshagarwal @Huang-Wei @k82cn

@k8s-ci-robot k8s-ci-robot requested a review from bsalamat Aug 27, 2018

@k8s-ci-robot k8s-ci-robot added the size/L label Aug 27, 2018

@k8s-ci-robot k8s-ci-robot requested review from aveshagarwal and k82cn Aug 27, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 27, 2018

@ravisantoshgudimetla: GitHub didn't allow me to request PR reviews from the following users: Huang-Wei.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Partly Fixes #66526. Built on top of #67790. The main idea here is when ScheduleDSPods in enabled, even when there is a resource crunch, DSController would still create pod. Previously(before this feature), we would not allow that to happen.

As part of this PR, I am skipping some tests which does resource checks and fails to create which are not relevant anymore when ScheduleDSPods feature is enabled and updating the create/delete pod events to reflect it.

Release note:

Promote ScheduleDaemonSetPods by default scheduler to beta

/cc @bsalamat @aveshagarwal @Huang-Wei @k82cn

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.

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Aug 27, 2018

/cc @janetkuo

@k8s-ci-robot k8s-ci-robot requested a review from janetkuo Aug 27, 2018

@@ -686,6 +686,9 @@ func allocatableResources(memory, cpu string) v1.ResourceList {

// DaemonSets should not place onto nodes with insufficient free resource
func TestInsufficientCapacityNodeDaemonDoesNotLaunchPod(t *testing.T) {
if utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {

This comment has been minimized.

@Huang-Wei

Huang-Wei Aug 27, 2018

Member

as utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) is used multiple times, maybe storing the value to a local variable in init() is better?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 27, 2018

Author Contributor

Sure, I can look into it. I am much more concerned about the PR as such, does it make sense to skip them or remove them? I personally feel that these tests should be in till this feature becomes GA.

@bsalamat
Copy link
Contributor

bsalamat left a comment

Thanks, @ravisantoshgudimetla! A few minor comments.

@@ -715,6 +718,9 @@ func TestInsufficientCapacityNodeDaemonDoesNotLaunchPod(t *testing.T) {

// DaemonSets should not unschedule a daemonset pod from a node with insufficient free resource
func TestInsufficientCapacityNodeDaemonDoesNotUnscheduleRunningPod(t *testing.T) {
if utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {

This comment has been minimized.

@bsalamat

bsalamat Aug 27, 2018

Contributor

Does this test fail when the feature is enabled? I would keep this test to ensure that DS controller does not react to out of resource nodes.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

There are create and delete events mismatch that would happen after enabling this feature. I think, we can modify them to reflect the new changes just like I did for other tests instead of skipping. I will make that change.

@@ -686,6 +686,9 @@ func allocatableResources(memory, cpu string) v1.ResourceList {

// DaemonSets should not place onto nodes with insufficient free resource
func TestInsufficientCapacityNodeDaemonDoesNotLaunchPod(t *testing.T) {
if utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {

This comment has been minimized.

@bsalamat

bsalamat Aug 27, 2018

Contributor

We have a test that verifies that DS controller creates pods for all appropriate nodes regardless of their schedulability, when this feature is enabled. Please delete the "skip" at line 458 to enable the test.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Sure.

@@ -1837,7 +1864,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) {
},
ds: &apps.DaemonSet{
Spec: apps.DaemonSetSpec{
Selector: &metav1.LabelSelector{MatchLabels: simpleDaemonSetLabel},
//Selector: &metav1.LabelSelector{MatchLabels: simpleDaemonSetLabel},

This comment has been minimized.

@bsalamat

bsalamat Aug 27, 2018

Contributor

Why is this commented out? If not needed, you should delete it.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Forgot to add it back. I am doing it now. Thanks :)

@@ -1847,7 +1874,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) {
},
},
wantToRun: true,
shouldSchedule: false,
shouldCreate: shouldCreate, //This is because we don't care about the resource constraints any more and let default

This comment has been minimized.

@bsalamat

bsalamat Aug 27, 2018

Contributor

I guess you have not finished the comment.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Done. Thanks.

@@ -686,6 +686,9 @@ func allocatableResources(memory, cpu string) v1.ResourceList {

// DaemonSets should not place onto nodes with insufficient free resource
func TestInsufficientCapacityNodeDaemonDoesNotLaunchPod(t *testing.T) {
if utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {

This comment has been minimized.

@k82cn

k82cn Aug 28, 2018

Member

Prefer to set ScheduleDaemonSetPods to false for those test until we upgrade to GA.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Klaus, I have disabled and enabled the featuregate as you suggested. PTAL.

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:ScheduleDaemonSetPods-beta branch from 6674635 to 8815f47 Aug 28, 2018

@@ -517,22 +502,21 @@ func TestSimpleDaemonSetScheduleDaemonSetPodsLaunchesPods(t *testing.T) {
pod.Name, len(nodeSelector.NodeSelectorTerms))
}

if len(nodeSelector.NodeSelectorTerms[0].MatchExpressions) != 1 {
if len(nodeSelector.NodeSelectorTerms[0].MatchFields) != 1 {
t.Fatalf("incorrect expression number of pod %s node selector term, expected: 1, got: %d.",

This comment has been minimized.

@Huang-Wei

Huang-Wei Aug 28, 2018

Member

s/expression/field/

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

According to [1] the two have the same type. I couldn't find any docs on why we would use one vs the other. Can you elaborate?

[1] https://godoc.org/k8s.io/api/core/v1#NodeSelectorTerm

This comment has been minimized.

@Huang-Wei

Huang-Wei Aug 28, 2018

Member

@misterikkit I think it's for injecting a specific nodeAffinity when ScheduleDaemonsetPods is enabled. The Key can only be "metadata.name", and Values can only be length 1 - validation logic is in place.

PR #62002 introduced the change.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

And the reason for the PR is labels cannot map arbitrary data(which was the case previously, so we introduced the fields with tight validations).

Discussion here - #61410

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

Thanks for the background. Now I understand.

@@ -517,22 +502,21 @@ func TestSimpleDaemonSetScheduleDaemonSetPodsLaunchesPods(t *testing.T) {
pod.Name, len(nodeSelector.NodeSelectorTerms))
}

if len(nodeSelector.NodeSelectorTerms[0].MatchExpressions) != 1 {
if len(nodeSelector.NodeSelectorTerms[0].MatchFields) != 1 {
t.Fatalf("incorrect expression number of pod %s node selector term, expected: 1, got: %d.",
pod.Name, len(nodeSelector.NodeSelectorTerms[0].MatchExpressions))

This comment has been minimized.

@Huang-Wei

Huang-Wei Aug 28, 2018

Member

s/MatchExpressions/MatchFields/

@@ -1847,7 +1899,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) {
},
},
wantToRun: true,
shouldSchedule: false,
shouldCreate: shouldCreate, //This is because we don't care about the resource constraints any more and let default scheduler handle it.

This comment has been minimized.

@Huang-Wei

Huang-Wei Aug 28, 2018

Member

nit: "//This" -> "// This"

@Huang-Wei
Copy link
Member

Huang-Wei left a comment

The changes looks good to me. Thanks @ravisantoshgudimetla !!

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Aug 28, 2018

Thanks for the review @bsalamat @k82cn @Huang-Wei. Updated the PR, PTAL.

/cc @misterikkit @mikedanese

@k8s-ci-robot k8s-ci-robot requested review from mikedanese and misterikkit Aug 28, 2018

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:ScheduleDaemonSetPods-beta branch from 8815f47 to d65539f Aug 28, 2018

@Huang-Wei Huang-Wei referenced this pull request Aug 28, 2018

Closed

REQUEST: New membership for Huang-Wei #47

6 of 6 tasks complete
@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Aug 28, 2018

/retest

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Aug 28, 2018

when there is a resource crunch, DSController would still create pod

by "create" do you mean "assign to node"?

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Aug 28, 2018

by "create" do you mean "assign to node"?

If you mean, updating pod.Spec.NodeName, we are not doing it, we are just setting node affinity, so that scheduler can assign. DS Controller would just create the pod with affinity set and scheduler does the actual node assignment.

@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Aug 28, 2018

@misterikkit I think it means "creating a pod with empty nodeName"

t.Fatalf("incorrect expression number of pod %s node selector term, expected: 1, got: %d.",
pod.Name, len(nodeSelector.NodeSelectorTerms[0].MatchExpressions))
if len(nodeSelector.NodeSelectorTerms[0].MatchFields) != 1 {
t.Fatalf("incorrect Field number of pod %s node selector term, expected: 1, got: %d.",

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

The grammar was weird before you touched it, so feel free to ignore this comment.

grammar nit: rather than "incorrect field number" (or incorrect expression number) it should read, "incorrect number of fields". Also, replace "of" with "in" or "for"

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Sure.

}
}()

utilfeature.DefaultFeatureGate.Set("ScheduleDaemonSetPods=false")

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

Could we add a comment about why the feature gate is disabled in this test?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Sure.

@@ -734,9 +725,17 @@ func TestInsufficientCapacityNodeDaemonDoesNotUnscheduleRunningPod(t *testing.T)
manager.dsStore.Add(ds)
switch strategy.Type {
case apps.OnDeleteDaemonSetStrategyType:
syncAndValidateDaemonSets(t, manager, ds, podControl, 0, 0, 2)
if !utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {
syncAndValidateDaemonSets(t, manager, ds, podControl, 0, 0, 2)

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

From reading these function calls, I have no idea what they are validating and whether the numbers are appropriate.

EDIT: Okay, so I went and read the code. syncAndValidateDaemonSets calls validateSyncDaemonSets. How are those two different? Well, the former is calling manager.syncHandler(key), which is the only real difference. What does syncHandler do? 🤷‍♂️ I couldn't tell you, because it is defined as,

// To allow injection of syncDaemonSet for testing.
syncHandler func(dsKey string) error

and I don't have more time to go digging through DSController code.

Anyway, validateSyncDaemonSets is checking counters for create, delete, and event.

@@ -697,6 +697,15 @@ func TestNotReadyNodeDaemonDoesLaunchPod(t *testing.T) {
}

func TestInsufficientCapacityNodeDaemonDoesNotLaunchPod(t *testing.T) {
// Look for TestInsufficientCapacityNodeWhenScheduleDaemonSetPodsEnabled, we don't need this test anymore.

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

So can we delete it?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

This is until we graduate the feature to GA, IIUC @k82cn wanted existing code to be tested till we remove it. After that we can delete this test.

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

Please update the comment to reflect this plan.

}
}()

utilfeature.DefaultFeatureGate.Set("ScheduleDaemonSetPods=false")

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

As above, why is the feature disabled in this test?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

We would launch(create) the pods, even with insufficient capacity on the nodeafter this feature has been enabled, it is the job of the scheduler to check for resource usage on the node.

@@ -1767,7 +1817,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) {
},
},
wantToRun: true,
shouldSchedule: false,
shouldCreate: shouldCreate,

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

If we care about test coverage for both enabled and disabled, can we move that to be one of the fields in the test struct? Having test cases with variable inputs makes me uncomfortable.

I think part of the reason I am confused is that sometimes you use literals, and sometimes you use the variables.

@@ -2029,8 +2079,8 @@ func TestNodeShouldRunDaemonPod(t *testing.T) {
if wantToRun != c.wantToRun {
t.Errorf("[%v] strategy: %v, predicateName: %v expected wantToRun: %v, got: %v", i, c.ds.Spec.UpdateStrategy.Type, c.predicateName, c.wantToRun, wantToRun)
}
if shouldSchedule != c.shouldSchedule {
t.Errorf("[%v] strategy: %v, predicateName: %v expected shouldSchedule: %v, got: %v", i, c.ds.Spec.UpdateStrategy.Type, c.predicateName, c.shouldSchedule, shouldSchedule)
if shouldSchedule != c.shouldCreate {

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

update the local variable name for shouldSchedule?

@@ -2053,6 +2103,7 @@ func TestUpdateNode(t *testing.T) {
ds *apps.DaemonSet
expectedEventsFunc func(strategyType apps.DaemonSetUpdateStrategyType) int

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

This is such an odd pattern. Clearly update strategy and expectedEvents int should be fields of the test struct. Out of scope for this PR, but I finding these tests difficult to read because of patterns like this.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

Sure, I can create a separate issue for this and it could be included in table driven tests issue that you created.

This comment has been minimized.

@misterikkit

misterikkit Aug 28, 2018

Contributor

I guess DS controller is technically owned by sig-scheduling... 😄

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Aug 28, 2018

Author Contributor

I think by sig-apps but sig-scheduling is partly responsible as well.

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Aug 28, 2018

we are just setting node affinity

Okay, I was confused by your PR description. It makes it sound more complicated than, "we changed the default flag value".

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:ScheduleDaemonSetPods-beta branch 2 times, most recently from 4305660 to 13b0365 Sep 6, 2018

@@ -407,19 +410,26 @@ func clearExpectations(t *testing.T, manager *daemonSetsController, ds *apps.Dae
}

func TestDeleteFinalStateUnknown(t *testing.T) {
for _, strategy := range updateStrategies() {
manager, _, _, err := newTestController()
var ScheduleDSPods = "ScheduleDaemonSetPods"

This comment has been minimized.

@janetkuo

janetkuo Sep 8, 2018

Member

Use features.ScheduleDaemonSetPods instead to avoid typo.

@@ -407,19 +410,26 @@ func clearExpectations(t *testing.T, manager *daemonSetsController, ds *apps.Dae
}

func TestDeleteFinalStateUnknown(t *testing.T) {
for _, strategy := range updateStrategies() {
manager, _, _, err := newTestController()
var ScheduleDSPods = "ScheduleDaemonSetPods"

This comment has been minimized.

@janetkuo

janetkuo Sep 8, 2018

Member

Rollback the feature gate once you're done

This comment has been minimized.

@janetkuo

janetkuo Sep 8, 2018

Member

You can package the whole thing into func forEachFeatureGate, following the pattern in integration test:

func TestOneNodeDaemonLaunchesPod(t *testing.T) {
forEachFeatureGate(t, func(t *testing.T) {
forEachStrategy(t, func(t *testing.T, strategy *apps.DaemonSetUpdateStrategy) {

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 8, 2018

Author Contributor

Thanks @janetkuo. I thought about creating a function like that but it needed changes in some other tests that we are not touching as part of the current change(within the same file though). Probably, I can handle this and changing var to a features.ScheduleDaemonSetPods in a separate PR for a refactor. Coming to rolling back the featuregate, I think it doesn't matter as we are enabling and disabling the flag for every test.

This comment has been minimized.

@janetkuo

janetkuo Sep 10, 2018

Member

Okay, please fix this in a follow-up PR. But I'd like to at least use features.ScheduleDaemonSetPods instead of "ScheduleDaemonSetPods" in this PR.

}
}
}()
err := utilfeature.DefaultFeatureGate.Set("ScheduleDaemonSetPods=false")

This comment has been minimized.

@janetkuo

janetkuo Sep 8, 2018

Member

Same comment here, use features.ScheduleDaemonSetPods to avoid typo

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:ScheduleDaemonSetPods-beta branch from 13b0365 to 0d34e1e Sep 11, 2018

@janetkuo

This comment has been minimized.

Copy link
Member

janetkuo commented Sep 12, 2018

/lgtm

@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Sep 12, 2018

Thanks a lot, @janetkuo and @ravisantoshgudimetla for pushing this through.

@tnozicka
Copy link
Contributor

tnozicka left a comment

/hold
I don't think we are manipulating the feature gate correctly

if !utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {
syncAndValidateDaemonSets(t, manager, ds, podControl, 0, 0, 2)
} else {
syncAndValidateDaemonSets(t, manager, ds, podControl, 1, 0, 0)

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

With the new DS scheduling I don't see a value in this branch. Scheduler is the one to decide resources and we create the pod no matter the capacity.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

Depending on DS Scheduling enabled or not, the number of events would vary, create in this case is varying because previously, we stopped even creating pod after delete when there were no sufficient resources. Now, as you suggested we are creating pod irrespective of resource crunch, that is why we need this branch. Without this, we have to refactor the syncAndValidateDaemonSets fn

// Rollback feature gate.
defer func() {
if enabled {
err := utilfeature.DefaultFeatureGate.Set("ScheduleDaemonSetPods=true")

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

should be using setFeatureGate

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

Yeah, probably a nit, I can handle that in a separate PR, given that we have past code freeze

enabled := utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods)
// Rollback feature gate.
defer func() {
if enabled {

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

this whole lambda could be extracted to a helper leading to

defer rollbackFetureGate(enabled)

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

Sure, as I told earlier, there are few places, where we can refactor, I don't want to do everything in this PR as the PR has grown from 139 lines to 1300 lines.

@@ -1741,14 +1908,20 @@ func setDaemonSetCritical(ds *apps.DaemonSet) {
}

func TestNodeShouldRunDaemonPod(t *testing.T) {

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

I don't see this tested for both cases of the feature gate enabled/disabled

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

Yup. I missed it. Thanks for catching.

if ds.Name == "ds" {
enqueued = true
}
if shouldRun != c.shouldCreate {

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

the naming looks inconsistent

var enqueued bool
for _, f := range []bool{true, false} {
setFeatureGate(t, features.ScheduleDaemonSetPods, f)
{

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

why this block?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

You are right. This is not necessary.

func TestAddPod(t *testing.T) {
for _, strategy := range updateStrategies() {
for _, f := range []bool{true, false} {
setFeatureGate(t, features.ScheduleDaemonSetPods, f)

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

if we are setting a global value we should set it back to what it was before running this subtest (applies globally)

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

So, if we are running the tests when the global value is enabled and disabled, I think it doesn't matter.

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

I though some of the tests depend on the value being set initially, if one of the test changes that it has side effects. Why would you do the rollback in those other tests?

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

You are right but say if one test has modified the flag it responsible for unsetting it. For example, TestSimpleDaemonSetScheduleDaemonSetPodsLaunchesPods, sets and unsets it. For rest of the tests, we are testing when the flag is enabled as well as disabled.

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

if a test setting enable and disable runs before a test that ask what the default value of that feature is, you have lost it

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Sep 12, 2018

Author Contributor

I think, in all the tests, we check the value(I believe you are calling this as default value). We set it or unset it explicitly. I don't mind adding rollback logic for every function.

This comment has been minimized.

@tnozicka

tnozicka Sep 12, 2018

Contributor

I guess we should either do the rollback in all the places or in none. Given it doesn't have a side effect in the unit tests I won't block the PR.
/hold cancel
/lgtm

@ravisantoshgudimetla ravisantoshgudimetla force-pushed the ravisantoshgudimetla:ScheduleDaemonSetPods-beta branch from 0d34e1e to b2e92f1 Sep 12, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 12, 2018

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Sep 12, 2018

@tnozicka
Copy link
Contributor

tnozicka left a comment

I guess we should either do the rollback in all the places or in none. Given it doesn't have a side effect in the unit tests I won't block the PR. We should fix it in a followup.
/hold cancel
/lgtm

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor Author

ravisantoshgudimetla commented Sep 12, 2018

Thanks @tnozicka for your review.

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, janetkuo, ravisantoshgudimetla, tnozicka

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5be0a0e into kubernetes:master Sep 12, 2018

18 checks passed

cla/linuxfoundation ravisantoshgudimetla authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.